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

onChange doesn't fire if input re-renders due to a setState() in a non-React capture phase listener #13424

Open
gaearon opened this issue Aug 17, 2018 · 13 comments
Labels
Component: DOM React Core Team Opened by a member of the React Core Team Type: Bug

Comments

@gaearon
Copy link
Collaborator

gaearon commented Aug 17, 2018

Extracting from #12643.

This issue has always been in React. I can reproduce it up to React 0.11. However it's probably extremely rare in practice and isn't worth fixing. I'm just filing this for posterity.

Here is a minimal example.

class App extends React.Component {
  state = {value: ''}
  handleChange = (e) => {
    this.setState({
      value: e.target.value
    });
  }
  componentDidMount() {
    document.addEventListener(
      "input",
      () => {
        // COMMENT OUT THIS LINE TO FIX:
        this.setState({});
      },
      true
    );
  }
  render() {
    return (
      <div>
        <input
          value={this.state.value}
          onChange={this.handleChange}
        />
      </div>
    );
  }
}

ReactDOM.render(<App />, document.getElementById("container"));

Typing doesn't work — unless I comment out that setState call in the capture phase listener.

Say the input is empty and we're typing a.

What happens here is that setState({}) in the capture phase non-React listener runs first. When re-rendering due to that first empty setState({}), input props still contain the old value ("") while the DOM node's value is new ("a"). They're not equal, so we'll set the DOM node value to "" (according to the props) and remember "" as the current value.

screen shot 2018-08-17 at 1 08 42 am

Then, ChangeEventPlugin tries to decide whether to emit a change event. It asks the tracker whether the value has changed. The tracker compares the presumably "new" node.value (it's "" — we've just set it earlier!) with the lastValue it has stored (also "" — and also just updated). No changes!

screen shot 2018-08-17 at 1 10 59 am

Our "a" update is lost. We never get the change event, and never actually get a chance to set the correct state.

@Kachulio1
Copy link
Contributor

@gaearon I would love to take this one whats the difficulty level?

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 20, 2018

I’m not sure but since it has always been broken I guess it’s not the easiest one. I’m not even sure it can be solved nicely.

@whs-dot-hk
Copy link

whs-dot-hk commented Aug 31, 2018

@gaearon Can it be solved by passing the input value at that moment along before running setState?

enqueueSetState(inst, payload, callback) {
const fiber = ReactInstanceMap.get(inst);
const currentTime = requestCurrentTime();
const expirationTime = computeExpirationForFiber(currentTime, fiber);
const update = createUpdate(expirationTime);
update.payload = payload;
if (callback !== undefined && callback !== null) {
if (__DEV__) {
warnOnInvalidCallback(callback, 'setState');
}
update.callback = callback;
}
enqueueUpdate(fiber, update);
scheduleWork(fiber, expirationTime);
},

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 31, 2018

Maybe, I haven't dived deep into it. Send a PR?

@whs-dot-hk
Copy link

whs-dot-hk commented Aug 31, 2018

Sorry I can't figure out another way but add another snapshot argument, which is ugly... so let it be

@vinay72
Copy link

vinay72 commented Sep 25, 2018

Instead of using this onChange={this.handleChange}, you can use onChange={event => this.handleChange(event.target.value)} />

@necolas necolas added the React Core Team Opened by a member of the React Core Team label Jan 8, 2020
@arkakkar
Copy link

arkakkar commented Apr 15, 2020

The issue here is not only because of non-React capture phase listener . I have a separate use case reproducing the same issue, since react makes batch updates to state once per event handler so if multiple events are attached (eg onKeydown and onChange) here onKeydown handler triggers a state update using the useReducer hook which in turn triggers a re-render thus re-setting the updated/changed DOM node value to what is present in the state thus not firing onChange event at all.
These two changes worked for me

  1. replacing the onKeydown with onKeyup thus triggering it after the onChange event
  2. handling the keyDown handler in an async way, refer to this for more details https://reactjs.org/docs/events.html#event-pooling

onKeyDown = {(e) => { e.persist(); setTimeout(() => { this.onKeyDown(e) }, 0); }}

@owenDods
Copy link

Oh man, I wish I'd stumbled across this thread earlier - was banging my head against this issue for a while and thought I was going insane.

I managed to reproduce my simplified use case here.

I have an application where I've started to add in keyboard shortcuts, hence why I was making use of document.addEventListener as well as other form inputs on the page.

What I was seeing in my app while I was trying to debug this really confused me. When I added debugger statements within the relevant useEffect hook I could see the text appear in the input then disappear within the useEffect hook's cleanup function - all without firing the input's onChange.

While I appreciate that it might not be pragmatic to try and fix what might be considered an "extremely rare" issue; how realistic would it be to add a warning for users about this potential conflict? Maybe even just adding a "N.B." to the documentation?

When I searched for "addEventListener" on the reactjs.org site it took me to Handling Events. Maybe a note here could save people some hassle? Or making a "Gotchas" page for known edge-case issues like this that are known but aren't going to be fixed anytime soon?

In any case, thanks for filing this issue in the first place - it gave me some much needed closure!

@peluprvi
Copy link

peluprvi commented Nov 4, 2020

I got this issue on a requirement where the select file dialog should automatically open when the form is opened/rendered/mounted (like an autofocus behaviour). I can't avoid the form re-render due to other code's part. In this case, the select file dialog opens but the input file onChange event doesn't trigger when the user confirms the select the files.

Edit: in case someone is having the same required and is looking for a solution, I did a workaround by creating a second input outside the form.

@ottworks
Copy link

ottworks commented Jun 30, 2021

I have come across this bug in practice - mouse coordinates are kept in state and communicated via props. The result is that controlled select elements are inoperable by the mouse, but work fine with the keyboard.

@westlife29
Copy link

I got this issue on a requirement where the select file dialog should automatically open when the form is opened/rendered/mounted (like an autofocus behaviour). I can't avoid the form re-render due to other code's part. In this case, the select file dialog opens but the input file onChange event doesn't trigger when the user confirms the select the files.

Edit: in case someone is having the same required and is looking for a solution, I did a workaround by creating a second input outside the form.

Hi Can you show me what did you do on your work around please ? Thank you !

@rudhman
Copy link

rudhman commented Feb 21, 2022

I was able to leverage queueMicrotask https://developer.mozilla.org/en-US/docs/Web/API/queueMicrotask without having to move the input out. @peluprvi

@eps1lon
Copy link
Collaborator

eps1lon commented Feb 23, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: DOM React Core Team Opened by a member of the React Core Team Type: Bug
Projects
None yet
Development

No branches or pull requests

13 participants