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

Change event fires extra times before IME composition ends #3926

Open
chenxsan opened this Issue May 21, 2015 · 34 comments

Comments

Projects
None yet
@chenxsan

chenxsan commented May 21, 2015

Extra details

  • Similar discussion with extra details and reproducing analysis: #8683
  • Previous attempt to fix it: #8438 (includes some unit tests, but sufficient to be confident in the fix)

Original Issue

When I was trying this example from https://facebook.github.io/react/blog/2013/11/05/thinking-in-react.html, any Chinese characters inputted by Chinese pinyin input method would fire too many renders like:

screen shot 2015-05-21 at 14 04 36

Actually I would expect those not to fire before I confirm the Chinese character.

Then I tried another kind of input method - wubi input method, I got this:

screen shot 2015-05-21 at 14 17 15

It's weird too. So I did a test in jQuery:

screen shot 2015-05-21 at 14 05 12

Only after I press the space bar to confirm the character, the keyup event would fire.

I know it might be different between the implementation of jQuery keyup and react onChange , but I would expect the way how jQuery keyup handles Chinese characters instead of react's onChange.

@chenxsan chenxsan changed the title from Change event fires too much when inputing Chinese characters. to Change event fires too many times when inputing Chinese characters. May 21, 2015

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 21, 2015

Member

cc @salier :) – What should we do here?

Member

sophiebits commented May 21, 2015

cc @salier :) – What should we do here?

@hellendag

This comment has been minimized.

Show comment
Hide comment
@hellendag

hellendag May 21, 2015

Contributor

I think we should not fire onChange until the IME string is committed.

One way to handle this in ChangeEventPlugin would be to ignore all input events between compositionstart and compositionend, then use the input event immediately following compositionend.

I did some quick testing on OSX Chrome and Firefox with Simplified Pinyin and 2-Set Korean, and the event order and data seem correct enough. (I predict that we'll have problems with IE Korean, but we may get lucky.)

I think we may continue to see issues with alternative input methods like the Google Input Tools extension, but there may be workarounds for that.

Contributor

hellendag commented May 21, 2015

I think we should not fire onChange until the IME string is committed.

One way to handle this in ChangeEventPlugin would be to ignore all input events between compositionstart and compositionend, then use the input event immediately following compositionend.

I did some quick testing on OSX Chrome and Firefox with Simplified Pinyin and 2-Set Korean, and the event order and data seem correct enough. (I predict that we'll have problems with IE Korean, but we may get lucky.)

I think we may continue to see issues with alternative input methods like the Google Input Tools extension, but there may be workarounds for that.

@Bertg

This comment has been minimized.

Show comment
Hide comment
@Bertg

Bertg Sep 15, 2015

This also influences how dialectic characters are typed for latin languages. For example on OS X inserting an é will fail using an international keyboard. Even the long press e and then use variant are failing here.

Sorry this seems to not be related. My apologies.

Bertg commented Sep 15, 2015

This also influences how dialectic characters are typed for latin languages. For example on OS X inserting an é will fail using an international keyboard. Even the long press e and then use variant are failing here.

Sorry this seems to not be related. My apologies.

@jasonslyvia

This comment has been minimized.

Show comment
Hide comment
@jasonslyvia

jasonslyvia Sep 16, 2015

Contributor

Is there any update? Suffering from this issue too.

Contributor

jasonslyvia commented Sep 16, 2015

Is there any update? Suffering from this issue too.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Sep 23, 2015

Member

None currently – this is not a high priority for us right now. I'd be happy to look at a pull request if anyone dives into fixing this.

Member

sophiebits commented Sep 23, 2015

None currently – this is not a high priority for us right now. I'd be happy to look at a pull request if anyone dives into fixing this.

@pswai

This comment has been minimized.

Show comment
Hide comment
@pswai

pswai Oct 2, 2015

@salier It seems like IE does not fire input event after compositionend. I have tested on IE11 and Edge on Windows 10. It fires properly in Chrome and Firefox.

pswai commented Oct 2, 2015

@salier It seems like IE does not fire input event after compositionend. I have tested on IE11 and Edge on Windows 10. It fires properly in Chrome and Firefox.

lingceng added a commit to lingceng/ajax-chosen that referenced this issue Apr 28, 2016

Fix problem when input with Chinese input method
Similar problem with:
Change event fires too many times when inputing Chinese characters
facebook/react#3926

lingceng added a commit to lingceng/ajax-chosen that referenced this issue Apr 29, 2016

Fix problem when input with Chinese input method
Similar problem with:
Change event fires too many times when inputing Chinese characters
facebook/react#3926
@suhaotian

This comment has been minimized.

Show comment
Hide comment
@suhaotian

suhaotian May 31, 2016

in ie 9, Change event fires too many times when inputing Chinese characters again

suhaotian commented May 31, 2016

in ie 9, Change event fires too many times when inputing Chinese characters again

@zhaoyao91

This comment has been minimized.

Show comment
Hide comment
@zhaoyao91

zhaoyao91 Aug 5, 2016

Hello, Facebook guys, in fact this issue causes a SERIOUS problem: we can't update input asynchronously with Chinese input.
For example, we can't use meteor reactive datasources or stores like redux, because they all feedback update asynchronously.
Here is a simplest example to show this problem, it use setTimeout to do async update:
https://jsfiddle.net/liyatang/bq6oss6z/1/

I really hope you can fix this quickly, so that we won't waste efforts to workaround it here and there and over and over again.

Thanks.

Here is my workaround. If anyone face the same problem, you can have a look

zhaoyao91 commented Aug 5, 2016

Hello, Facebook guys, in fact this issue causes a SERIOUS problem: we can't update input asynchronously with Chinese input.
For example, we can't use meteor reactive datasources or stores like redux, because they all feedback update asynchronously.
Here is a simplest example to show this problem, it use setTimeout to do async update:
https://jsfiddle.net/liyatang/bq6oss6z/1/

I really hope you can fix this quickly, so that we won't waste efforts to workaround it here and there and over and over again.

Thanks.

Here is my workaround. If anyone face the same problem, you can have a look

@eyesofkids

This comment has been minimized.

Show comment
Hide comment
@eyesofkids

eyesofkids Aug 16, 2016

I made a simple example to demo how to use compositionstart and compositionend events to prevent inputing Chinese IME on onchange event problem.
Here is the link: https://jsfiddle.net/eyesofkids/dcxvas28/8/

eyesofkids commented Aug 16, 2016

I made a simple example to demo how to use compositionstart and compositionend events to prevent inputing Chinese IME on onchange event problem.
Here is the link: https://jsfiddle.net/eyesofkids/dcxvas28/8/

@zhaoyao91

This comment has been minimized.

Show comment
Hide comment
@zhaoyao91

zhaoyao91 Aug 17, 2016

@eyesofkids nice work, this could be made as the default implementation of onChange for input, textarea ...

zhaoyao91 commented Aug 17, 2016

@eyesofkids nice work, this could be made as the default implementation of onChange for input, textarea ...

@suhaotian

This comment has been minimized.

Show comment
Hide comment
@suhaotian

suhaotian Aug 18, 2016

nice work !

suhaotian commented Aug 18, 2016

nice work !

@julen

This comment has been minimized.

Show comment
Hide comment
@julen

julen Aug 21, 2016

Contributor

I was hitting the same issue and @eyesofkids' workaround works perfectly (thank you!).

After having the workaround in place, I was diving into React's source code to at least try to add a failing test for this —hoping to later add the expected behavior to the library— although it seems a bit complicated for someone unfamiliar with the internals.

Initially I was expecting that a test similar to what's already available for ChangeEventPlugin should work, i.e. simulating a native compositionStart/compositionUpdate and checking no onChange callback was fired; also checking onChange would only be fired once compositionEnd was simulated. However this doesn't seem to work.

Hence I was thinking that maybe checking for ChangeEventPlugin.extractEvents() would be a feasible approach, similar to what's done in the tests for SelectEventPlugin. Here for some reason I always get undefined when extracting the events though.
For reference, this is the test code I tried within ChangeEventPlugin-test.js:

  var EventConstants = require('EventConstants');
  var ReactDOMComponentTree = require('ReactDOMComponentTree');
  var topLevelTypes = EventConstants.topLevelTypes;

  function extract(node, topLevelEvent) {
    return ChangeEventPlugin.extractEvents(
      topLevelEvent,
      ReactDOMComponentTree.getInstanceFromNode(node),
      {target: node},
      node
    );
  }

  function cb(e) {
    expect(e.type).toBe('change');
  }
  var input = ReactTestUtils.renderIntoDocument(
    <input onChange={cb} value='foo' />
  );

  ReactTestUtils.SimulateNative.compositionStart(input);

  var change = extract(input, topLevelTypes.topChange);
  expect(change).toBe(null);

I'm afraid I don't know exactly how one is supposed to debug these tests—otherwise I'd have a clearer picture of what's going on. Any guidance on how to proceed or any other pointers would be highly appreciated.

Contributor

julen commented Aug 21, 2016

I was hitting the same issue and @eyesofkids' workaround works perfectly (thank you!).

After having the workaround in place, I was diving into React's source code to at least try to add a failing test for this —hoping to later add the expected behavior to the library— although it seems a bit complicated for someone unfamiliar with the internals.

Initially I was expecting that a test similar to what's already available for ChangeEventPlugin should work, i.e. simulating a native compositionStart/compositionUpdate and checking no onChange callback was fired; also checking onChange would only be fired once compositionEnd was simulated. However this doesn't seem to work.

Hence I was thinking that maybe checking for ChangeEventPlugin.extractEvents() would be a feasible approach, similar to what's done in the tests for SelectEventPlugin. Here for some reason I always get undefined when extracting the events though.
For reference, this is the test code I tried within ChangeEventPlugin-test.js:

  var EventConstants = require('EventConstants');
  var ReactDOMComponentTree = require('ReactDOMComponentTree');
  var topLevelTypes = EventConstants.topLevelTypes;

  function extract(node, topLevelEvent) {
    return ChangeEventPlugin.extractEvents(
      topLevelEvent,
      ReactDOMComponentTree.getInstanceFromNode(node),
      {target: node},
      node
    );
  }

  function cb(e) {
    expect(e.type).toBe('change');
  }
  var input = ReactTestUtils.renderIntoDocument(
    <input onChange={cb} value='foo' />
  );

  ReactTestUtils.SimulateNative.compositionStart(input);

  var change = extract(input, topLevelTypes.topChange);
  expect(change).toBe(null);

I'm afraid I don't know exactly how one is supposed to debug these tests—otherwise I'd have a clearer picture of what's going on. Any guidance on how to proceed or any other pointers would be highly appreciated.

@julen

This comment has been minimized.

Show comment
Hide comment
@julen

julen Sep 13, 2016

Contributor

The workaround suddenly broke in Chrome 53+ and it seems it is not valid anymore because they changed the order compositionend is fired: previously it happened before textInput, now after textInput. As a consequence of this, change won't be fired if it is aborted while in composition 😕.

Contributor

julen commented Sep 13, 2016

The workaround suddenly broke in Chrome 53+ and it seems it is not valid anymore because they changed the order compositionend is fired: previously it happened before textInput, now after textInput. As a consequence of this, change won't be fired if it is aborted while in composition 😕.

@suhaotian

This comment has been minimized.

Show comment
Hide comment
@suhaotian

suhaotian commented Sep 13, 2016

@eyesofkids

This comment has been minimized.

Show comment
Hide comment
@eyesofkids

eyesofkids Oct 5, 2016

There is a tricky solution for Chrome v53. To call the handlechange after compositionend is fired.

handleComposition  = (event) => {

    if(event.type === 'compositionend'){
      onComposition = false

      //fire change method to update for Chrome v53
      this.handleChange(event)

    } else{
      onComposition = true
    }
  }

check the demo here: https://jsfiddle.net/eyesofkids/dcxvas28/11/

eyesofkids commented Oct 5, 2016

There is a tricky solution for Chrome v53. To call the handlechange after compositionend is fired.

handleComposition  = (event) => {

    if(event.type === 'compositionend'){
      onComposition = false

      //fire change method to update for Chrome v53
      this.handleChange(event)

    } else{
      onComposition = true
    }
  }

check the demo here: https://jsfiddle.net/eyesofkids/dcxvas28/11/

@vincent714

This comment has been minimized.

Show comment
Hide comment
@vincent714

vincent714 Oct 9, 2016

@chenxsan did you find out the solution?
you can detect the compositionStart and let a variable equal to true.
Then to use the variable, which you set, at onChange to see if it should fire the query

vincent714 commented Oct 9, 2016

@chenxsan did you find out the solution?
you can detect the compositionStart and let a variable equal to true.
Then to use the variable, which you set, at onChange to see if it should fire the query

flarnie added a commit to flarnie/react that referenced this issue Jun 7, 2017

Backport input fix (facebook#8575)
* 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)

@layershifter layershifter referenced this issue Jun 12, 2017

Closed

feat(UniversalInput): add component #1753

0 of 1 task complete
@SimeonC

This comment has been minimized.

Show comment
Hide comment
@SimeonC

SimeonC Jun 26, 2017

Just to add fuel to the fire, I've been trying to workaround this issue and discovered that the current version of iOS safari doesn't trigger the compositionend event when using Japanese Hiragana IME, I think this is intentional as the composition menu never seems to be closed.
On @eyesofkids example workaround the inputValue is never updated, though for me https://github.com/zhaoyao91/react-optimistic-input fixes the issue with Japanese IME.

SimeonC commented Jun 26, 2017

Just to add fuel to the fire, I've been trying to workaround this issue and discovered that the current version of iOS safari doesn't trigger the compositionend event when using Japanese Hiragana IME, I think this is intentional as the composition menu never seems to be closed.
On @eyesofkids example workaround the inputValue is never updated, though for me https://github.com/zhaoyao91/react-optimistic-input fixes the issue with Japanese IME.

@aprilandjan

This comment has been minimized.

Show comment
Hide comment
@aprilandjan

aprilandjan Jul 18, 2017

For anyone looking for solution for this, here's a ready-to-use component. https://github.com/aprilandjan/react-starter/blob/test/search-input/src/components/SearchInput.js Just use it instead of normal text input element and everything is ok.

aprilandjan commented Jul 18, 2017

For anyone looking for solution for this, here's a ready-to-use component. https://github.com/aprilandjan/react-starter/blob/test/search-input/src/components/SearchInput.js Just use it instead of normal text input element and everything is ok.

@PokimLee

This comment has been minimized.

Show comment
Hide comment
@PokimLee

PokimLee Aug 28, 2017

@zhaoyao91 your workaround just works! thx a lot.

PokimLee commented Aug 28, 2017

@zhaoyao91 your workaround just works! thx a lot.

@gaearon gaearon added the Type: Bug label Oct 4, 2017

@gaearon gaearon referenced this issue Oct 4, 2017

Open

Umbrella: React DOM Bugs #11097

11 of 28 tasks complete
@cristianroche

This comment has been minimized.

Show comment
Hide comment
@cristianroche

cristianroche Apr 11, 2018

hey guys some news in this issue?

cristianroche commented Apr 11, 2018

hey guys some news in this issue?

lingceng added a commit to lingceng/select2 that referenced this issue Apr 13, 2018

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 1, 2018

Member

It hasn't been a high priority because onChange firing too frequently rarely causes problems. Where is it causing problems in your app?

Member

sophiebits commented May 1, 2018

It hasn't been a high priority because onChange firing too frequently rarely causes problems. Where is it causing problems in your app?

@craigkovatch

This comment has been minimized.

Show comment
Hide comment
@craigkovatch

craigkovatch May 1, 2018

@sophiebits sorry accidentally clicked the 'X'. This can degrade performance if there are filtering operations or server callbacks used in the change event handlers. The approach shown in #3926 (comment) is a fine workaround for uncontrolled or native inputs but doesn't map well to React controlled inputs. Seems like some in this thread have tried to develop a PR but found the internals a bit complex -- but maybe an engineer on your team could make quicker work of it? #8683 is a much better description of the real issue IMO.

craigkovatch commented May 1, 2018

@sophiebits sorry accidentally clicked the 'X'. This can degrade performance if there are filtering operations or server callbacks used in the change event handlers. The approach shown in #3926 (comment) is a fine workaround for uncontrolled or native inputs but doesn't map well to React controlled inputs. Seems like some in this thread have tried to develop a PR but found the internals a bit complex -- but maybe an engineer on your team could make quicker work of it? #8683 is a much better description of the real issue IMO.

lingceng added a commit to lingceng/select2 that referenced this issue May 16, 2018

@gaearon gaearon changed the title from Change event fires too many times when inputing Chinese characters. to Change event fires before IME composition has ended Aug 29, 2018

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 29, 2018

Member

Can somebody please help me understand: is the problem strictly in extra onChange calls in the middle? Or do you get incorrect value in the end?

Member

gaearon commented Aug 29, 2018

Can somebody please help me understand: is the problem strictly in extra onChange calls in the middle? Or do you get incorrect value in the end?

@gaearon gaearon changed the title from Change event fires before IME composition has ended to Change event fires extra times before IME composition ends Aug 29, 2018

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 29, 2018

Member

The test from the fix attempt in #8438 passes if I remove the assertion about the number of times onChange is called. So I suppose this issue is only about the extra onChange calls.

Member

gaearon commented Aug 29, 2018

The test from the fix attempt in #8438 passes if I remove the assertion about the number of times onChange is called. So I suppose this issue is only about the extra onChange calls.

@crochefluid

This comment has been minimized.

Show comment
Hide comment
@crochefluid

crochefluid Aug 29, 2018

there are no extra onChange calls, it is just getting the incorrect value at the end, seems more like a onComposition problem.

crochefluid commented Aug 29, 2018

there are no extra onChange calls, it is just getting the incorrect value at the end, seems more like a onComposition problem.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 29, 2018

Member

@crochefluid Can you create a failing test for this? Similar to what #8438 tried to do. In that test, there was no incorrect value.

Member

gaearon commented Aug 29, 2018

@crochefluid Can you create a failing test for this? Similar to what #8438 tried to do. In that test, there was no incorrect value.

@crochefluid

This comment has been minimized.

Show comment
Hide comment
@crochefluid

crochefluid Aug 29, 2018

@gaearon I'm gonna try it. Did you try that test on safari (mac/IOS)?

crochefluid commented Aug 29, 2018

@gaearon I'm gonna try it. Did you try that test on safari (mac/IOS)?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 29, 2018

Member

It’s a Node test but it encodes sequences captured from different browsers and devices. Please see its source. You’d need to add sequences that are failing.

Member

gaearon commented Aug 29, 2018

It’s a Node test but it encodes sequences captured from different browsers and devices. Please see its source. You’d need to add sequences that are failing.

@yesmeck

This comment has been minimized.

Show comment
Hide comment
@yesmeck

yesmeck Oct 16, 2018

So I suppose this issue is only about the extra onChange calls.

Exactly.

yesmeck commented Oct 16, 2018

So I suppose this issue is only about the extra onChange calls.

Exactly.

@robbyemmert

This comment has been minimized.

Show comment
Hide comment
@robbyemmert

robbyemmert Oct 16, 2018

I'm still getting this issue. It looks like this issue has been open for 3 years, does React support Chinese input in controlled components at the moment?

robbyemmert commented Oct 16, 2018

I'm still getting this issue. It looks like this issue has been open for 3 years, does React support Chinese input in controlled components at the moment?

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