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

Handle IME composing state in onBeforeInput #1107

Closed
wants to merge 6 commits into from
Closed

Handle IME composing state in onBeforeInput #1107

wants to merge 6 commits into from

Conversation

doodlewind
Copy link

@doodlewind doodlewind commented Sep 11, 2017

This PR suggests a moderate solution for issue #854 based on PR #885 and #880, which can also be a prerequisite to solve issue #1012.

Desktop IME has native behaviors which is different from mobile. Briefly, there are three types of events related to IME:

  • onCompositionStart
  • onCompositionUpdate
  • onCompositionEnd

On mobile the onCompositionStart and onCompositionUpdate event does not ever fire since user can choose words inside IME UI and insert the result all at once with onCompositionEnd fires. On desktop the default behavior generally works as below:

  1. When user inputs first character with IME, onCompositionStart fires.
  2. When user inputs each character with IME, onCompositionUpdate fires on each key down, but during which onKeyDown does not fire.
  3. When user choose the resulting word to input via space or mouse, onBeforeInput fires.
  4. After onBeforeInput fires, onCompositionEnd fires.

One essential key to IME related issues is that onBeforeInput fires before onCompositionEnd, so it's necessary for onBeforeInput to know whether current input state is composing. In this PR we mimic the onKeyDown way that pass in a isComposing state inside data arg to plugin's onBeforeInput, which enables both core and custom plugins to add extra logic fixing IME state.

As as result, onBeforeInput inside core plugin can have more control: The reason causes #854 is that during desktop IME inputs, the iOS COMPAT code which manually changes selection inside core plugin's onBeforeInput should not have been worked. Here is an example:

  1. If a Chinese IME user begins input with init selection collapsed, after 5 keys haode stroke, IME will interpret the keys as hao'de with 6 letters, which appears on screen underlined (haode means 好(hao)的(de) in Chinese).
  2. When user ends input, onBeforeInput fires.
  3. Before the word 好的 applys to state, the iOS COMPAT code inside onBeforeInput will consider it a mismatch, selecting a new selection with a width of 6 letters after current cursor, then replace them with 好的, but since the temp letters hao'de is natively removed by browser, this is not necessary, resulting 'eating' letters after current cursor.

In this case, with isComposing provided to plugin, simply ignoring iOS COMPAT code on desktop solves this issue.

Hope it helps :-)

@ianstormtaylor
Copy link
Owner

@doodlewind this is an extremely written up PR! Thank you!

I'll be back in a few hours and I can check this out locally and hope to get it merged. Thanks for taking the time to work on this!

@@ -7,7 +7,7 @@
*/

function findDeepestNode(element) {
return element.firstChild
return element && element.firstChild
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@doodlewind do you know where this was causing an issue?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been done with another PR. When element itself is null, IMO returning null is somehow better then throwing an error. This happens when removing nodes in a loose schema. Sorry for not providing reproduction here, while if this is not suitable to be merged here, it's okay to close this with another pull request.😅

@danburzo
Copy link
Contributor

danburzo commented Sep 11, 2017

@doodlewind thank you for the thorough write-up!

I'd looked at the IME problem when I opened #1012 and I understand your solution echoes PR #885, that is go around the problem introduced by the iOS autocomplete fix for composition specifically. I have a feeling that composition may not be the only area affected by that, so merging this patch will potentially obscure other issues.

An alternative approach, which IMO is cleaner, will be to drop that iOS autocomplete fix and use Input Events generated by iOS 10.1+ (as per this comment, it's not a tragedy to not support iOS <=10.0)

@doodlewind
Copy link
Author

@danburzo thanks for your insight!
You are suggesting Input Events on iOS 10.1+ on mobile, this is a great step on mobile and since this PR focus on desktop IME, our solutions can be in coexistence. In fact composition state is not only related in issue #854 , I submitted issue #1114 showing another case needs to be handled with desktop composition events, which is also related to native desktop IME behaviors. To solve this, IMO it also requires core plugin's onBeforeInput to know composition state to handle IME's native behavior. Very welcomed for your suggestion. 🙏

@doodlewind
Copy link
Author

Closing this since my forked branch contains some unrelated changes.

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

Successfully merging this pull request may close these issues.

3 participants