Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve iOS IME handling #30

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve iOS IME handling #30

wants to merge 1 commit into from

Conversation

lishid
Copy link
Contributor

@lishid lishid commented Feb 1, 2022

FIX: Avoids text duplication in some cases caused by weird behaviors of IME composition on iOS.

This might not be the best code to fix this issue, please let me know if there's a better way to achieve this.

This patch fixes a specific case of iOS IME composition - it doesn't seem to happen everywhere but I have one case that triggers this (notice the space in the outer span).

<span class="outer"><span class="inner">-</span> </span>

When the cursor is placed at the end, typing causes exponential character duplication. I've logged the events and it seems that the following happens:

  • compositionstart
  • MutationObserver event, during which
    • view.composing is false but view.inputState.compositionFirstChange is true.
    • m.oldValue is " " and m.target.nodeValue is " g" (assuming g is typed).
  • In this case, if flush is called, then iOS will immediately trigger another mutation and insert the entire composition string again (in this case, "g").
  • Each new character typed repeats this process, each time inserting a full copy of the composed string, causing exponential insertion.

This patch causes flushSoon to be used when that happens, which causes iOS to properly keep detecting the correct composed string in the DOM and avoids inserting a new copy.

RPReplay_Final1643727583.mov

@marijnh
Copy link
Member

@marijnh marijnh commented Feb 4, 2022

In this case, if flush is called, then iOS will immediately trigger another mutation and insert the entire composition string again (in this case, "g").

This tends to happen on Webkit browsers when the selection or DOM is modified by the script during a composition. So the root cause of the issue is probably in the composition-protection code (CompositionWidget in docview.ts) somehow failing to do what it should do (create a decoration that 'pins' the DOM around the selection, preventing the view from messing with it).

@lishid lishid force-pushed the main branch 3 times, most recently from 1c991bf to d347b8f Compare Feb 22, 2022
@marijnh
Copy link
Member

@marijnh marijnh commented Feb 22, 2022

Does using view.compositionStarted instead of view.composing in this check (and making no other changes) help with this issue for you?

@lishid
Copy link
Contributor Author

@lishid lishid commented Feb 22, 2022

Does using view.compositionStarted instead of view.composing in this check (and making no other changes) help with this issue for you?

I can give it a try. If I remember correctly it would not be sufficient because of this part:

m.oldValue is " " and m.target.nodeValue is " g" (assuming g is typed).

This causes the check to fail: m.oldValue!.length > m.target.nodeValue!.length because it's expecting the new value to be shorter than the previous value.

@lishid
Copy link
Contributor Author

@lishid lishid commented Feb 22, 2022

I have just tested that removing m.oldValue!.length > m.target.nodeValue!.length is indeed required to properly fix the issue from the video.

@lishid
Copy link
Contributor Author

@lishid lishid commented Feb 22, 2022

Updated PR to use view.compositionStarted instead.

marijnh added a commit that referenced this issue Feb 23, 2022
FIX: Fix an issue where the editor would compute DOM positions inside composition
contexts incorrectly in some cases, causing the selection to be put in the wrong
place and needlessly interrupting compositions.

Issue #30
@marijnh
Copy link
Member

@marijnh marijnh commented Feb 23, 2022

I think I've found the root cause of this issue (though I've only reproduced a very-similar looking issue in Safari since I don't have an iOS device handy) and fixed it in attached patch. Could you take a look and see if it helps for you?

@lishid
Copy link
Contributor Author

@lishid lishid commented Feb 23, 2022

This seems to have fixed the underlying bug which generates extra characters, definitely an improvement.

The patch seems to have introduced a small visual glitch with the cursor though. I'm hoping it's a simple fix 🙏

GCCV3250.MP4

FIX: Avoids text duplication in some cases caused by weird behaviors of IME composition on iOS.
marijnh added a commit that referenced this issue Mar 3, 2022
FIX: Fix a bug where mapping positions to screen coordinates could return incorred
coordinates during composition.

Issue #30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants