React: onChange doesn't fire if a capture-phase document listener for the underlying native event calls setState()

[Reaverart]

Do you want to request a feature or report a bug?
A bug

What is the current behavior?
Checkbox not fires onChange for controlled component, it somehow related to global event listeners with setState, see sandbox example.

What is the expected behavior?
Checkbox should fire onChange handler

Broken example with REACT 16:
https://codesandbox.io/s/8y6jv95k18

Working example with REACT 15 version:
https://codesandbox.io/s/rl3w4nqqzm

[stmoreau]

Actually, the HTML elements are structured in a wrong way!
Based on:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label
your elements should be like so:
<label>Click me <input type="text"></label>

Demo: https://codesandbox.io/s/pk6k50qm1m

Even if that was supported in REACT 15, I am not sure that it's a good idea to add that flexibility in REACT 16.
It would maybe be more beneficial to add that in the migration notes.

At least that would enforce people to write better HTML 😉

[ad1992]

This is an interesting bug.

Adding global onClick listener doesn't fire onChange listener of the controlled checkbox (when it doesn't have a wrapper)

Any of the below steps makes it working

• on changing OnClick global listener to onChange global listener everything works fine.

• if onClick listener is used in checkbox for the controlled component when adding global onClick listener, It works fine.

• If the third paramter in document,addEventListener which is basically the useCapture is set to false, everything works fine

    document.addEventListener("click", this.handleIncrement, false);

• If the setState is commented on the callback of the global event listener then it works fine

increment() {
    //this.setState((prevState, props) => ({
    //  count: prevState.count + 1
    // }));
}

[Reaverart]

@stmoreau thank you, but input dont have to be nested in label and my example is totally valid, you might read it at usage notes section on MDN by link you left. I also suspect that it fixes checkbox only due to nesting, because second checkbox in my example was nested in a div and worked fine. So guess you messing things up.

@ad1992 yes, thank you, there is workarounds, the sad part - sometimes you cant control it, like if we have 3rd party UI lib. Assume its wide issue because global onclick event listener with capture phase used almost everywhere to capture clicks outside elements (popovers, datepickers, selects and etc).

[jquense]

this seems like a bug to me. I think https://github.com/facebook/react/pull/10830 should fix it

[stephenkingsley]

In this case, maybe the checkbox that id is checkbox can not fire onChange event. @jquense I do not know your fix, could you please explain for me?

[stvkoch]

Humm... You'll see that your example in React 16 will work well when you configure _addEventListener_ to trigger callback in bubbling phase

document.addEventListener("click", this.handleIncrement, false);

But still be a bug because the _capture phase flag_ only set in what direction should be propagate the event.

[gaearon]

Wow. This is an interesting bug. Digging into it.

Here's a minimal repro:

import React, { Component } from "react";
import { render } from "react-dom";

class App extends Component {
  handleChange() {
    alert("hi");
  }
  componentDidMount() {
    document.addEventListener(
      "click",
      () => {
        this.setState({});
      },
      true
    );
  }
  render() {
    return (
      <div>
        <input
          type="checkbox"
          checked={true}
          onChange={this.handleChange}
        />
      </div>
    );
  }
}

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

[gaearon]

While https://github.com/facebook/react/pull/10830 would fix it, I don't think it really solves the underlying problem which has to do with the input tracking mechanism.

Here's a repro case that only uses regular inputs, not checkboxes:

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.

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!

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

[gaearon]

I can reproduce this going as far back as... React 0.11.
😄

[gaearon]

There does seem to be a difference in 16 and 15 behavior though. I'm not sure if it's limited to checkboxes or other inputs too. Maybe it's between setState being in same or different components.

[gaearon]

In particular I don't understand why in this example we end up committing input update at all, or even reconciling it (we get into the complete phase). I'd expect it to bail out because setState was in the Counter. Or maybe our bail out doesn't prevent complete phase from running? That would explain why wrapping in a div "fixes" it. Seems like that's a more generic problem that we can fix.

class Counter extends React.Component {
  state = {};

  componentDidMount() {
    document.addEventListener(
      "click",
      this.increment,
      true
    );
  }
  componentWillUnmount() {
    document.removeEventListener(
      "click",
      this.increment,
      true
    );
  }
  increment = () => {
    this.setState({});
  };
  render() {
    return <div>hi</div>;
  }
}

class App extends React.Component {
  handleChange(val) {
    alert("ok");
  }

  render() {
    return (
      <div>
        <Counter />
        <input
          type="checkbox"
          checked={true}
          onChange={e => this.handleChange()}
        />
      </div>
    );
  }
}

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

[gaearon]

I split this out into two issues.

• Generic issue that existed forever and probably isn't worth fixing: https://github.com/facebook/react/issues/13424
• Regression in React 16 that causes it to happen more often: https://github.com/facebook/react/issues/13425 (fix in https://github.com/facebook/react/pull/13423)

[gaearon]

The particular regression in 16 was fixed via https://github.com/facebook/react/pull/13423 so you'll get the fix in the next patch.