Work around incorrect data on compositionupdate events in Chrome 56
#15265
Conversation
We use the value of the hidden input to display a preview of the composition, but it might already contain spaces from previous keystrokes, since we don't call preventDefault when spaces are inserted.
ed66b03 to
54a6f0d
Compare
|
I tested this using Japanese IME on Windows 10 and following the instructions on https://msdn.microsoft.com/en-us/library/windows/desktop/ee418266(v=vs.85).aspx this seems to fix the issue. I have never used IME before and don't understand japanese but it looks correct 🙂 /cc: @50Wliu @ungb Can you test this with other languages? |
|
I'm testing now. Mainly following repros in #14911. I was able to see the issue where only one character comes up one by one. I'll also test japanese as well, but it seems to impact all languages with IME |
|
Looks like the tests are failing. Will try to get this working tomorrow. < 8000 /td> |
|
Tested this on mac, was able to see it not working on 1.19 and fixed with this PR. I tested Japanese and Chinese(simplified) see gif: After: There's still a few test failures that need to be address if these open up some other regression. |
|
In ca183dd I have addressed a small wrinkle introduced by this that was causing Atom to insert an extra entry in the undo stack. Namely, the insertion of the preview on b4f029e takes care of making the test suite green again, adding also coverage for the IME behavior observed on Chrome 56 and on other Chrome versions where the bug does not exist. Those specs test only the accented character menu behavior, but since that feature works via composition events (i.e. the ones used for IME input) I think they provide a good safety net for possible regressions. We can also add a specific test for CJK input in case we think it would be valuable. |
b0d6783 to
b4f029e
Compare
|
LGTM 🚢 |
|
Thanks for pushing this over the line @as-cii ❤️ |
Work around incorrect data on `compositionupdate` events in Chrome 56
Work around incorrect data on `compositionupdate` events in Chrome 56
Fixes #14911
The
compositionupdateevent is associated with adataproperty that is supposed to contain the full in-progress state of the composed string. Due to a bug in Chrome that has been resolved as of Electron 1.7, that field only contains the most recently typed character in Electron 1.6.This PR attempts to work around this on Chrome 56 by reading the contents of the hidden input field directly rather than relying on the incorrect
dataproperty. Sincecompositionupdateevents seem to be fired prior to the contents of the input field changing, I perform the read onprocess.nextTickafter the browser has a chance to update the field.One extra wrinkle is I now clear the input field on
compositionstartevents so that previous input doesn't pollute the previewed composition. The only keystrokes that could potentially pollute the preview are spaces, since we usepreventDefaultin ourtextInputevent handler to prevent every other character from actually being inserted. If wepreventDefaulton space however, it causes the browser's default behavior of scrolling on space, so we have to let that one through. It's okay though, because we're clearing it out when the composition starts./cc @ungb @Ben3eeE