Bug: setState in onFocus breaks onChange event in Concurrent Mode

[airhorns]

Using concurrent mode on 0.0.0-experimental-7f28234f8 makes <Checkbox/> from Baseweb stop firing onChange events.

Baseweb is a popular set of React components that reflects real world usage patterns such that maybe it should work right off the bat with Concurrent mode. Feel free to close if you're confident this is a userland problem and not a framework problem, but I think this may indicate an incompatibility between Concurrent Mode and Legacy mode that isn't documented or warned about it if so, because <Checkbox/> isn't doing anything too fancy and doesn't use deprecated APIs or warn when rendered in Strict mode or Concurrent mode. I feel like that means it's supposed to just work, right?

React version: 0.0.0-experimental-7f28234f8

Steps To Reproduce

1. Click a <Checkbox/> rendered using Legacy mode and note that it checks
2. Click a <Checkbox/> rendered using Concurrent mode and note that it does not check

See issue here: https://baseweb-react-concurrent-checkbox.stackblitz.io (edit here: https://stackblitz.com/edit/baseweb-react-concurrent-checkbox?file=src/index.js)

I've been trying to debug and haven't found any smoking guns yet, but there are some interesting tidbits I can share that might allude to if this is a React bug or a Baseweb bug:

• onFocus and onBlur handlers fire just fine on both modes
• <Checkbox/> doesn't use findDOMNode or other deprecated APIs in Concurrent mode as best I can tell. No warnings are fired when rendering it in Strict mode or Concurrent + Strict mode
• focusing the Checkbox by tabbing and pressing space checks and fires the onChange handler on both Legacy and Concurrent mode
• Wide swaths of the rest of baseweb work just fine in Concurrent mode

See https://github.com/uber/baseweb/issues/3476 for the baseweb issue.

[gaearon]

Concurrent mode isn't supposed to require any code changes to work

This sounds a bit like an oversimplification. Concurrent Mode has several differences in behavior (some of them would still change before the release) so it's not expected that 100% of the code would work. Otherwise it wouldn't be a mode.

That said, if you can reduce this to a small repro case that doesn't involve third-party libraries, we can take a close look. I don't think it's going to be scalable for us to look at every single issue from every single third-party library. So far, there hasn't been conclusive evidence that there is a bug in React based on what you wrote.

[gaearon]

So the best way to diagnose this is to copy and paste Checkbox code into your sandbox, and then start removing things until you find the minimal possible repro.

[airhorns]

That makes sense -- I will try to isolate and find a repro that proves it's a React issue, will close for now.

[airhorns]

Ok, I think I have a pretty minimal reproduction that shows the same issue when using plain ole React here:

https://stackblitz.com/edit/react-concurrent-checkbox?file=src%2FCheckbox.js

The same <Checkbox/> component is rendered 3 times in 3 different roots, in each of Concurrent, Blocking and Legacy mode. React only seems to fire the onChange handler in Legacy mode.

My understanding of the internals of the React event system is limited, but I know that it is trying to dispatch the click event, but I get kind of lost trying to follow the stack of where that dispatch goes in Legacy mode that it doesn't in Concurrent. Would love some assistance debugging.

[aweary]

I took a brief look, and the issue seems to stem from onFocus setting state. Here's a further reduced example that reproduces the problem. The first time you click on that "fake" checkbox, nothing happens. If you remove the setFocused call in onFocus it works as expected.

[gaearon]

Wonder if this is related to #19290 or #19078.

[gaearon]

Just wanted to follow up that we have some related changes planned in this area in the next months to simplify the model, so we'll be looking at this bug after those changes.

[gaearon]

I wanted to note that if you do

  onFocus = e => {
    ReactDOM.flushSync(() => {
      this.setState({ isFocused: true });
      this.props.onFocus(e);
    });
  };

this fixes the problem as a temporary workaround.

[airhorns]

👋 any update on this bug? I know it's prerelease software and it would be unreasonable to have any expectations, but, I have hacked in the flushSync fix to my UI library and we're still experiencing some similar issues. I don't think the workaround fully works around the problem, though it did improve things in a number of situations. I will continue hunting and make sure that it isn't just a missed application of the workaround.

[eps1lon]

Closing in favor of #18591 which describes the same issue. Current workaround is explained in #18591 (comment). Test for this bug can be found in #19617.

[gaearon]

This appears to be fixed with some flags we're planning to enable. Here's the sandbox demonstrating it (note it's built from #20792 rather than experimental because those flags aren't yet enabled in an experimental npm release).

https://codesandbox.io/s/samc-focus-demo-forked-7mbkl?file=/src/App.js

CM version looks fixed.

There's still an issue with the Blocking Mode version, but we'll be addressing it separately in a different way.

[gaearon]

Here's the reduced sandbox, also fixed when the same flags are enabled.

https://codesandbox.io/s/samc-focus-demo-forked-8znoo?file=/src/App.js

[gaearon]

The latest experimental release has the fix.

Also, Blocking Mode was deleted altogether so that part is solved too.