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

[IME] Disable iOS COMPAT behaviour on desktop #1236

Merged
merged 1 commit into from Oct 16, 2017

Conversation

3 participants
@doodlewind
Contributor

doodlewind commented Oct 16, 2017

This PR closes a series of desktop IME issues: #1229 #1209 #854 #810 .
It also closes PR #885 #880 .

All issues mentions above is caused by a workaround introduced in #655 , which adds a piece of iOS COMPAT code fixing autocorrect behaviour. In this workaround, onBeforeInput will adjust slate's selection if selection is expanded, resolve the mismatch.

However, during desktop IME composition, the selection is also expanded. This leads to deletion of unrelated text, in other word, 'eating' characters afterwards:

Current Behaviour

before

Previous solution adds composition-state-related data for onBeforeInput, this works but may introduce more potential issues. In fact, by simply limiting the iOS COMPAT code according to environment, this issue can be solved in a much simpler way. Moreover, it also benefits performance by reducing unnecessary DOM ops like window.getSelection() on other platforms.

This patch not only solves Chinese Pinyin IME issues, for other languages it also works:

After: Native Pinyin IME

after-native-pinyin

After: Thiry Party Pinyin IME

after-third-party-pinyin

After: Native Japanese IME

after-native-japanese

After: Native Korean IME

after-native-korean

BTW, another IME related bug can be solved based on this PR with React 16's error boundary, I may submit another PR if this one can get merged 馃挭

@doodlewind doodlewind changed the title from Disable iOS COMPAT behaviour on desktop to [IME] Disable iOS COMPAT behaviour on desktop Oct 16, 2017

@ianstormtaylor ianstormtaylor added the bug label Oct 16, 2017

@ianstormtaylor

This comment has been minimized.

Show comment
Hide comment
@ianstormtaylor

ianstormtaylor Oct 16, 2017

Owner

Thank you @doodlewind, this is awesome. Thanks for the very detailed PR!

Owner

ianstormtaylor commented Oct 16, 2017

Thank you @doodlewind, this is awesome. Thanks for the very detailed PR!

@ianstormtaylor ianstormtaylor merged commit 4fe6ac3 into ianstormtaylor:master Oct 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lxcid

This comment has been minimized.

Show comment
Hide comment
@lxcid

lxcid Oct 16, 2017

Collaborator

Hey @doodlewind! Thanks for the fix! Sorry for introducing the bug!

Collaborator

lxcid commented Oct 16, 2017

Hey @doodlewind! Thanks for the fix! Sorry for introducing the bug!

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