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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always revert to composition checkpoint, even if input is disabled #15833

Merged
merged 1 commit into from Oct 6, 2017

Conversation

Projects
None yet
5 participants
@as-cii
Member

as-cii commented Oct 6, 2017

Fixes #15783

Previously, if the user opened the IME menu while input was disabled, we would create a composition checkpoint without reverting to it after the composition ended. When enabling input again, the first keystroke would cause any buffer change that occurred between the IME composition and the keystroke to be reverted.

With this commit we will always revert and delete the composition checkpoint as soon as the composition ends, regardless of whether the input is enabled or not.

@t9md @ungb @Ben3eeE: since we're touching the IME code path, could you help me test this doesn't introduce any weird regression? Thanks! 馃檱

/cc: @nathansobo

Always revert to composition checkpoint, even if input is disabled
Previously, if the user opened the IME menu while input was disabled, we
would create a composition checkpoint without reverting to it after the
composition ended. When enabling input again, the first keystroke would
cause any buffer change that occurred between the IME composition and
the keystroke to be reverted.

With this commit we will always revert and delete the composition
checkpoint as soon as the composition ends, regardless of whether the
input is enabled or not.
@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Oct 6, 2017

Member

I should also add that a better solution here would have been to completely disable the hidden input element when calling setInputEnabled(false). Unfortunately, however, that interacts in a weird way with the focus and, since we will likely cherry-pick this to stable and beta, I decided to fix the problem by using a more cautious approach.

Member

as-cii commented Oct 6, 2017

I should also add that a better solution here would have been to completely disable the hidden input element when calling setInputEnabled(false). Unfortunately, however, that interacts in a weird way with the focus and, since we will likely cherry-pick this to stable and beta, I decided to fix the problem by using a more cautious approach.

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Oct 6, 2017

Contributor

I did quick check by downloading artifacts for mac from circle-CI.
https://circleci.com/gh/atom/atom/5540#artifacts/containers/0

  • Issue fixed(no cursor jump, no content revert).
  • I don't see any regression.

Thanks for fixing this @as-cii

~/Downloads/Atom.app/Contents/MacOS/Atom -d --version
Atom    : 1.23.0-dev-683cdea
Electron: 1.6.14
Chrome  : 56.0.2924.87
Node    : 7.4.0
Contributor

t9md commented Oct 6, 2017

I did quick check by downloading artifacts for mac from circle-CI.
https://circleci.com/gh/atom/atom/5540#artifacts/containers/0

  • Issue fixed(no cursor jump, no content revert).
  • I don't see any regression.

Thanks for fixing this @as-cii

~/Downloads/Atom.app/Contents/MacOS/Atom -d --version
Atom    : 1.23.0-dev-683cdea
Electron: 1.6.14
Chrome  : 56.0.2924.87
Node    : 7.4.0
@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 6, 2017

Member

I've verified the fix on Windows and found no regressions when testing 馃殺

Member

Ben3eeE commented Oct 6, 2017

I've verified the fix on Windows and found no regressions when testing 馃殺

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Oct 6, 2017

Contributor

@Ben3eeE
Thank you as always.
You always catch update and doing issue reproduction and verifying fixes effortlessly, quickly.
I want to say your effort is super meaningful and important, thank you.
I really respect your contribution to Atom editor as one atom-fan user.

Contributor

t9md commented Oct 6, 2017

@Ben3eeE
Thank you as always.
You always catch update and doing issue reproduction and verifying fixes effortlessly, quickly.
I want to say your effort is super meaningful and important, thank you.
I really respect your contribution to Atom editor as one atom-fan user.

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Oct 6, 2017

Contributor

LGTM! 馃殺

@as-cii Looks like circle is failing though.

Contributor

ungb commented Oct 6, 2017

LGTM! 馃殺

@as-cii Looks like circle is failing though.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 6, 2017

Member

@t9md Thank you so much for the kind words 鉂わ笍 馃檪 I love your issue reports especially because they are always well written with clear and easy steps to reproduce. Keep up the good work 馃挴

Member

Ben3eeE commented Oct 6, 2017

@t9md Thank you so much for the kind words 鉂わ笍 馃檪 I love your issue reports especially because they are always well written with clear and easy steps to reproduce. Keep up the good work 馃挴

@nathansobo nathansobo merged commit 6209c90 into master Oct 6, 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 as-always-clear-composition-checkpoints branch Oct 6, 2017

nathansobo added a commit that referenced this pull request Oct 6, 2017

Merge pull request #15833 from atom/as-always-clear-composition-check鈥
鈥oints

Always revert to composition checkpoint, even if input is disabled

nathansobo added a commit that referenced this pull request Oct 6, 2017

Merge pull request #15833 from atom/as-always-clear-composition-check鈥
鈥oints

Always revert to composition checkpoint, even if input is disabled
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 6, 2017

Contributor

Love to see love on PRs. Thanks everybody!

Contributor

nathansobo commented Oct 6, 2017

Love to see love on PRs. Thanks everybody!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment