Added support for the `compositionupdate` event by mathieumg · Pull Request #1285 · facebookarchive/draft-js

mathieumg · 2017-07-12T21:17:23Z

Summary

@flarnie As we discussed, this adds support for the compositionupdate event and ignores the beforeinput event if it were to be fired in that scenario too. I imagine a slightly cleaner way of doing this would be to remove the listener on beforeinput altogether when we realize compositionupdate fires. Should I squash my changes?

Test Plan

I didn't add new tests, should I add tests that fail without this fix?

flarnie · 2017-07-12T21:20:26Z

Thanks for making this into a PR!

Should I squash my changes?

Nah, we always 'Squash and merge' when merging, so you can leave the commits separate if that's easier for you.

mathieumg · 2017-07-12T21:21:25Z

we always 'Squash and merge' when merging

I totally forgot GitHub added that as a feature a while ago, all good! 👍

flarnie

Thanks again for making this PR @mathieumg!
@spicyj, @paloobi, myself, and other internal folks were discussing this, and here was our thinking:

we would prefer to default to using 'onBeforeInput' and falling back to 'onCompositionUpdate' rather than defaulting to using 'onCompositionUpdate'. There are inconsistencies in composition events across different browser/OS combinations, in general, and the smallest initial change would be to use 'onCompositionUpdate' as a fallback.
We'd like to wrap the changed behavior in a feature-flag so that we can test internally. I'm happy to add a commit to this PR doing that, or if you want to here is the way it usually works: https://github.com/facebook/draft-js/wiki/Draft-Feature-Flags
(if that doesn't make sense please ask - I just wrote it, and there could be inconsistencies.)
I'm going to do some browser testing in IE/Edge and with different IME input, and see if there are any issues with this approach.

Let me know if you are too busy and if so I can try to update this.

sophiebits · 2017-07-14T23:45:07Z

Also, one other thing: onBeforeInput is not actually the browser's native event. We polyfill it in many cases inside of React here: https://github.com/facebook/react/blob/3b5e3b5a23069dd03feaa6979bccb64de4d76d09/src/renderers/dom/shared/eventPlugins/BeforeInputEventPlugin.js. It might make sense to switch that logic so that onBeforeInput events are properly synthesized based on the composition events that Android provides, and then we don't need to change Draft proper at all.

mathieumg · 2017-07-17T21:39:14Z

@flarnie Thanks for the return! I just got home right now, will try to update the PR ASAP!

flarnie · 2017-07-28T23:36:45Z

Hi again -
I checked this out and ran into a couple of issues. I'd still like to see this fix or a similar fix added, but I'm not sure how to make it work. Or we may go in the direction of adding this to the existing 'onBeforeInput' polyfill in React. I'm not sure.

Issue #1
To make this a smaller change to the current behavior, we suggested defaulting to 'onBeforeInput' instead of 'onCompositionUpdate'. But because 'onCompositionUpdate' fires first, it's going to be more complex to fall back to it. Just seems like it will add complexity and maybe there is a different way.

Issue #2
This doesn't quite make sense with the pattern of events fired for Korean input, and other IME use cases.

I manually tested Korean, and also recorded the input events using https://dvcs.w3.org/hg/d4e/raw-file/tip/key-event-test.html in Chrome on mac. You can see that the data doesn't contain the whole composition, and so it ends up clearing the composition at the end and replacing it with the last character:

This is inconsistent with Japanese I think, where the data on the 'onCompositionUpdate' contains the entire composition so far. Japanese seemed to work ok with this fix, but I still would be concerned about subtle changes there.

I'm attaching pdfs of the keypress events for Korean and Japanese examples, for reference. Basically I think someone (maybe me eventually) needs to catalogue all the event patterns for different IME and composition use cases, and come up with handlers or modes for the different cases.

If you don't need to support Korean or other IME input then this fix may still be good enough for your use case. If you're open to addressing the issues with Korean input I can help look into it, or we can close this pending further investigation.

chrome_mac_katana_keyboard_events.pdf
korean_input_events.pdf

octatone · 2017-08-02T16:29:10Z

@flarnie This PR, from Apr 20, seems to tackle a number of IME/OS composition event combinations in a simplified manner here: #1152

What are your thoughts on this solution?

mathieumg · 2017-08-02T16:37:02Z

@octatone I'm pretty sure I gave a shot to that PR first, but it also ignores the compositionupdate event, which is the only thing working on some browsers/devices.

@flarnie I've been meaning to get to this but have been rather busy as of late, will try my best to find some time soon! 😅

This continues from facebookarchive#1152 and facebookarchive#1285 making them work together for broad support for polatform inconsistencies.

octatone · 2017-08-09T09:00:49Z

@mathieumg @njjewers @flarnie

I merged and resolved conflicts with the work done so far by both @njjewers and @mathieumg which hopefully fixes a bunch of issues with IME and Android keyboard weirdness.

You can see my result here: master...octatone:raymond/input-bugs

I manually tested Korean, Japanese and Chinese (simplified) with these changes, and added unit tests to ensure that further improvements don't break expect normal IME cases. My unit tests are based on captured IME events on Chrome + Windows.

Please review and let me know if combining these two bodies of work into one is okay, and I will submit a proper PR with feature flags, etc.

flarnie · 2017-08-09T15:06:43Z

@octatone - this looks interesting, thanks for working on it!

I'm working on some React stuff at the moment but will review your result as soon as I can.

tomkelsey · 2017-08-18T22:25:52Z

@octatone Thanks to you (and everyone else) for your work on this. It's been driving our Android users a bit nutty!

I've just tested out your changes with my own Android device and it looks like it clears things up with GBoard but still odd behaviour with SwiftKey - typing any word causes the keyboard to immediately lose focus. But, either way, significantly better than it was.

sophiebits

(switching away from github review labels; requesting changes to match earlier review above)

matthewshirley · 2017-09-22T21:13:14Z

@tomkelsey Same problem with SwiftKey. If I enter @ or $ before typing it works as intended though. Same for you?

octatone · 2017-10-04T10:25:45Z

I have opened a PR that continues from this work #1419

This continues from facebookarchive#1152 and facebookarchive#1285 making them work together for broad support for polatform inconsistencies.

* Modified compositoin mode event handlers to provide better support for mobile browsers * Added test cases for DraftEditorCompositionHandler * Linter compliance * Added support for `compositionupdate` event. * Made the linter happy! * Normalize beforeInput and compositionUpdate data This continues from facebookarchive#1152 and facebookarchive#1285 making them work together for broad support for polatform inconsistencies. * Put new composition fixes behind a feature flag * lint * correct resets * Fix Synthetic Event flow types * Convert DraftEditorCompositionHandler tests to root tests + snapshots * Remove unnecessary DraftFeatureFlags

…r mobile browsers Added test cases for DraftEditorCompositionHandler Linter compliance Added support for `compositionupdate` event. Made the linter happy! Normalize beforeInput and compositionUpdate data This continues from facebookarchive#1152 and facebookarchive#1285 making them work together for broad support for polatform inconsistencies. Put new composition fixes behind a feature flag lint correct resets Fix Synthetic Event flow types Convert DraftEditorCompositionHandler tests to root tests + snapshots

mathieumg added 2 commits July 12, 2017 14:07

Added support for compositionupdate event.

27f11df

Made the linter happy!

7c05c86

facebook-github-bot added GH Review: review-needed CLA Signed labels Jul 12, 2017

flarnie suggested changes Jul 14, 2017

View reviewed changes

flarnie added GH Review: needs-revision and removed GH Review: review-needed labels Jul 28, 2017

octatone added a commit to octatone/draft-js that referenced this pull request Aug 9, 2017

Normalize beforeInput and compositionUpdate data

9b804b7

This continues from facebookarchive#1152 and facebookarchive#1285 making them work together for broad support for polatform inconsistencies.

laukaichung mentioned this pull request Aug 21, 2017

Question : Can I use "draft js" in "react native"? #1343

Closed

sophiebits suggested changes Aug 24, 2017

View reviewed changes

sophiebits removed the GH Review: needs-revision label Aug 24, 2017

octatone mentioned this pull request Oct 4, 2017

Normalize input data application from onBeforeInput and compositionUpdate #1419

Open

octatone added a commit to octatone/draft-js that referenced this pull request Nov 21, 2017

Normalize beforeInput and compositionUpdate data

fb6cc05

This continues from facebookarchive#1152 and facebookarchive#1285 making them work together for broad support for polatform inconsistencies.

lan9 mentioned this pull request Feb 28, 2018

There's input bug in Android keyboard #1224

Closed

ericls mentioned this pull request Apr 17, 2018

Input bugs (#2) ericls/draft-js#1

Merged

Conversation

mathieumg commented Jul 12, 2017

Uh oh!

flarnie commented Jul 12, 2017

Uh oh!

mathieumg commented Jul 12, 2017

Uh oh!

flarnie left a comment

Choose a reason for hiding this comment

Uh oh!

sophiebits commented Jul 14, 2017

Uh oh!

mathieumg commented Jul 17, 2017

Uh oh!

flarnie commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

octatone commented Aug 2, 2017

Uh oh!

mathieumg commented Aug 2, 2017

Uh oh!

octatone commented Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flarnie commented Aug 9, 2017

Uh oh!

tomkelsey commented Aug 18, 2017

Uh oh!

sophiebits left a comment

Choose a reason for hiding this comment

Uh oh!

matthewshirley commented Sep 22, 2017

Uh oh!

octatone commented Oct 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

flarnie commented Jul 28, 2017 •

edited

Loading

octatone commented Aug 9, 2017 •

edited

Loading