[Presence] React 18 – Fix animation frame sync flicker #1344

andy-hook · 2022-04-26T10:42:44Z

The introduction of batched updates across all handlers in 18 concurrent mode introduces a frame sync issue where the state transition (unmount) is communicated a frame after animation has completed.

Currently we are recommending animation-fill-mode: forwards to cover this gap. While it does work, it creates other problems.

I'm still getting my head around the internal mechanics of react scheduling so feedback here would be very welcome, but as it stands I feel that this change is a superior option vs the css we are recommending.

⚠️ Note for docs: Remove forwards recommendation from demos if accepted?

Before:

flicker.mp4

After:

no-flicker.mp4

benoitgrelard

I do feel like it's a better fix too, I've left a few comments inline.

benoitgrelard · 2022-04-26T11:12:19Z

packages/react/presence/src/Presence.tsx

+          // With React 18 concurrency this update is applied
+          // a frame after the animation ends, creating a flash of visible content.
+          // By manually flushing we ensure they sync within a frame, removing the flash.
+          ReactDOM.flushSync(() => send('ANIMATION_END'));


Interesting…
I'm no unclear as to when flushSync should be used or not.

As far as I understand, we use flushSync to opt-out of automatic batching.

But what's not clear to me here, is that if it was automatically batching and that was the issue, why would it be delayed by a frame? Unless it is batching 2 state updates "later", and opting out results in applying the first one at the correct time for us?

In which case that would kinda make sense.

But what's not clear to me here, is that if it was automatically batching and that was the issue, why would it be delayed by a frame?

This isn't entirely clear to me either, my current thought is that React is pushing this to the next tick as part of the batching mechanism, I saw this eluded to in the wg post:

React doesn’t render the result of the first setState synchronously—the render occurs during the next browser tick. So the render hasn’t happened yet

some code may depend on reading something from the DOM immediately after a state change. For those use cases, you can use ReactDOM.flushSync() to opt out of batching

Which I think is why we're seeing the state change trail the native event, here's a reduced example:

https://codesandbox.io/s/elated-snow-x36ypm?file=/src/App.js

I just remembered this post, which suggests forcing sync as a solution, though that was in the context of useEffect, I suppose the mechanics are similar here but with the events

This is what I'm confused about though with that reduced example.
Once you isolate just the bad case (the red box), there's only really 1 setState right? So where's the batching? There's no batching in this case, or are we saying that React delays it always regardless of whether there are more than 1 updates?

are we saying that React delays it always regardless of whether there are more than 1 updates?

Yes, in a way. I think it's more a case of concurrency fundamentally changing its relationship with browser rendering and how paints are scheduled.

Here's another example that would suggest the same:

facebook/react#24331

Try resizing the grey box using the handle:

https://codesandbox.io/s/amazing-montalcini-j6lz46?file=/src/App.js:894-899

Notice how the native resize is painting ahead of state.

Man yeah definitely seems to be the case, it's pretty clear here.
It does make me think we probably need to do it in other places too then, as we use things like resize observer too…
The worse part in all that really, the part that bothers me is that it seems like there's no way to tell where it should be applied until you see an issue really…

I hear you, though I do think we’ll begin to see best practices emerge over time through guidance from the core team and community (as we did with state, FCs, hooks etc). I’d like to understand the underlying mechanics better though, right now it’s not particularly intuitive when to reach for certain tools.

Something that crossed my mind is where we explicitly opt out of batching using setTimeout when programatically focusing, nothing has come up yet though so we're probably ok. We might need to think about how to test in 17 if we decide to make changes in any spicy areas.

benoitgrelard · 2022-04-26T11:14:15Z

packages/react/presence/package.json

@@ -21,7 +21,8 @@
    "@radix-ui/react-use-layout-effect": "workspace:*"
  },
  "peerDependencies": {
-    "react": "^16.8 || ^17.0 || ^18.0"
+    "react": "^16.8 || ^17.0 || ^18.0",
+    "react-dom": "^16.8 || ^17.0 || ^18.0"


Because of this, we will need to track all of the dependents of Presence and their dependends subsequently and add that peer-dependency too.

Sometimes I wonder if it would be easier to always add them both as peer-dep anyway haha, would there be any harm considering that they are meant to be used with the DOM anyway?

Good point!

Sometimes I wonder if it would be easier to always add them both as peer-dep anyway haha, would there be any harm considering that they are meant to be used with the DOM anyway?

Practically speaking I don't think it'd be a problem, technically inaccurate in terms of the dependency contracts but maybe that's ok in this case? at least we'd never forget in the future.

Yeah exactly, I'm mostly worried that it's hard to track through multiple levels of dependencies, if it was just flat, it might be ok, but it becomes a bit harder when there's more than 1 level.

But I guess let's see how it evolves, it may end up that they mostly all have it anyway for one reason or another (basically if they or any of their deps uses Portal, or Presence now)

benoitgrelard

Approving for the code change, but still needs to deps changes.

jjenzz · 2022-04-26T20:38:55Z

super educational following along here 👀

i just wondered if this would work in React 16.8 tho and my quick hacky try yielded a "no":

https://codesandbox.io/s/react-16-8-0-forked-1vey3y?file=/src/components/App.js

something to look into perhaps? 😬

benoitgrelard · 2022-04-27T09:22:03Z

Yikes!!
How are we supposed to support both??! 😬

andy-hook · 2022-04-27T09:26:30Z

i just wondered if this would work in React 16.8 tho and my quick hacky try yielded a "no":

Interesting, wasn’t aware of this limitation in 16 but worth keeping in mind! I know that's contrived but useLayoutEffect is already sync so not a valid case.

Given that syncing state in useEffect should be avoided I can't imagine many cases where this would bite us. Providing we call it outside of render we should be fine:

https://codesandbox.io/s/react-16-8-0-forked-t6fhlc?file=/src/components/App.js

Thanks for mentioning, underscores this point:

We might need to think about how to test in 17 if we decide to make changes in any spicy areas.

benoitgrelard · 2022-04-27T09:35:23Z

oh right, I missed the useLayoutEffect I thought simply that calling React.flushSync in 16 was an issue.
So I think we're ok to merge then?

jjenzz · 2022-04-28T10:56:15Z

ah my bad, i saw that Presence calls it in a useLayoutEffect so tested the same but missed the fact that it actually happens in an event handler inside the useLayoutEffect. that's good to know.

react 17+ doesn't error with my sandbox, interestingly.

glad it wasn't a tricky one!

benoitgrelard · 2022-04-28T11:27:17Z

Yeah in your sandbox in 17 it doesn't error but it has a warning instead, seems they changed that in 17.

* [Presence] flushSync dom with react * Update stories * Provide comment * Update dependent peerDeps

andy-hook added 3 commits April 26, 2022 11:08

[Presence] flushSync dom with react

47ce6d4

Update stories

78ea151

Provide comment

d94459e

andy-hook requested a review from benoitgrelard as a code owner April 26, 2022 10:42

benoitgrelard reviewed Apr 26, 2022

View reviewed changes

benoitgrelard approved these changes Apr 26, 2022

View reviewed changes

Update dependent peerDeps

78ae189

andy-hook merged commit 6f61521 into main Apr 27, 2022

andy-hook deleted the react-18-fix-animation-flicker branch April 27, 2022 10:23

andy-hook mentioned this pull request Apr 27, 2022

Primitives 1344 documentation radix-ui/website#376

Merged

andy-hook mentioned this pull request May 13, 2022

Height animation doesn't work for nested accordions #1395

Closed

huksley mentioned this pull request Jul 5, 2022

Screen Glitch When Close Modal pradel/react-responsive-modal#495

Closed

luisorbaiceta pushed a commit to luisorbaiceta/primitives that referenced this pull request Jul 21, 2022

[Presence] React 18 – Fix animation frame sync flicker (radix-ui#1344)

1d7e324

* [Presence] flushSync dom with react * Update stories * Provide comment * Update dependent peerDeps

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Presence] React 18 – Fix animation frame sync flicker #1344

[Presence] React 18 – Fix animation frame sync flicker #1344

andy-hook commented Apr 26, 2022 •

edited

Loading

benoitgrelard left a comment

benoitgrelard Apr 26, 2022

andy-hook Apr 26, 2022 •

edited

Loading

benoitgrelard Apr 26, 2022

andy-hook Apr 26, 2022

benoitgrelard Apr 26, 2022

andy-hook Apr 26, 2022 •

edited

Loading

benoitgrelard Apr 26, 2022

andy-hook Apr 26, 2022

benoitgrelard Apr 26, 2022

benoitgrelard Apr 26, 2022

benoitgrelard left a comment

jjenzz commented Apr 26, 2022 •

edited

Loading

benoitgrelard commented Apr 27, 2022

andy-hook commented Apr 27, 2022 •

edited

Loading

benoitgrelard commented Apr 27, 2022

jjenzz commented Apr 28, 2022 •

edited

Loading

benoitgrelard commented Apr 28, 2022

[Presence] React 18 – Fix animation frame sync flicker #1344

[Presence] React 18 – Fix animation frame sync flicker #1344

Conversation

andy-hook commented Apr 26, 2022 • edited Loading

benoitgrelard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-hook Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-hook Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitgrelard left a comment

Choose a reason for hiding this comment

jjenzz commented Apr 26, 2022 • edited Loading

benoitgrelard commented Apr 27, 2022

andy-hook commented Apr 27, 2022 • edited Loading

benoitgrelard commented Apr 27, 2022

jjenzz commented Apr 28, 2022 • edited Loading

benoitgrelard commented Apr 28, 2022

andy-hook commented Apr 26, 2022 •

edited

Loading

andy-hook Apr 26, 2022 •

edited

Loading

andy-hook Apr 26, 2022 •

edited

Loading

jjenzz commented Apr 26, 2022 •

edited

Loading

andy-hook commented Apr 27, 2022 •

edited

Loading

jjenzz commented Apr 28, 2022 •

edited

Loading