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

Fix incorrect data in compositionend event with Korean IME on IE11 #12563

Merged
merged 2 commits into from
Jun 14, 2018

Conversation

crux153
Copy link
Contributor

@crux153 crux153 commented Apr 6, 2018

This fixes wrong behavior with composition events when using Korean IME on IE11. (#10217)

It seems that it is fine to use native compositionend event from Korean IME, so this simply disable using fallback composition mode when using Korean IME.

Check for Korean IME is implemented using CompositionEvent.locale property. Although it is deprecated according to MDN, it is still available in IE (where fallback composition mode gets enabled) and works well with detecting Korean IME.

I checked the functionality of this fix manually on IE9, 10, 11, though it seems that fallback composition mode only causes problem in IE11. I think it should have no impact on environments rather than IE with Korean IME.

It also fixes other rich text editors depending on React, such as Draft.js.


Before
Before

After
After

…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
@crux153 crux153 changed the title Fix incorrect data in composition events with Korean IME on IE Fix incorrect data in compositionend event with Korean IME on IE11 Apr 6, 2018
Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

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

This is really clean and easy to understand. However I feel totally unqualified to approve it. I'll ping other members of the team.

@nhunzaker
Copy link
Contributor

Are there any other languages where this is applicable?

@crux153
Copy link
Contributor Author

crux153 commented Jun 14, 2018

Thank you for your response. I would love to see this PR or alternative fix to the issue gets merged soon, as it is quite big problem to Korean users. Due to the strange web environments of Korea, with weird dependency to old, unfashioned ActiveX, IE's market share still exceeds 35% of total desktop traffic here. (http://gs.statcounter.com/browser-market-share/desktop/south-korea)

As this issue renders rich text input of React completely unusable with Korean, it forces developers to either abandon IE11 users or throw up React totally, or just stick to plain text input. Not surprisingly, this seems to be a long issue with Facebook itself.

I think this issue happens because composition events of Korean characters may affect the following character and React's fallback mode is not able to handle it due to its implementation. Another approach to fix this issue might be a rewrite of FallbackCompositionState itself, but that would ends up with bunch of unnecessary boilerplates to other language inputs in my opinion.

I thought about keeping blacklisted locales in an array, instead of checking only Korean IME, but I was not able to find any other languages with this kind of problem. I've applied a custom build of React with this fix on website I manage (with 600K monthly pageviews and 10K visitors) at the time of opening this PR, and there have been no issue reported so far.

@@ -229,7 +243,7 @@ function extractCompositionEvent(
return null;
}

if (useFallbackCompositionData) {
if (useFallbackCompositionData && !isUsingKoreanIME(nativeEvent)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason you don't make this check a part of useFallbackCompositionData declaration itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since my implementation of isUsingKoreanIME depends on CompositionEvent.locale property from composition event, making it part of useFallbackCompositionData requires it to be a function that needs to be invoked every time a composition event occurs.

Current declaration of useFallbackCompositionData happens at load time as it only checks for browser. Check for Korean IME only needs to be invoked when using fallback mode, so I thought additional function invocation would be inefficient for those with modern browsers, though it will be certainly better for code readability.

@pull-bot
Copy link

ReactDOM: size: 0.0%, gzip: 🔺+0.1%

Details of bundled changes.

Comparing: bc963f3...49a9d71

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 626.48 KB 626.99 KB 145.95 KB 146.1 KB UMD_DEV
react-dom.production.min.js 0.0% 🔺+0.1% 95.27 KB 95.3 KB 30.79 KB 30.81 KB UMD_PROD
react-dom.development.js +0.1% +0.1% 610.85 KB 611.36 KB 141.95 KB 142.09 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 93.77 KB 93.8 KB 29.77 KB 29.78 KB NODE_PROD
ReactDOM-dev.js +0.1% +0.1% 620.45 KB 620.98 KB 141.33 KB 141.48 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 0.0% 269.45 KB 269.54 KB 50.53 KB 50.54 KB FB_WWW_PROD
react-dom.profiling.min.js 0.0% 0.0% 94.67 KB 94.71 KB 30.09 KB 30.1 KB NODE_PROFILING

Generated by 🚫 dangerJS

@gaearon gaearon merged commit 2e75779 into facebook:master Jun 14, 2018
@gaearon
Copy link
Collaborator

gaearon commented Jun 14, 2018

The fix is well-contained and I don't see harm in merging this. Let's see if we'll get any other reports.
Thank you!

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.

5 participants