Skip to content

Fix cursor jumping when doing character replacement - #1522

Merged
cpeel merged 1 commit into
DistributedProofreaders:masterfrom
cpeel:fix-text-processsor-jumping
Aug 28, 2025
Merged

cpeel merged 1 commit into
DistributedProofreaders:masterfrom
cpeel:fix-text-processsor-jumping

Conversation

@cpeel

@cpeel cpeel commented Aug 24, 2025

Copy link
Copy Markdown
Member

The JS to detect and automatically replace characters that aren't allowed on the site needs to adjust the cursor based on the grapheme length, not the JS internal UTF-16 string length. This solves this issue reported in the forums.

I'm going to rely on @chrismiceli to tell me if there's a better way to solve this 😁

Sandbox: https://www.pgdp.org/~cpeel/c.branch/fix-text-processsor-jumping/

@cpeel
cpeel requested review from 70ray, chrismiceli and srjfoo August 24, 2025 22:25
@cpeel cpeel self-assigned this Aug 24, 2025

@srjfoo srjfoo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

P2 project Special Character Test Project with music has 4 astral plane characters at the end of the project custom chars.

@chrismiceli

Copy link
Copy Markdown
Collaborator

This looks fine to me, but I don't see any behavior change. Based on the forum, it seems the issue is with the '𝑓' character having .length === 2. I don't see the replacement char there with an adjust set to 0. Would it make sense to swap conversion.length - 1; with [...conversion].length - 1;? That should get the most natural 'length' of a string based on graphemes and not utf-16 points. You can see @70ray doing this with the [...textDataElement.value].map((character) logic to get individual graphemes.

@cpeel

cpeel commented Aug 26, 2025

Copy link
Copy Markdown
Member Author

This looks fine to me, but I don't see any behavior change. Based on the forum, it seems the issue is with the '𝑓' character having .length === 2. I don't see the replacement char there with an adjust set to 0.

It's not unique to '𝑓', it's that generally the .length is getting UTF-16 character length. If you open a random project on TEST, open a page, and paste the character the cursor jumps around.

Would it make sense to swap conversion.length - 1; with [...conversion].length - 1;?

Maybe, what the heck does [..conversion] do? I don't even know what to look up to understand this JS syntax.

@chrismiceli

Copy link
Copy Markdown
Collaborator

This looks fine to me, but I don't see any behavior change. Based on the forum, it seems the issue is with the '𝑓' character having .length === 2. I don't see the replacement char there with an adjust set to 0.

It's not unique to '𝑓', it's that generally the .length is getting UTF-16 character length. If you open a random project on TEST, open a page, and paste the character the cursor jumps around.

Would it make sense to swap conversion.length - 1; with [...conversion].length - 1;?

Maybe, what the heck does [..conversion] do? I don't even know what to look up to understand this JS syntax.

:D [...iterator] essentially iterates an iterable and stores it in an array. The iterator for a string is unicode aware. So calling [...string].length returns a different value than string.length. Namely it returns the number of unicode code points handling surrogate pairs. It is mentioned here: https://dmitripavlutin.com/what-every-javascript-developer-should-know-about-unicode/#length-and-surrogate-pairs and is a common strategy for handling these kinds of .length issues.

And to my first point, I am not sure what change in this PR fixes the issue with 𝑓, but I would not be surprised if it just went right over my head.

@srjfoo

srjfoo commented Aug 26, 2025

Copy link
Copy Markdown
Member

And to my first point, I am not sure what change in this PR fixes the issue with 𝑓, but I would not be surprised if it just went right over my head.

The test project I mentioned above has some characters in it (at the end of the custom chars) that are from non-BMP characters. If you open a page in the main test sandbox (i.e. not the link above, but the same project in a regular TEST sandbox) and insert one or more of those characters and try deleting a regular character, the cursor should jump by the number non-BMP characters that you inserted. This doesn't happen in Casey's sandbox.

@chrismiceli

Copy link
Copy Markdown
Collaborator

And to my first point, I am not sure what change in this PR fixes the issue with 𝑓, but I would not be surprised if it just went right over my head.

The test project I mentioned above has some characters in it (at the end of the custom chars) that are from non-BMP characters. If you open a page in the main test sandbox (i.e. not the link above, but the same project in a regular TEST sandbox) and insert one or more of those characters and try deleting a regular character, the cursor should jump by the number non-BMP characters that you inserted. This doesn't happen in Casey's sandbox.

I see now. I knew the behavior from the forum, but wasn't sure how @cpeel's change resolved it. I just played with it in the debugger to confirm. Essentially the fix is to not adjust the end on characters that are not in the map and let the browser take care of it. I like that solution. I think then my recommendation is one more small change to avoid manually storing the adjust for the replacements and continue to use the .length - 1 logic. But instead of using string length, we use the iterable length to handle surrogate pairs.

The JS to detect and automatically replace characters that aren't
allowed on the site needs to adjust the cursor based on the grapheme
length, not the JS internal UTF-16 string length.
@cpeel
cpeel force-pushed the fix-text-processsor-jumping branch from 9e5b66b to 9732a68 Compare August 26, 2025 19:23
@cpeel

cpeel commented Aug 26, 2025

Copy link
Copy Markdown
Member Author

:D [...iterator] essentially iterates an iterable and stores it in an array. The iterator for a string is unicode aware. So calling [...string].length returns a different value than string.length. Namely it returns the number of unicode code points handling surrogate pairs.

Perfect, thank you very much for that, it was very helpful!

I've made the smallest surgical fix with this new knowledge which appears to resolve the issue.

@70ray

70ray commented Aug 27, 2025

Copy link
Copy Markdown
Collaborator

This looks fine to me, but I don't see any behavior change. Based on the forum, it seems the issue is with the '𝑓' character having .length === 2. I don't see the replacement char there with an adjust set to 0. Would it make sense to swap conversion.length - 1; with [...conversion].length - 1;? That should get the most natural 'length' of a string based on graphemes and not utf-16 points. You can see @70ray doing this with the [...textDataElement.value].map((character) logic to get individual graphemes.

I don't remember writing any of this. git blame thinks @chrismiceli wrote it.

@70ray 70ray left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes odd behaviour around 𝑓

@chrismiceli

Copy link
Copy Markdown
Collaborator

This looks fine to me, but I don't see any behavior change. Based on the forum, it seems the issue is with the '𝑓' character having .length === 2. I don't see the replacement char there with an adjust set to 0. Would it make sense to swap conversion.length - 1; with [...conversion].length - 1;? That should get the most natural 'length' of a string based on graphemes and not utf-16 points. You can see @70ray doing this with the [...textDataElement.value].map((character) logic to get individual graphemes.

I don't remember writing any of this. git blame thinks @chrismiceli wrote it.

It was me! Look at that.

@cpeel
cpeel merged commit 99f117a into DistributedProofreaders:master Aug 28, 2025
9 checks passed
@cpeel
cpeel deleted the fix-text-processsor-jumping branch August 28, 2025 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants