-
Notifications
You must be signed in to change notification settings - Fork 46.4k
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 a capture-phase document listener for the underlying native event calls setState() #12643
Comments
Actually, the HTML elements are structured in a wrong way! 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. At least that would enforce people to write better HTML 😉 |
This is an interesting bug. Adding global Any of the below steps makes it working
document.addEventListener("click", this.handleIncrement, false);
increment() {
//this.setState((prevState, props) => ({
// count: prevState.count + 1
// }));
} |
@stmoreau thank you, but @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 |
this seems like a bug to me. I think #10830 should fix it |
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? |
Humm... You'll see that your example in React 16 will work well when you configure addEventListener to trigger callback in bubbling phase
But still be a bug because the capture phase flag only set in what direction should be propagate the event. |
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")); |
While #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 Say the input is empty and we're typing What happens here is that Then, Our |
I can reproduce this going as far back as... React 0.11. |
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 |
In particular I don't understand why in this example we end up committing 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")); |
I split this out into two issues.
|
The particular regression in 16 was fixed via #13423 so you'll get the fix in the next patch. |
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
The text was updated successfully, but these errors were encountered: