Repository navigation
fix(router): leave back/forward scroll to the browser when it owns restoration - #923
Merged
Merged
Conversation
commit: |
ivogt
force-pushed
the
fix/client-urls-scroll-restore
branch
from
September 28, 2026 11:32
aa614c8 to
f436c83
Compare
ivogt
force-pushed
the
fix/client-urls-scroll-restore
branch
from
September 28, 2026 11:58
f436c83 to
f47615d
Compare
7 tasks done
ivogt
force-pushed
the
fix/client-urls-scroll-restore
branch
from
September 28, 2026 15:13
f47615d to
78c86bc
Compare
6 tasks done
…storation
Without <Html.ScrollRestoration> the browser restores back/forward scroll itself
(history.scrollRestoration "auto"), but handleNavigationEnd({ restore: true })
fell through to scrollToTop() after every popstate. The two raced: desktop
engines restore after the router's scrollTo(0, 0), so the page ended up in
the right place, while a browser that restores first lost to it and landed
at the top (reported in mobile Safari, PLP -> PDP -> back). On back/forward
the router now leaves any "auto" entry to the browser; that also covers
entries created before <Html.ScrollRestoration> mounted.
The bridge's pageshow handler set scrollRestoration = "manual" after every
bfcache restore, even for apps that never mounted <Html.ScrollRestoration>, which
switched native restoration off and brought the scroll-to-top back. Taking
scroll back after bfcache now lives in initScrollRestoration, next to the
pagehide handler that hands it to the browser.
Tests: unit (handleNavigationEnd branches, bfcache mode with and without
<Html.ScrollRestoration>, bridge pageshow), and dev + production e2e in
cloudflare-basic (its own document renders no Html.ScrollRestoration) through
a shared expectBackLeavesScrollToBrowser body (@shared/e2e) that spies on
every scroll API after popstate. No test-app case: since the default document
renders Html.ScrollRestoration, test-app can no longer run without it.
ivogt
force-pushed
the
fix/client-urls-scroll-restore
branch
from
September 28, 2026 16:19
78c86bc to
67014a4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In apps whose own
documentdoesn't render<Html.ScrollRestoration>(the default document renders it since #948), going back to a long page could land at the top instead of where the user was. The browser restores back/forward scroll itself in that setup (history.scrollRestoration === "auto"), but the router also calledscrollTo(0, 0)after everypopstate, and the two raced. Desktop Chromium and WebKit apply their restore after the router's call, so the page ends up in the right place there. When the browser restores first, the router's call wins and the page jumps to the top. Reported on a production storefront in mobile Safari (PLP -> PDP -> back lands at the top of the PLP); the instrumented live page shows the router'sscrollTo(0,0)right afterpopstatein both desktop engines.Two related paths brought the same scroll-to-top back:
pageshowhandler sethistory.scrollRestoration = "manual"after every bfcache restore, including in apps without<Html.ScrollRestoration>. After a user left the site and came back with Back (a payment redirect, say), native restoration was off and the router scrolled to the top again.<Html.ScrollRestoration>mounted. Those stay"auto", so the browser restores them. With restoration initialized but no saved position for the entry, the router still fell through to the top.Before / after
Same app, same navigation: scroll a long list to Y=1500, open a detail page, press back. No
<Html.ScrollRestoration>.popstatescrollTo(0, 0)scrollRestorationforced to"manual": no native restore, router scrolls to top"auto": the browser restoresNo consumer code changes. Router-managed restoration comes from the default document (#948), or from rendering it in your own document:
Semantics
<Html.ScrollRestoration>scroll: falsekeeps position"auto""manual"(set by the app)"auto"(entry predates the mount)"manual"pageshow, persisted)"manual"(now done byinitScrollRestoration)pageshow, persisted)"auto"Code change
src/browser/scroll-restoration.tshandleNavigationEnd({ restore: true }): after trying a saved position, it returns without scrolling when the destination entry is"auto". It checks the entry's mode rather thaninitialized, so an app that sets"manual"itself keeps the old top/hash fallback.initScrollRestorationnow registers thepageshowhandler that re-takes"manual"after a bfcache restore, next to thepagehidehandler that hands scroll to the browser. Only apps that opted in get it.src/browser/navigation-bridge.ts: thepageshowhandler only aborts the stale navigation; it no longer touchesscrollRestoration.tests/shared-e2e/src/index.ts:expectBackLeavesScrollToBrowser, the shared e2e body. Same precedent asexpectStreamedBoundariesAdopted.Test plan
scroll-restoration.test.ts:handleNavigationEndin auto mode does not scroll on back/forward."auto"entry with restoration initialized is left to the browser."manual"with<Html.ScrollRestoration>; unchanged without it.afterEach.navigation-bridge.test.ts:pageshow(persisted) aborts the stale navigation and leavesscrollRestorationalone.<Html.ScrollRestoration>, bridgepageshow)./->/about, a custom document without<Html.ScrollRestoration>):back-forward-scroll.test.ts.scrollTo,scroll,scrollByandscrollIntoViewasserts the router makes no scroll call afterpopstate.["scrollTo([0,0])"]recorded afterpopstate, in all 4 runs (test-app + cloudflare-basic, dev + production, before the rebase).default-document-scroll+mini(both on the default document's<Html.ScrollRestoration>): 36 dev / 37 production passed.back-forward-scroll,navigation,location-state: 14 dev / 14 production passed.test:unit:all, lint, format,check:e2e-bucketing,check:e2e-parity --strict.No userland (
@rangojs/router/testing) test:renderRoutedoes not model scrolling or history traversal. The bfcache path is unit-only: Playwright doesn't give a reliable bfcache round trip.Notes
scrollTo) is inferred from the report and the desktop traces; it was not observed on a device.clientUrls()optimistic destinations, which replace the page before the navigation commits and can clampscrollY. The review showed that save could still be overwritten (server redirect, superseding click, reload), and that it saved too early for same-route and held navigations. It moves to its own PR, which will capture scroll at the actual DOM swap and hold it until the origin returns. Until then, back after a shorter optimistic destination behaves as it did before this PR.updateVersion(), a cross-tabclearHistoryCache(), or 20+ entries of history), the popstate commit saved the old page'sscrollYunder the destination's key, and itsreplacerebuilthistory.statewithout the scroll key or the entry's location state. fix(router): keep a back/forward entry's history state and save the page left under its own scroll key #943 also saves the page being left on every back/forward."auto"entries to the browser also covers async popstate renders (cache miss, view transitions), where the browser's restore may run before the page it restores has rendered. Desktop engines retried until the page was tall enough in the traces behind this PR. Before this PR the router forced the top in that case, which isn't a better answer.Html.ScrollRestoration). The test-app e2e case is dropped: since feat(router): the default document restores scroll on back/forward #948 the default document renders<Html.ScrollRestoration>, so test-app can no longer run without it (the case would assert the opposite of the app's setup). cloudflare-basic, whose own document doesn't render it, keeps dev + production coverage. With the default document on, this PR's changed rows apply to custom documents without<Html.ScrollRestoration>and to entries created before it mounts./simplify: shared the back/forward e2e body through@shared/e2e. Skipped hoisting the bridge test file's per-describemakeTx()factories, which is pre-existing code outside this diff.