remix-run / react-router

Declarative routing for React
https://reactrouter.com
MIT License
52.9k stars 10.24k forks source link

[Bug]: `v7_startTransition` breaks native scroll restoration #11030

Open OliverJAsh opened 10 months ago

OliverJAsh commented 10 months ago

What version of React Router are you using?

6.17.0

Steps to Reproduce

Here's a reduced test case I created: https://github.com/OliverJAsh/react-transition-scroll-restoration/compare/react-router. Branch: react-router.

This is a minimal application that has two pages, with v7_startTransition enabled.

Here's the relevant code: https://github.com/OliverJAsh/react-transition-scroll-restoration/blob/27b6067d47d7490c78b4fd2cf731ba4c006fb783/src/index.tsx#L4-L44

Expected Behavior

When the user navigates backwards/forwards using the history, the browser will natively restore the scroll position after React has updated the page.

Actual Behavior

When the user navigates backwards/forwards using the history, the browser will natively restore the scroll position, however it does so synchronously meaning scroll may be restored before the page has been updated by React, as can be seen in this video (using the reduced test case shared above):

https://github.com/remix-run/react-router/assets/921609/e518e7ea-fb85-4f0c-8ee6-07a56a22b287

I suppose one way to fix this would be to use the ScrollRestoration component instead of relying on native scroll restoration, but I think there should be a way to fix this that doesn't require completely ditching native scroll restoration. (Use the platform™.)

Reading this issue it sounds like Next.js had a similar problem. This is how they solved it:

we've made changes in React to support React Transitions being synchronous when triggered in the popstate event. This ensures React blocks the main thread while rendering on popstate

https://github.com/vercel/next.js/issues/3303#issuecomment-1617910905

If I'm reading this correctly, it sounds like Next.js solved this problem by making "pop" navigations (backwards/forwards) render synchronously i.e. not inside of startTransition. Perhaps React Router could do the same thing?

OliverJAsh commented 10 months ago

@brophdawg11 Do you have any thoughts on this at all?

OliverJAsh commented 6 months ago

@mjackson @ryanflorence Do you have any thoughts on this? This is a bug we have at Unsplash and I'd really like to find a solution. Happy to help in any way I can.