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

Korean IME input issue on Chrome after the latest update #1484

Closed
wisdomcrane opened this issue Aug 28, 2024 · 18 comments
Closed

Korean IME input issue on Chrome after the latest update #1484

wisdomcrane opened this issue Aug 28, 2024 · 18 comments

Comments

@wisdomcrane
Copy link

wisdomcrane commented Aug 28, 2024

What is the issue?

After the latest Chrome update, there is an issue with typing Korean characters using IME in the ProseMirror editor. When typing Korean and pressing Enter, the last character disappears or is not displayed correctly.

Steps to reproduce

  1. Open a ProseMirror editor in Chrome (version 128.0.6613.85 - latest build, 64 bit).
  2. Switch to a Korean IME (e.g., 2-set Korean keyboard).
  3. Type any Korean word, such as "한글".
  4. Press Enter.

Expected behavior

The typed Korean word should remain intact in the editor after pressing Enter.

Actual behavior

The last Korean character is removed or does not display correctly after pressing Enter.

Environment

  • Chrome version: 128.0.6613.85
  • ProseMirror version:
    "prosemirror-codemirror-block": "^0.5.7",
    "prosemirror-commands": "^1.6.0",
    "prosemirror-example-setup": "^1.2.3",
    "prosemirror-keymap": "^1.2.2",
    "prosemirror-markdown": "^1.13.0",
    "prosemirror-model": "^1.22.3",
    "prosemirror-schema-basic": "^1.2.3",
    "prosemirror-schema-list": "^1.4.1",
    "prosemirror-tables": "^1.5.0",
    "prosemirror-view": "^1.34.1",
  • Operating System: Windows 10

Additional context

  • This issue started occurring after updating Chrome to version 128.0.6613.85 (TODAY, 2024-8-28)
  • The issue does not occur in Firefox.
  • This issue is specific to Korean IME input (May be CJK); English and direct typing without IME work correctly.
  • Screenshot
    image
    Enter
    image

Workaround

By returning true in the compositionend event handler, I was able to stop the unexpected update.

But I think this is workaround now. It is not recommend, becuase it can break the system.

Please let me know if you need more information or if I can assist further in reproducing this issue.

@marijnh
Copy link
Member

marijnh commented Aug 28, 2024

Type any Korean word, such as "한글".

Could you tell me which physical keys you press to type that (since I have no idea)? Does this happen for all characters or just specific ones?

@wisdomcrane
Copy link
Author

wisdomcrane commented Aug 28, 2024 via email

@marijnh
Copy link
Member

marijnh commented Aug 30, 2024

Looks like the browser was, in this situation, firing a keydown even before notifying the script that the selection changed (which is generally done before any input events fire). Attached patch makes the library actively query the DOM selection before handling key events.

@wisdomcrane
Copy link
Author

wisdomcrane commented Aug 31, 2024 via email

@Nantris
Copy link

Nantris commented Sep 27, 2024

I noticed this change breaks some, frankly, pretty funky stuff we're doing in TipTap to workaround the inability to cancel most events on Android. It only breaks one test in our hundreds and it's not a Prosemirror bug, but this is plaguing us nonetheless.

I wonder if there's any way we can prevent this behavior in a select circumstance? I tried forcibly setting view.domObserver.suppressingSelectionUpdates = true but that doesn't do the trick. I wonder if there's anything else I could try?

@Nantris
Copy link

Nantris commented Sep 27, 2024

After studying flush I thought maybe we could circumvent this by using view.domObserver.flushingSoon = 1 but still we hit an error on this line:

domSel.collapse(anchorDOM.node, anchorDOM.offset); - specifically Uncaught IndexSizeError: Failed to execute 'collapse' on 'Selection': The offset 3 is larger than the node's length

I know none of this is highly advisable. We can always use a custom fork or patch-package, but both are super painful to maintain and probably even more prone to bugs in the long run. We can avoid upgrading, but that can't go on forever. Any possible alternative approach would be most welcome, if there might be any.

@marijnh
Copy link
Member

marijnh commented Sep 27, 2024

Without info on what that test is trying to do, I can't say much here. But yeah, reaching into random internals to try and influence code paths seems ill-advised.

@Nantris
Copy link

Nantris commented Oct 2, 2024

Thanks for your reply @marijnh.

Basically we're invoking the editor to set a selection inside of a setTimeout but now that selection logic breaks with this change.

The entirety of the logic is pretty complex and I haven't had time to see if we can fix this by instead setting a flag in beforeinput and then setting the selection in appendTransaction but I'm not hopeful that it will produce the necessary results even though it seems theoretically more sound than what we have now.

I wish Android's webview and/or IME would let you override them instead of forcing you to fight them so such crazy things wouldn't be necessary to begin with...

@Nantris
Copy link

Nantris commented Oct 4, 2024

Unfortunately filterTransaction can't stop the Android events. The only approach I can envision then, to work around this, is to use appendTransaction to basically rebuild the intended state of the editor and the selection but there's no guarantee that would work either. And if it doesn't, are we out of luck?

Is there any way we could circumvent this logic on a case-by-case basis, or any other way this could be implemented that might not conflict with existing logic?

Edit: I did a "quick" (painstaking) test and I think this can be worked around, but I'm not certain yet.

@Nantris
Copy link

Nantris commented Oct 10, 2024

Working around this The Right Way™ is possible but it causes confusing selection logic on Android, both out of the box and after undo due to us having to manually correct the selection.

I don't really know how to fully outline the problem since the relevant code is literally hundreds of lines, and there doesn't really seem to be any way to resolve this fully from our end. It's not a showstopper, but it is a downgrade in the quality of our editor at the end of the day.

I think the root cause here is that we were relying on setting a custom selection, let's say, { from: 4, to: 33 } and that selection could include display: none content. On Android now, in the beforeinput the selection collapses to 33 which requires several workarounds. This in itself is a pain but not necessarily a problem. The problem seems to occur in that we then need to manually set the selection to 4 after the deletion occurs as a separate transaction, but upon undo, this selection setting is not undone to the last place they modified text as the user might expect since the selection is now being set explicitly. In 1.34.2 this wasn't an issue.

I think the root problem might be that after deleting the expanded selection that includes display: none content, the selection is collapsing to the end point, or the nearest available position (eg 33), rather than to the start (eg 4). This then necessitates we set the selection manually which affects how undo handles it.

I know this is a terrible bug report - I just don't know how to better describe or demonstrate the issue.

Any thoughts? Sorry I can't come up with a better way to communicate this.

@Nantris
Copy link

Nantris commented Oct 10, 2024

I wonder if this PR is necessary on Android? All our woes would disappear if this behavior could be gated to only affect desktop environments - though I'm not sure that's viable.

@marijnh
Copy link
Member

marijnh commented Oct 10, 2024

Do I understand correctly that the issue you're running into occurs when someone starts typing with your custom selection active, because the browser mangles the selection right before the typing starts, causing ProseMirror to read an unintended small selection, and thus not overwriting the content it should overwrite?

Is it possible to try and condense this down to a small example script whose behavior I could observe on my side?

@Nantris
Copy link

Nantris commented Oct 11, 2024

Upon further reflection, I can't honestly say I know exactly where this behavior is coming from.

Previously my findings were based on ever so slightly different implementations, but now that I have a fully working implementation in 1.34.3 (except this undo issue) I downgraded to 1.34.2 with an identical implementation and still see the discrepancy.

I'm not certain how to reproduce this at the moment. It's theoretically possible I could make a repro, but we're already shipping mega-late, the logic isn't straightforward, it's a relatively minor issue, and without knowing what part(s) are required to reproduce the issue I can't imagine when I'd be able to find the time to properly repro this in the near future.


I'm not sure of the value, but I did at least capture screen recordings of the discrepancy in the hopes that something might jump out to you.

To briefly explain the setup here, the red selected widget represents text that's programmatically selected, but is not displayed.

Not sure why the videos are so dark. Sorry about that!


Additional findings: Chrome on PC is unaffected. Safari is unaffected. Only Chrome for Android (and Android webview) seem to be affected.


[email protected]

ProseMirror-1.34.2.mp4

[email protected]

ProseMirror-1.34.3.mp4

@Nantris
Copy link

Nantris commented Oct 11, 2024

This particular issue appears to possibly occur in TipTap 2.7.0 which theoretically should use [email protected] so I'll have to test more to see if this is really the same issue.

This uses TipTap, but I just happened upon another circumstance where Android seems to product the same error I was originally seeing when you hold backspace down and delete long enough through listItems (eg Uncaught IndexSizeError: Failed to execute 'collapse' on 'Selection': The offset 3 is larger than the node's length )

Tested on Android 14 with GBoard.

https://codesandbox.io/p/sandbox/heuristic-agnesi-hn5slf

@dineug
Copy link

dineug commented Oct 28, 2024

https://issues.chromium.org/issues/363266897

In Chromium version 128, there was a bug related to selection due to IME changes, but this has been fixed and deployed in versions 128.xx, 129.xx, and 130.xx

If you need the specific code for this fix, rolling back should be feasible if necessary.

@Nantris
Copy link

Nantris commented Oct 28, 2024

@dineug great find!

@wisdomcrane I wonder if you test an older version of prosemirror-view on a newer Chromium version, is this issue now resolved for you? There would be benefits in our case if this change were to be rolled back - though I could see an argument for keeping it for a short while until Chromium 128 is no longer widely used. In theory it should already largely be replaced as Chrome is on 130 now.

@wisdomcrane
Copy link
Author

@Nantris Thank you for letting me know this.
I have tested on prosemirror-view 1.34.1 (older version I used) and chrome 130.0.6723.70 (the latest version now).
The issue appears to be resolved.

I think the issue was resolved in the chrome 129.0.6668.45 build.
https://issues.chromium.org/issues/363266897#comment29

@marijnh
Copy link
Member

marijnh commented Oct 29, 2024

All right, I've reverted ProseMirror/prosemirror-view@42d6b1d in ProseMirror/prosemirror-view@a8d862d, on the assumption that the group of people still impacted by this issue is tiny.

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

No branches or pull requests

4 participants