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

Disable Next.js scroll restoration #5026

Closed
wants to merge 1 commit into from
Closed

Disable Next.js scroll restoration #5026

wants to merge 1 commit into from

Conversation

gaearon
Copy link
Member

@gaearon gaearon commented Sep 9, 2022

I am adding a hack. I don't want to keep it forever. If someone in the future finds this PR and it causes some pains (like with upgrading Next), feel free to revert it but ping me so I can add it back. Eventually this hack shouldn't be needed because it's really due to how the current Next.js router works, and it's a known issue that should be resolved by the upcoming one.

The problematic behavior is this. Basically, if you scroll even a little bit, the Back gesture no longer shows a preview of the previous/next page during the swipe. This feels confusing and disorienting.

RPReplay_Final1662727484.MP4

I only have a vague understanding of the issue, but here's what I gather so far:

  • Browsers these days have automatic scroll restoration. You normally don't need to do anything special.
  • However, (?) it seems like automatic restoration assumes that you're going to update the UI during popstate or at least close enough to it.
  • Next.js doesn't update the UI during popstate. It waits for a Promise tick (although in PROD it resolves immediately with cached data). Then it renders in startTransition.
  • Since the route transition is asynchronous, (it seems like?) we can't rely on browser built-in scroll restoration. We use Next.js's scroll restoration. But something that Next.js does breaks the swipe gesture preview. I assume this is somehow related to the timing of when Next.js calls scrollTo. Here is a related post but with the opposite problem. If someone wants to dig in why Next.js's custom scroll restoration breaks that heuristic, be my guest! Then maybe we can fix that part and remove my hack.
  • Anyway, I don't want Next.js scroll restoration, and I do want to rely on the browser one so that we don't break the gesture.
  • For that to work, I need to help the browser. I need to make popstate synchronously update the UI. Which this PR (almost) does:
    • It removes startTransition during popstate, which I believe should not be happening in the first place. Transitions should be used for navigations like links, not for the Back button click. We already fixed something similar in vanilla React in Add more non-React events to the priority list facebook/react#20774, so I think Next.js should have some kind of similar behavior.
    • It doesn't get rid of the Promise tick... I don't know why this PR works anyway. I guess waiting for Promise.resolve is fine?
  • But this is not the end. Even if you disable Next.js "scroll restoration" option in the config, Next.js is still going to call scrollTo(). For navigations to new links this is 100% reasonable (we want to scroll to top). But I don't think it should be doing this on popstate since my whole point is that I wanted to let the browser do it! So I disabled the codepath that tries to scrollTo(0, 0) on popstate, since that interferes with what the browser does.

This seems enough to fix the Safari issue on iOS.

Unfortunately this introduces DEV-only jumps on Back/Forward because in DEV, there's not just a Promise tick but an actual refetch on every navigation (even Back navigation). This seems unfortunate.

Overall, all of this is super hacky and I'm happy to get rid of this when/if the new Next.js router fixes this. For now, I'm ok maintaining this hack.

@github-actions
Copy link

github-actions bot commented Sep 9, 2022

Size Changes

📦 Next.js Bundle Analysis

This analysis was generated by the next.js bundle analysis action 🤖

🎉 Global Bundle Size Decreased

Page Size (compressed)
global 84.22 KB (🟢 -112 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

@gaearon
Copy link
Member Author

gaearon commented Sep 10, 2022

After a bit more investigation on twitter it appears that the root cause of the grey background is simply this:

https://github.com/vercel/next.js/blob/abcf991d11131fe45298f57d3f98e5e23b7dfb3f/packages/next/shared/lib/router/router.ts#L1017

So if manual restoration is on (such as Next.js custom one), Safari won't attempt to do the default gesture thing because it doesn't "believe" we're going to show the right UI immediately. Which is fair I guess.

@gaearon
Copy link
Member Author

gaearon commented Sep 10, 2022

let's do #5029 instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants