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

Bug: React 18 commits to DOM too early before microtask runs #24335

Closed
atomiks opened this issue Apr 11, 2022 · 2 comments
Closed

Bug: React 18 commits to DOM too early before microtask runs #24335

atomiks opened this issue Apr 11, 2022 · 2 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@atomiks
Copy link

atomiks commented Apr 11, 2022

React version: 18.0.0

Steps To Reproduce

  1. Visit https://codesandbox.io/s/recursing-cherry-5ih4qf?file=/src/App.js
  2. Refresh the CodeSandbox iframe window

The current behavior

The tooltip should not render before it gets positioned.

In React 17 this does not occur.

The expected behavior

There's a jump.

Context

Here's the relevant code:

https://github.com/floating-ui/floating-ui/blob/43742e9b1736b4738ba8044093c8982dcb44da24/packages/react-dom/src/index.ts#L66-L98

Floating UI's computePosition() function is async (only to support React Native's async measurement methods, in the DOM it happens instantly in the next microtask). If the async wrappers are removed the problem does not occur.

I'm not sure if this is intended or if it's possible to fix. I wonder if userland microtasks can be run before the first commit to the DOM?

To mimic React 17's behavior we'd need to force consumers to use this style:

  • visibility: x != null ? 'visible' : 'hidden'

But it's also broken in the following case (see video): floating-ui/react-popper#349 (comment), which means they'd need to use another state variable, which is cumbersome and requires another render, further reducing perf. It's impractical to remove the async wrappers because React Native is supported (and other platforms with async measurement APIs?)


I'm not sure if "commit to the DOM too early" is even the right term here. More like, "lets the browser paint too early".

The flow in the real DOM (and seemingly React 17):

  • Create element
  • Mount it to the DOM
  • Run callback refs
  • computePosition() schedules a microtask
  • Remaining? Macrotask work runs
  • Microtask work runs
  • Browser paints, element is positioned with no jumps

In React 18, it seems to let the browser paint before the ref callback function’s scheduled microtask can run.

@atomiks atomiks added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Apr 11, 2022
@atomiks
Copy link
Author

atomiks commented Apr 11, 2022

I made an even simpler reproduction here: https://codesandbox.io/s/spring-dew-9kyfx1?file=/src/App.js

It looks like it's related to setting state in the callback ref when that state update happens in a microtask.

  • microtask + state update inside callback ref = browser paints before the state update happens
  • no microtask + state update inside callback ref = browser paints after the state update happens

it also happens with useLayoutEffect instead of the callback ref

@atomiks
Copy link
Author

atomiks commented Apr 11, 2022

I just saw this issue got opened. It's the same thing I'm pretty sure #24331

@atomiks atomiks closed this as completed Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

1 participant