React: Bug: onFocus and onBlur has inconsistent batching

[samcooke98]

React version: 0.0.0-experimental-4c8c98ab9 with concurrent mode.

Steps To Reproduce

1. In the provided code sandbox, click on a Todo component
2. Tab back and forth between the two
3. At inconsistent times, there will be a frame where the focus hasn't left, but the outline isn't rendered. The outline is only drawn when focus is in. Because there is a brief frame without the border, the todo visibly flickers.

Link to code example:

https://codesandbox.io/s/samc-focus-demo-9jyso?file=/package.json:201-229

Video; https://drive.google.com/file/d/18PW8M_VvE3Nau7NkdQq4X8En_Vcen_xQ/view?usp=sharing
Focus shifts multiple times at the beginning, without a flicker.
Around the 6 seconds mark, there is a flicker.

Photo of the frame;

The current behavior

At times there is an 'impossible' frame, where the focus has been set to false, despite the focus just having shifted from one element to another within the same div.

The expected behavior

I'm not sure if this pattern is something that is concurrent mode incompatible or something...

But regardless, I'd expect the behaviour to be consistent - eg for focus and blur events to be combined into a single frame.

This codepen shows the expected outcome with vanila JS https://codepen.io/samcooke98/pen/OJMaXvx

Related?

https://github.com/facebook/react/issues/19385

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

[jaredpalmer]

Any updates on this? Seeing weird behavior with setState in onFocus and onBlur

[gaearon]

We haven’t gotten to this yet but it’s planned. This is CM-only, right?

[jaredpalmer]

@gaearon correct seeing it on experimental with CM. Confirmed it wasn't recoil either.

[gaearon]

As a temporary fix, like always with this kind of issue, you can wrap setState into ReactDOM.flushSync:
https://codesandbox.io/s/samc-focus-demo-forked-g08oj

[jaredpalmer]

Can you explain the difference between that and wrapping with unstable_batchedUpdate? I’m sure they are completely different, but humor me.

[gaearon]

Ah, yeah.

• batchedUpdates means "don't let the setStates inside commit independently". So it prevents flush inside but it doesn't force the flush on exit.
• flushSync means "commit the setStates inside as you exit me". So it doesn't prevent flushes inside (afaik) but it forces a flush on exit.

So batchedUpdates is useful when you have multiple setStates (e.g. due to Flux subscription) but you want to only have one render pass. It's less useful in CM because we already batch things by default more aggressively in it.

Whereas flushSync is useful to force a thing to happen now. Like if iOS prevents audio from playing if you don't do it in the same tick, so you say "apply this now". It has a caveat that it's not always possible (e.g. you can't call it from inside a lifecycle to avoid recursive commits).

[gaearon]

For example,

function handleClick() {
  batchedUpdates(() => {
    setStateA()
    setStateB()
  })
  console.log('hello')
}

This is actually useless because React event handers are already batched. You can think of it as there being an outermost batchedUpdates call. The render pass begins when you exit the outermost batchedUpdates call. And then the actual flush happens depending on the mode (e.g. in CM it may be time-sliced, although we plan to make changes for discrete events like focus/click).

On the other hand, something like this:

function handleClick() {
  flushSync(() => {
    setStateA()
    setStateB()
  })
  console.log('hello')
}

actually forces both the render pass and the commit to happen before your log. Because it's useful to enforce the DOM has already been changed. Like if you need to do something with it. Usually comes up in cases where you're interfacing with some browser API that needs to be called right away.

[gaearon]

Correction about batchedUpdates:

It's less useful in CM because we already batch things by default more aggressively in it.

It's actually completely useless in CM because its semantics don't affect anything. This is because in CM there is nothing that would force setState to flush "right now" by default. So there is no need for a method that prevents that from happening.

[jaredpalmer]

Thank you! Very useful. So flushSync is definitely what I need as I’m doing programmatic scroll into view + focus based on some effects and keyboard events.