react icon indicating copy to clipboard operation
react copied to clipboard

Change event fires extra times before IME composition ends

Open chenxsan opened this issue 9 years ago • 50 comments

Extra details

  • Similar discussion with extra details and reproducing analysis: https://github.com/facebook/react/issues/8683
  • Previous attempt to fix it: https://github.com/facebook/react/pull/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 avatar May 21 '15 06:05 chenxsan

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

sophiebits avatar May 21 '15 17:05 sophiebits

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.

hellendag avatar May 21 '15 18:05 hellendag

~~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 avatar Sep 15 '15 08:09 Bertg

Is there any update? Suffering from this issue too.

jasonslyvia avatar Sep 16 '15 06:09 jasonslyvia

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.

sophiebits avatar Sep 23 '15 16:09 sophiebits

@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 avatar Oct 02 '15 09:10 pswai

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

suhaotian avatar May 31 '16 06:05 suhaotian

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 avatar Aug 05 '16 19:08 zhaoyao91

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 avatar Aug 16 '16 15:08 eyesofkids

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

zhaoyao91 avatar Aug 17 '16 07:08 zhaoyao91

nice work !

suhaotian avatar Aug 18 '16 00:08 suhaotian

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 avatar Aug 21 '16 22:08 julen

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 😕.

julen avatar Sep 13 '16 09:09 julen

https://github.com/suhaotian/react-input maybe help someone

suhaotian avatar Sep 13 '16 18:09 suhaotian

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 avatar Oct 05 '16 13:10 eyesofkids

@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 avatar Oct 09 '16 13:10 vincent714

I have submited a new issue for controlled components in #8683

The temporary solution for uncontrolled and controlled components(input, textarea) is uploaded to react-compositionevent.

eyesofkids avatar Jan 04 '17 17:01 eyesofkids

Let's make https://github.com/facebook/react/pull/8438 happen.

yesmeck avatar Jan 05 '17 02:01 yesmeck

@yesmeck very happy to see this news.

I saw the test only focus on the Webkit, it should be separate into Chrome and Safari because Chrome change its compositionend event triggered order after 53+.

eyesofkids avatar Jan 06 '17 21:01 eyesofkids

@eyesofkids Added a new test case for Chrome under 53.

yesmeck avatar Jan 08 '17 05:01 yesmeck

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 avatar Jun 26 '17 23:06 SimeonC

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 avatar Jul 18 '17 12:07 aprilandjan

@zhaoyao91 your workaround just works! thx a lot.

PokimLee avatar Aug 28 '17 12:08 PokimLee

hey guys some news in this issue?

cristianroche avatar Apr 11 '18 18:04 cristianroche

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

sophiebits avatar May 01 '18 00:05 sophiebits

@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 https://github.com/facebook/react/issues/3926#issuecomment-316049951 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? https://github.com/facebook/react/issues/8683 is a much better description of the real issue IMO.

craigkovatch avatar May 01 '18 00:05 craigkovatch

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 avatar Aug 29 '18 17:08 gaearon

The test from the fix attempt in https://github.com/facebook/react/pull/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.

gaearon avatar Aug 29 '18 17:08 gaearon

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

crochefluid avatar Aug 29 '18 18:08 crochefluid

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

gaearon avatar Aug 29 '18 18:08 gaearon

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

crochefluid avatar Aug 29 '18 18:08 crochefluid

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.

gaearon avatar Aug 29 '18 20:08 gaearon

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

Exactly.

yesmeck avatar Oct 16 '18 06:10 yesmeck

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 avatar Oct 16 '18 12:10 robbyemmert

Also seeing this in Japanese with certain characters...

cbengtson85 avatar Oct 26 '18 17:10 cbengtson85

Here's a code sandbox that reproduces my issue. Looks like it's related to forms. Using inputs directly is fine.

https://codesandbox.io/s/0m1760xqnl

robbyemmert avatar Oct 27 '18 09:10 robbyemmert

I added some cases: Using react state and plain inputs is fine Using react state, plain forms, and plain inputs is fine We're using a context-based form component which isn't working. It might be a context-related issue.

robbyemmert avatar Oct 27 '18 10:10 robbyemmert

Problem solved: I got it working in codepen. For some reason passing 'input' in as a component worked, when passing (props) => <input {...props]/> did not.

Anyone have an idea what the difference is?

Actually, I've also tried:

Works

<Field {...otherProps} component="input" />

Doesn't work

<Field {...otherProps} component={(props) => <input {...props} />} />

Works oddly enough

const WrappedInput = (props) => <input {...props} />
...
<Field {...otherProps} component={WrappedInput} />

Clearly there is some magic going on here that I don't understand. 😕

robbyemmert avatar Oct 27 '18 13:10 robbyemmert

Any updates?

makotot avatar Jan 10 '19 03:01 makotot

It seems to cause incorrect result when IME is enabled

e84721f3ec71a5ce043ef8290

otakustay avatar Feb 21 '19 13:02 otakustay

I've experienced the same issue as @otakustay It seems impossible to support controlled input with IME input. I've traced the sequence of events to the following.

  1. User types a letter, say w
  2. onChange is triggered
  3. State is updated with new value
  4. New value is propagated down to the input by way of the value attribute.
  5. The IME "composition" is interrupted at this point
    • There is a w string in the input element
    • There is also a separate w string stored in the IME buffer
  6. The user types another letter, say a
  7. The string in the input a combines with the string the IME buffer to produce wwa.
  8. Repeat steps 1-7 to get a bunch of duplicate characters.

I've noticed that the bug only occurs if the input re-renders ~~>15ms after the compositionUpdate event~~ after the next repaint.

Right now my only solution is to switch away from controlled inputs.

Edit: Here is a simple reproduction: https://jsfiddle.net/kbhg3xna/ Edit2: Here is my hacky workaround: https://jsfiddle.net/m792qtys/ cc: @otakustay

knubie avatar Feb 26 '19 20:02 knubie

Any updates on this?

blessonbabu avatar Aug 21 '19 03:08 blessonbabu

update ??

huawin03 avatar Oct 24 '19 09:10 huawin03

Any update on this?

turutosiya avatar May 11 '20 09:05 turutosiya

Stunned, I was confronted with this question

eugle avatar Jun 19 '20 10:06 eugle

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

I don't think this is a valid solution as a component might want to know the "uncommitted" IME string for e.g. filtering options in a list as the user types.

I am not sure if the approach I use in this other thread might help those hitting this issue, but here's a link just in case: https://github.com/facebook/react/issues/13104#issuecomment-691393940

craigkovatch avatar Sep 12 '20 03:09 craigkovatch

any updates?

kazukinagata avatar Sep 28 '20 07:09 kazukinagata

any updates?

Hi, I face the same issue when I want to do some operation in handleChange function, and my solution is just use composition event to handle it.

If it works for you, extract it to hooks and we use reuse it easily.

import React, { useState } from 'react';

export default function Solution() {
  const [value, setValue] = useState('');
  const [onComposition, setOnComposition] = useState(false);

  function handleComposition(event) {
    if (event.type === 'compositionstart') {
      setOnComposition(true);
    }
    if (event.type === 'compositionend') {
      setOnComposition(false);
    }
  }
  function handleChange(e) {
    let temp = e.target.value;
    if (!onComposition) {
      // any operation you want to do...
      temp = temp.replace(/z/g, 'Z');
    }
    setValue(temp);
  }

  return (
    <input
      onChange={handleChange}
      value={value}
      onCompositionStart={handleComposition}
      onCompositionEnd={handleComposition}
    />
  );
}

RebaekSoparkKitchen avatar Sep 29 '21 03:09 RebaekSoparkKitchen

also this code https://zhuanlan.zhihu.com/p/415365117

krmao avatar Sep 29 '21 07:09 krmao

also this code https://zhuanlan.zhihu.com/p/415365117

A simpler component:

// refs: https://zhuanlan.zhihu.com/p/415365117

import { useRef, useState, forwardRef } from 'react'

function InputIME(props, ref) {
  const { onChange, value, ...otherProps } = props

  // log if on composition
  const onComposition = useRef(false)
  // temp input
  const [inputValue, setInputValue] = useState('')

  const _onChange = (event) => {
    setInputValue(event.target.value)

    // IME method start
    if (event.type === 'compositionstart') {
      onComposition.current = true
      return
    }

    // IME method end
    if (event.type === 'compositionend') {
      onComposition.current = false
    }

    // handle parent onChange
    if (!onComposition.current) {
      onChange(event)
    }
  }

  return (
    <input
      {...otherProps}
      ref={ref}
      value={inputValue}
      onChange={_onChange}
      onCompositionEnd={_onChange}
      onCompositionStart={_onChange}
    />
  )
}

export default forwardRef(InputIME)

eyesofkids avatar Jul 31 '22 12:07 eyesofkids