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

Work around incorrect data on `compositionupdate` events in Chrome 56 #15265

Merged
merged 4 commits into from Aug 12, 2017

Conversation

Projects
None yet
4 participants
@nathansobo
Contributor

nathansobo commented Aug 11, 2017

Fixes #14911

The compositionupdate event is associated with a data property 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 data property. Since compositionupdate events seem to be fired prior to the contents of the input field changing, I perform the read on process.nextTick after the browser has a chance to update the field.

One extra wrinkle is I now clear the input field on compositionstart events so that previous input doesn't pollute the previewed composition. The only keystrokes that could potentially pollute the preview are spaces, since we use preventDefault in our textInput event handler to prevent every other character from actually being inserted. If we preventDefault on 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

nathansobo added some commits Aug 11, 2017

Clear hidden input compositionstart on Chrome 56
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.
@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Aug 11, 2017

Member

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?

Member

Ben3eeE commented Aug 11, 2017

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?

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Aug 11, 2017

Contributor

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

Contributor

ungb commented Aug 11, 2017

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

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Aug 11, 2017

Contributor

Looks like the tests are failing. Will try to get this working tomorrow.

Contributor

nathansobo commented Aug 11, 2017

Looks like the tests are failing. Will try to get this working tomorrow.

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Aug 11, 2017

Contributor

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:
before:
beforeime

After:
ime

There's still a few test failures that need to be address if these open up some other regression.

Contributor

ungb commented Aug 11, 2017

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:
before:
beforeime

After:
ime

There's still a few test failures that need to be address if these open up some other regression.

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Aug 12, 2017

Member

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 process.nextTick could fire after textInput has already been fired or, in other words, when the composition has already ended. That commit adds a guard that ensures composition is still in progress before attempting to display the IME preview.

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.

@Ben3eeE @ungb: could you give this another spin? Thanks. 🙏

Member

as-cii commented Aug 12, 2017

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 process.nextTick could fire after textInput has already been fired or, in other words, when the composition has already ended. That commit adds a guard that ensures composition is still in progress before attempting to display the IME preview.

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.

@Ben3eeE @ungb: could you give this another spin? Thanks. 🙏

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Aug 12, 2017

Member

LGTM 🚢

Member

Ben3eeE commented Aug 12, 2017

LGTM 🚢

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Aug 12, 2017

Contributor

Thanks for pushing this over the line @as-cii ❤️

Contributor

nathansobo commented Aug 12, 2017

Thanks for pushing this over the line @as-cii ❤️

@nathansobo nathansobo merged commit feb0cdd into master Aug 12, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nathansobo nathansobo deleted the ns-ime-workaround branch Aug 12, 2017

nathansobo added a commit that referenced this pull request Aug 12, 2017

Merge pull request #15265 from atom/ns-ime-workaround
Work around incorrect data on `compositionupdate` events in Chrome 56

nathansobo added a commit that referenced this pull request Aug 12, 2017

Merge pull request #15265 from atom/ns-ime-workaround
Work around incorrect data on `compositionupdate` events in Chrome 56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment