[react-dom] Enforce small gap between completed navigation and default Transition indicator #33354
Conversation
…s triggered We delay the default Transition indicator by 100ms to reduce jank in case we're dealing with a fast Transition. However, this delay was previously interrupted if a navigation completed before the default Transition indicator is shown. Instead of interrupting, we now reschedule the default Transition indicator while ensuring we don't indefinitely defer. This also prevents a crash in Chrome (https://issues.chromium.org/u/1/issues/419746417). ## Test plan - [x] "Provoke Chrome crash" in Flight fixture no longer crashes the page
|
Comparing: c0464ae...f8ab64e Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
| } else { | ||
| // Since it hasn't started yet, we want to delay it up to a bit. | ||
| // There needs to be an async gap to work around https://issues.chromium.org/u/1/issues/419746417. | ||
| const fakeNavigationDelay = Math.max( |
There was a problem hiding this comment.
I usually don't trust Math.max because it can be patched it is not automatically optimized like syntax can be. This is not a hot path but I try to avoid it as a pattern so that people don't copy paste it elsewhere.
There was a problem hiding this comment.
Nobody ever patches globals 😄
There was a problem hiding this comment.
The problem is that even if it's not patched, it can't be optimized as well because the VM can't assume it won't be. At least not without compromises.
| startFakeNavigation(); | ||
| if (wasRunning) { | ||
| // Restart sync to make it not janky if it was already running | ||
| startFakeNavigation(); |
There was a problem hiding this comment.
Wouldn't this cause the Chrome bug to happen anyway in this scenario?
There was a problem hiding this comment.
It seems to only crash if we start the spinner from navigationcomplete triggered by replaceState. If the spinner is already running, starting the fake sync navigation is fine. Probably because the earlier pendingResolve call will only stop the spinner in the next task?
There was a problem hiding this comment.
That's weird. I wonder if there's a race condition though that
Regardless, doesn't this trigger the spinner to restart? So maybe it's worth waiting a little to see if we get unblocked to avoid restarting it? Since it'll glitch regardless if we restart but not if we can avoid it.
There was a problem hiding this comment.
Regardless, doesn't this trigger the spinner to restart?
It doesn't actually for the case where another navigation completes before this Transition finished: navigationcomplete triggers while navigation.transition is still set even though it finishes right after in navigation.transition.finished.then. So we might have to add listeners to the navigation.transition.finished Promise as well.
Anyway, the current implementation can't be used as it crashes Chrome. So while I would definitely wait for Chrome to acknowledge the issue before landing this in Canary, I'd still merge this so that we can at least continue experimenting with it.
In the meantime I come up with some more synthetic scenarios that increase our coverage
| // There needs to be an async gap to work around https://issues.chromium.org/u/1/issues/419746417. | ||
| const fakeNavigationDelay = Math.max( | ||
| 0, | ||
| defaultTransitionIndicatorDelay - (performance.now() - scheduledAt), |
There was a problem hiding this comment.
I feel like the rationale here is a bit inversed.
If an unrelated navigation happens in beginning happen then there already is a spinner showing. The rationale for delaying it doesn't make us avoid a spinner from being shown at all since it was already triggered. The reason for the original default delay doesn't apply in this scenario because there was already one going. In fact, delaying it just makes it feel like there was two separate actions instead of one sequence.
On the other hand if this happens after the original one has already started, and a different navigation comes in to briefly interrupt and then we want to resume, it might be good to avoid restarting if we're about to finish.
So I feel like in either scenario there should be a small gap to let any pending tasks that unblocks finish it before we start but not so long to let it feel like a separate action.
Basically, this should just always be like 20ms whether we interrupted the first start or was already running and restarted.
There was a problem hiding this comment.
In fact, delaying it just makes it feel like there was two separate actions instead of one sequence.
Is that just because we entangle Transitions currently and won't let them commit independently? Because I feel like there is a desire to let two interleaved Transitions commit independently and in that model we'd want two spinners instead of one sequence?
If an unrelated navigation happens in beginning happen then there already is a spinner showing.
We don't know if there's already a spinner because history.(push|replace)State trigger navigationcomplete.
Basically, this should just always be like 20ms whether we interrupted the first start or was already running and restarted.
Wouldn't that make it more likely that the spinner is flashing?
There was a problem hiding this comment.
Because I feel like there is a desire to let two interleaved Transitions commit independently and in that model we'd want two spinners instead of one sequence?
Maybe if we restarted it upon the start of each Transition so it stops and restarts every time you start a new action but even then I'd think we'd still want to entangle the stop in case a later Transition ends before an earlier. Doesn't really address this scenario.
The thing to avoid is the scenario when the spinner starts disconnected from any interaction like a click. That's when it feels like something happened outside the user's control and something just started automatically and it's unclear what I'm waiting on.
When there are no other Navigations on the page that aren't coordinated by a React Transition, (e.g. pushState in useLayoutEffect should be ok), we can avoid that scenario.
In that case this should only happen in the scenario where you have unrelated navigations on the page or things like MPA navs (including bfcache navs that might restore the page back into a loading state).
Another scenario it can happen is if you are implementing a router using the Navigation API for push navigation. Now we don't actually recommend that because the Navigation API doesn't support the commit: 'after-transition' option yet, but if you insist we should support it.
In that scenario the spinner would potentially first restart upon the Navigation start. If an Action is still ongoing after the Navigation finishes, we should restart the indicator so that it keeps going.
A common scenario might for example be that upon click you start a React Transition and then in the same click but in a later macrotask, you start the Navigation such as if you listen to document.body clicks. In that scenario, the React Navigation would start and immediate stop due to the actual Navigation that comes later. Then let's say that Navigation takes 50ms to complete, it has already started a spinner so there's no reason for us to wait another 50ms before we restart it.
Now if the site instead did the same thing with the legacy pushState API then maybe there wasn't a spinner to begin with since it's immediately updating and in that case maybe it would be ok to wait but I feel like it's worse to wait longer in the Navigation API scenario to make it feel like it's auto-navigating without user input.
We could maybe detect it by tracking when navigations start and stop to see how long a spinner might have been running. If it was immediate, then we know we can wait but given that the code size of this default implementation is sensitive since everyone pays for it whether they use it or not. Doesn't seem worth it.
So that's the reason to favor 0ms or 20ms over 100ms.
Whether 0ms or 20ms is better for the immediate restart is a bit difficult. There's two reasons to favor 20ms. One is just that if you have programmatic logging etc observing the navigations, then you have less interference from fake ones so that's a reason to try to avoid them if we can.
If the UI causes a restart of a spinner or if the loading progress line goes back to zero upon restart then that's a reason to favor the 20ms to avoid that scenario when possible. However, if the UI doesn't cause a restart when we use 0ms but instead treats that as just a continuation of the on-going navigation in the browser UI then it's better to do that since that's our ideal anyway.
The problem is we don't know all the UIs that will use this since Safari and Firefox hasn't implemented it and various Chromium browsers might do their own thing.
I believe Chrome restarts the spinner so that's the reason I'd favor 20ms but 0ms is probably ok too given that if we're going to actually be able to finish the transition it's likely in the next macrotask which is already scheduled and so the timer happens after anyway.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
…t Transition indicator (facebook#33354) DiffTrain build for [5717f19](facebook@5717f19)
…t Transition indicator (facebook#33354) DiffTrain build for [5717f19](facebook@5717f19)
We delay the default Transition indicator by 100ms to reduce jank in case we're dealing with a fast Transition. However, this delay was previously interrupted if a navigation completed before the default Transition indicator is shown.
Instead of interrupting, we leave a small async gap to make it clear there are two separate actions.
This also prevents a crash in Chrome (https://issues.chromium.org/u/1/issues/419746417).
Test plan