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.

Sign up for GitHub

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

Jump to bottom

Handle composition events in ChangeEventPlugin #8438

Closed
wants to merge 2 commits into from
Closed

Handle composition events in ChangeEventPlugin #8438

wants to merge 2 commits into from

Conversation

yesmeck
Copy link

@yesmeck yesmeck commented Nov 28, 2016 •
edited
Loading

This PR attempts to fix #3926.

@yesmeck yesmeck force-pushed the composition-events branch 2 times, most recently from dca73a4 to f107975 Compare November 28, 2016 13:31
@yesmeck
Copy link
Author

yesmeck commented Nov 28, 2016

How can I make CI green?

@gaearon
Copy link
Collaborator

gaearon commented Nov 28, 2016

Please run ./scripts/fiber/record-tests and add that to the commit.

@yesmeck yesmeck force-pushed the composition-events branch 4 times, most recently from b0b7161 to 0d7e605 Compare November 29, 2016 09:05
@yesmeck
Copy link
Author

yesmeck commented Nov 29, 2016

Added.

@yesmeck yesmeck force-pushed the composition-events branch 2 times, most recently from 8a2fc89 to 0482af7 Compare December 3, 2016 11:31
jquense added a commit to jquense/react that referenced this pull request Dec 14, 2016
@jquense
Allow simulated native events to propagate
ff73f69 
fixes facebook#7211 fixes facebook#6822 fixes facebook#6614

we should make sure it doesn't break facebook#3926 any worse (or works with facebook#8438)
This was referenced Dec 14, 2016
Closed
Merged
@yesmeck yesmeck mentioned this pull request Jan 5, 2017
Open
@eyesofkids eyesofkids mentioned this pull request Jan 7, 2017
Closed
@yesmeck yesmeck force-pushed the composition-events branch 3 times, most recently from 4990965 to 383efe9 Compare January 8, 2017 07:34
@yesmeck yesmeck force-pushed the composition-events branch 3 times, most recently from 9897066 to 5178e0e Compare April 2, 2017 08:43
@paranoidjk
Copy link

Any update？ can we get this merged?

@paranoidjk
Copy link

@yesmeck There may be a problem.

On mobile device, such as iphone, chinese IME, we have something call Word prediction
from https://en.wikipedia.org/wiki/Pinyin_input_method

Word prediction (simplified Chinese: 联想; traditional Chinese: 聯想; pinyin: liánxiǎng; literally: "association") is a feature of an input method that attempts to guess the next series of characters that the user is attempting to enter. This feature is often used to refer to two different mechanisms that have similar functions.

so the events fire sequence will be：
compositionstart --> compositionupdate --> onChange --> compositionstart --> compositionupdate...

There will be no compositionend until the Word prediction is ended.

@johnnyshields
Copy link

Please merge this is affecting us as well.

flarnie pushed a commit that referenced this pull request May 20, 2017
@jquense @flarnie
Backport input fix (#8575)
68347c9 
* Only fire input value change events when the value changes (#5746)

* Allow simulated native events to propagate

fixes #7211 fixes #6822 fixes #6614

we should make sure it doesn't break #3926 any worse (or works with #8438)
flarnie pushed a commit to flarnie/react that referenced this pull request Jun 7, 2017
@jquense @flarnie
Backport input fix (facebook#8575)
f82d323 
* Only fire input value change events when the value changes (facebook#5746)

* Allow simulated native events to propagate

fixes facebook#7211 fixes facebook#6822 fixes facebook#6614

we should make sure it doesn't break facebook#3926 any worse (or works with facebook#8438)
@gaearon gaearon mentioned this pull request Oct 4, 2017
Open
29 tasks
@gaearon
Copy link
Collaborator

gaearon commented Jan 5, 2018

Thanks for the PR. It's not obvious to me whether this works or not.

I think we could take a fix like this if you can demonstrate you've done exhaustive testing of this. It's a pretty risky change though so I don't feel comfortable merging it.

Let's continue the discussion in #3926. If somebody can take ownership of this and demonstrate they've tested a wide range of cases, and also explain in detail why this solution works, maybe we can get it in.

@gaearon gaearon closed this Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change event fires extra times before IME composition ends
6 participants
@yesmeck @gaearon @paranoidjk @johnnyshields @sebmarkbage @facebook-github-bot