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

Incorrect data in compositionend event when typing Korean on IE11 #10217

Closed
robbertbrak opened this issue Jul 19, 2017 · 18 comments
Closed

Incorrect data in compositionend event when typing Korean on IE11 #10217

robbertbrak opened this issue Jul 19, 2017 · 18 comments

Comments

@robbertbrak
Copy link

To reproduce:

  • In IE11 (on Win7 or Win10) go to https://jsfiddle.net/robbertbrak/84v837e9/164/
  • Open the Developer console.
  • Switch to the Microsoft Korean IME (standard settings) and switch to Korean input.
  • Put the cursor in the contenteditable div and type 여름. (on a QWERTY keyboard this is typed as dufma.).
  • Do the same for the input field.
    Result: in the developer console a list of composition events and the contents of the data attribute is logged. However, the data of the first compositionend event is wrong. It should be 여, not 여르. See screenshot below.

selection_329

You can see that this is wrong by trying out the same thing on https://dvcs.w3.org/hg/d4e/raw-file/tip/key-event-test.html. As seen in the screenshot below, a plain (non-React) input field emits a compositionend event with the proper data.

korean-11 413 15063 0

This occurs with the latest version of React, but I have also seen this behaviour in older versions. An example of where this causes problems is in Draft JS.

@gaearon
Copy link
Collaborator

gaearon commented Jul 19, 2017

Does this reproduce with react@next and react-dom@next? The UMD locations have changed there, it would be /umd/react.development.js and /umd/react-dom.development.js.

@aweary
Copy link
Contributor

aweary commented Jul 19, 2017

@robbertbrak you can use this JSFiddle which loads react@next and react-dom@next as a starting point. I can try to reproduce this later as well.

Also, that W3 key event test site is great, thanks for sharing that!

@gaearon
Copy link
Collaborator

gaearon commented Jul 19, 2017

cc @flarnie @spicyj who also might have context on this

@robbertbrak
Copy link
Author

@gaearon Yes, this is still reproducible in react@next. With this JSFiddle I get the same issue: https://jsfiddle.net/robbertbrak/68rwz1mr/2/ (sorry for the horrible code in that Fiddle, but it seems that JSFiddle with Babel+JSX and IE11 don't work well together...)

@aweary Glad to help out. Actually I got that W3 site from @hellendag, who referred to it in facebookarchive/draft-js#311 :)

@aweary
Copy link
Contributor

aweary commented Jul 19, 2017

It's a great resource! We have a handful of open issues related to composition events. It's something we need to look into more, and a good candidate for our DOM fixtures, so thanks for bringing this to our attention!

@flarnie
Copy link
Contributor

flarnie commented Jul 19, 2017

Additional thanks - this will be helpful to the Draft.js project too. We also have open issues there related to composition, and the Korean IME in particular, and it could be that they were hard to track down because the issue is actually in React.

@gaearon
Copy link
Collaborator

gaearon commented Jul 19, 2017

It would be great if there was some way to simulate those in DOM fixtures so that we don’t have to type with special layouts.

@sophiebits
Copy link
Collaborator

There isn't a good way to simulate this without sidestepping the exact thing we want to test in the first place.

cc @msmania who might be interested here.

@nhunzaker
Copy link
Contributor

nhunzaker commented Jul 19, 2017

Could we use the Language and Keyboard settings on OSX to simulate this? What input settings should we use?

screen shot 2017-07-19 at 5 20 22 pm

http://osxdaily.com/2013/06/21/mac-virtual-keyboard-os-x/

@sophiebits
Copy link
Collaborator

Each browser provides rather different events depending on the browser/OS/keyboard combo. The only way to test this properly is to get a real copy of IE11 and set your keyboard input to the proper language. (For example, in the past I have broken Korean input on Chrome Windows while trying to fix Korean input on Chrome Mac. You would think they're the same but they are not.)

@robbertbrak
Copy link
Author

Yes, and it gets worse. Apparently not even all versions of IE11 behave the same. For example, I once made this screenshot of IE11:

selection_116

You can see that IE11 emitted the wrong Korean character in the compositionend event. Luckily, it seems that Microsoft fixed this in later versions of IE11.

Similarly, there used to be a bug that IE11 moved the cursor position to the right, just before the compositionstart event when typing Korean. This has recently been fixed as well.

All this means that it is very hard for Draft.js to do the right thing, because it relies on the correct Javascript events being fired. Workarounds that are appropriate for one browser/OS/IME may not work for other combinations, or suddenly stop working.

@msmania
Copy link
Contributor

msmania commented Jul 21, 2017

Thanks @spicyj for looping me in this discussion. I think this is one of the know issues. Let me see what we can do..

@msmania
Copy link
Contributor

msmania commented Jul 27, 2017

I also confirmed the data property in the composition end was wrong in IE11/Win8.1 and IE11/Win7.
This issue has been fixed in all Win10 family as mentioned in the above comment. This is IE11's bug, but does this particular issue break the behavior of React or Draft.js?

The actual issue I know is that when we type 'dufma.' on the Draft editor, the text "여르름." is committed, while the expected text is "여름.". This issue still happens in IE11/Win10. The root cause of this issue is that the fallback logic in React does not go well with Korean IME.

What the fallback logic does is:

  1. Pick up the element's text at compositionstart and cache it as FallbackCompositionState._startText
  2. Pick up the element's text at compositionend and do diff'ing it with the cached value
  3. Build the beforeinput event with a diff'ed character

In the scenario of typing 'dufma.' with Korean IME, the text at compositionstart is 'ㅇ', the text at compositionend is '여르', and thus the diff'ed result is '여르'. As a result, the beforeinput event is fired with the text '여르' though the second character '르' is not committed.

In IE11, React does not use the data property in composition events probably because React does not trust it.

@robbertbrak
Copy link
Author

In IE11, React does not use the data property in composition events probably because React does not trust it.

@msmania Is it known why React does not trust it? E.g., for which IME's is the data property incorrect in IE11?

@msmania
Copy link
Contributor

msmania commented Aug 2, 2017

@robbertbrak Yes, it's mentioned in BeforeInputEventPlugin.js. It seems that Japanese character \u3000 is one case, but I'm not sure about any other cases.

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/eventPlugins/BeforeInputEventPlugin.js#L42-L48

// In IE9+, we have access to composition events, but the data supplied
// by the native compositionend event may be incorrect. Japanese ideographic
// spaces, for instance (\u3000) are not recorded correctly.
var useFallbackCompositionData =
  ExecutionEnvironment.canUseDOM &&
  (!canUseCompositionEvent ||
    (documentMode && documentMode > 8 && documentMode <= 11));

@danburzo
Copy link

danburzo commented Aug 20, 2017

I've been recently investigating IME problems in the Slate editor — which uses React to manage the contenteditable and listen for events. Just wanted to share that I've created a riff on the Keyboard Events Viewer that shows native UI events and the ones emitted by React: https://danburzo.github.io/input-methods/ — I found it helps me a lot in debugging IMEs. (I haven't tested it on IE yet, unfortunately, so YMMV)

I also plan to include a comprehensive writeup of IME scenarios (taking a cue from this excellent page by the VSCode team), to have an input methods test suite of sorts.

(Sorry for inviting myself into this issue :P)

@robbertbrak
Copy link
Author

@danburzo Those pointers are fantastic! The UI events viewer seems to work pretty well in IE. So thanks for inviting yourself in :)

@flarnie flarnie self-assigned this Sep 24, 2017
crux153 added a commit to crux153/react that referenced this issue Apr 6, 2018
…ebook#10217)

* Add isUsingKoreanIME function to check if a composition event was triggered by Korean IME

* Add Korean IME check alongside useFallbackCompositionData and disable fallback mode with Korean IME
crux153 added a commit to crux153/react that referenced this issue Apr 6, 2018
…acebook#10217)

* Add isUsingKoreanIME function to check if a composition event was triggered by Korean IME

* Add Korean IME check alongside useFallbackCompositionData and disable fallback mode with Korean IME
gaearon pushed a commit that referenced this issue Jun 14, 2018
…10217) (#12563)

* Add isUsingKoreanIME function to check if a composition event was triggered by Korean IME

* Add Korean IME check alongside useFallbackCompositionData and disable fallback mode with Korean IME
@gaearon
Copy link
Collaborator

gaearon commented Jun 14, 2018

Merged #12563 into master which should fix this. Will be out in the next release.

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

No branches or pull requests

8 participants