fix: leave scroll restoration to the browser so iOS previews the back-swipe - #1430
Merged
Conversation
vivek7405
commented
Aug 18, 2026
vivek7405
left a comment
Collaborator
Author
There was a problem hiding this comment.
Went through the whole diff against the router's own invariants. The shape holds up: the levers are inert unless an app writes window.__webjsDiag, the yield is tied to recordHistoryNow being non-null so no background path picks up a frame of latency, and putting the yield in the caller rather than at the four swap.js commit points is the right call given applySwap is synchronous and its return value is consumed synchronously. Making it async to host a diagnostic would be a bigger change than the thing being diagnosed.
One real problem, on the ?scrolllast path, commented inline.
Two things I checked and am satisfied with. The early return after the yield sits inside the try whose finally calls clearFrameBusy, so it cannot leak a busy frame, and it matches the supersede guard directly above it. And the claim in the comment that a discard can never see a non-null thunk holds: revalidating: true is passed at exactly one call site, navigator.js:583, which passes recordHistory: false.
Worth restating what this does NOT do, because it is the part that is easy to misread from the diff: it fixes nothing. The levers exist so the device can tell us whether the snapshot is bound at the push or at a later composited frame, and until that run happens we do not know which. That is why the body references #1428 without a Closes.
7 tasks
#1410 moved the history push ahead of the DOM mutation and merged with its iOS acceptance criterion openly unmet, because the gesture preview exists only on a real iPhone. The blank survived there, so the assumption behind that fix is still untested: that WebKit binds the back-forward snapshot synchronously at the pushState call. If it instead captures the compositing surface when the didSameDocumentNavigation IPC lands in the UI process, that happens after the whole push-swap-scroll task, and reordering inside the task changes nothing the device can see. Rather than guess again, ship the two candidate timings behind default-off levers and let the device choose, which is the method that finally isolated #610. ?raf and ?raf2 hand WebKit one or two frames to paint the outgoing page between the push and the swap; ?scrolllast defers the scroll-to-top past the frame to isolate the clamp from the swap. The yield sits in fetchAndApply rather than at the four commit points inside applySwap, because applySwap is synchronous and cannot await. The thunk is one-shot, so firing it in the caller covers whichever commit point the swap reaches and leaves that call a no-op, which is the same ordering at every one of them rather than at a chosen few.
The ?scrolllast lever defers the scroll-to-top by a frame, which puts it outside the navigation's own task. A newer navigation can start in that frame, and the deferred callback would then scroll ITS page: worst on the hash branch, where scrollIntoView hunts the old URL's anchor in the new document and lands somewhere arbitrary if that id happens to exist. The synchronous path cannot do this, so the lever was adding a failure mode rather than isolating one, and a diagnostic that exists to measure scroll behaviour must not write scroll into a page it has nothing to do with.
The supersede assertion drove its second navigation with a fetch that never settled, which left a navigation in flight for the rest of the page's life holding the router's token and its own frame state. Under the full browser suite that leak reded an unrelated file, the #1310 back-restore residue assertion, on Firefox, while both files passed in isolation and while the branch's own file passed everywhere. Rejecting with an AbortError settles the navigation down the path the router already takes for a superseded one, so the assertion observes the same thing with nothing left running. Full browser suite green twice at this commit, against a baseline that was green before the file was added.
An on-device prior-art run reordered what this A/B should test. Three frameworks, same iPhone, same gesture: WebJs sets scrollRestoration to 'manual' and blanks the preview, Turbo Drive sets 'manual' and blanks it too, and Next's App Router leaves 'auto' and is clean. That kills the assumption the existing levers were built around. #1410 shipped Turbo's exact ordering, and Turbo has the symptom, so ordering was never going to be sufficient. Push timing in fact runs the other way across the three: both frameworks that push ahead of the swap blank, and the one that pushes after does not. Turbo also wraps its whole commit in a requestAnimationFrame and blanks anyway, which is most of the prior probability the two rAF levers had. scrollRestoration is the only property that tracks the symptom. Under 'auto' WebKit records a scroll position per history entry and can compose the gesture preview from it; under 'manual' the app owns that and the browser records nothing, leaving the preview no scroll state to render against. That also explains why the symptom follows the offset at navigation time and why the back button is fine. The lever is a measurement, not a candidate fix: leaving 'auto' lets the browser restore alongside the router's own restore, and reconciling the two is the design work a positive result buys (#1310 / #1313 guard it). Re #1428.
vivek7405
force-pushed
the
fix/ios-back-swipe-snapshot-ab
branch
from
August 20, 2026 07:37
cf5519c to
1613eb1
Compare
The comment said a newer navigation pushes its entry over the superseded one. pushState appends rather than overwrites, so a superseded ?raf navigation leaves an entry for a url that was never rendered and Back lands on it. Left in place deliberately (undoing it races the navigation that just superseded this one, and moving the push later defeats the lever), but a diagnostic measuring back-navigation must not misdescribe what it does to the back stack. A superseded cell should be discarded and re-run.
…swipe The router set history.scrollRestoration = 'manual' on boot (the Turbo Drive assumeControlOfScrollRestoration pattern) to stop the browser's own popstate restore racing the snapshot restore. That mode is also what stops the browser RECORDING a scroll position per history entry, and the recorded offset is what WebKit composes the edge back-swipe gesture preview from, so every scrolled page previewed BLANK for the whole gesture. Measured on a real iPhone across frameworks: WebJs and Turbo Drive both set 'manual' and both blank; Next's App Router leaves 'auto' and is clean; and this app flips from blank to correct on that one property. Push ORDERING, which #1410 changed, runs the other way (both frameworks pushing ahead of the swap blank, the one pushing after does not), so it was never the mechanism. The attribute is left alone and the race is settled where it happens instead. The UA's write lands a frame after the popstate handler, inside the restore window that is already open, so that window now writes back an off-target programmatic displacement for its duration. The write-back cannot fight a reader. Every window release event is a CAPTURE-phase input listener, and an input event precedes the scroll it causes, so a user-driven scroll always arrives with the window already closed; what remains is exactly a programmatic write inside the restore's own span. Both properties are asserted, and removing the write-back reds tests on all three engines. Two #1310 assertions now read at frame granularity rather than task granularity. The contract is the offset the reader sees by the next paint: the stale UA write and its correction land in the same task, so a task-granularity read could catch a transient between two writes that was never user-visible. Re #1428.
Every surface that described the router as taking manual control of scroll restoration, or that explained the blank iOS back-swipe preview as a consequence of push ordering, said something now measured false. The contract surfaces (the skill's client-router and muscle-memory references, and the docs site) now state what the router actually does: it leaves history.scrollRestoration at auto so the browser keeps recording per-entry offsets, restores from its own snapshot, and absorbs the browser's own late restore inside the restore window. Each also tells an app not to set 'manual' itself, since doing so re-breaks the gesture preview app-wide, and that line is the first thing most ported scroll-restoration recipes do. The mechanism claim from #1406 is corrected in place rather than deleted, in constants.js, swap.js, fetch-apply.js and the #1410 browser guard. All four asserted that WebKit binds a same-document entry's gesture snapshot at the moment the entry is recorded. Turbo Drive uses that same ordering and previews blank identically on a real iPhone, and the preview was fixed by the scrollRestoration change instead. The ordering is kept on its own merits (an entry should be recorded against the page it belongs to), and each site now says so, including what the guard does NOT prove. The blog post transcribing the router contract carried the same claim on a live page, and gains the Turbo divergence: WebJs borrowed this router from Turbo, inherited its manual-scroll pattern, and this is the first place the two part company. The published changelog for core 0.7.51 repeats the old claim and is left alone: it is a historical record of what that release believed. Re #1428.
The device has answered, so the instrumentation comes out and the branch is left carrying only the fix. Gone: diagFlag and diagFrameYield from diagnostics.js, the ?raf / ?raf2 frame-yield block and the ?scrolllast deferral in fetch-apply.js, the query-string capture in the website's root layout, the lever browser suite, and the lever unit tests. The forward-nav scroll block returns to its original inline shape, since the thunk existed only to give the lever a second call site. The levers cost two device rounds and never moved the symptom, which was the right outcome to get cheaply: they tested paint timing, and the cause was that the router had taken manual scroll restoration away from the browser. Verified after removal: 233 unit, 215 routing browser tests on Chromium, Firefox and WebKit, and the full browser suite green on all three (76 files). The whole node suite is 4461/4468, the 6 failures being the documented linked-worktree baseline (elision differentials, both Bun listener tests, one asset() prod-handler assertion), none of them client router. Re #1428.
vivek7405
marked this pull request as ready for review
August 20, 2026 12:03
The write-back added for the back-swipe fix lives in the #1310 restore window, and a frame-targeted navigation is the ONE navigation that deliberately leaves that window open. So the two features meet, and nothing covered the meeting point: the existing frame case asserts only that the window survives, never where the reader ends up. It matters because a click-driven frame nav reaches fetchAndApply with recordHistory: true (unlike loadFrame, which passes false) and the scroll block carries no frameId guard, so it runs the forward-nav scroll-to-top even though it swaps a single region. Inside an open restore that would drop the reader to the top of the page they just came back to. It does not, because the write-back corrects it. Green on Chromium, Firefox and WebKit; disabling the write-back reds this case specifically, so it is not passing vacuously. Re #1428.
…fset A Back restore re-inserts an outerHTML snapshot, and that markup is shorter than the page it came from until its components upgrade and render. Every scroll defect this path has had lived in that window: the recorded offset was unreachable, the browser clamped to whatever the short document allowed, and the router healed it afterwards by CHASING the offset once the page grew tall enough to hold it. The snapshot now records the settled scrollHeight alongside the offset, and the restore reserves that height on the root element across the swap. The offset is reachable on the first frame, so the restore lands exactly, once. The clamp cannot occur, so there is nothing to chase. Removed: the clamped branch, catchUpToRestoredScroll and cancelScrollCatchUp with their supersede wiring, and the now-unused ANCHOR_SUPPRESS_FLOOR_MS import. scroll.js drops from 279 to ~200 lines and suppression becomes unconditional. Kept, because the reservation does not subsume them: the anchoring window (content still SHIFTS above the viewport within a constant total height, and anchoring adds that shift to the offset just replayed) and the window's write-back. Two things the migration surfaced, both worth knowing: The restore needs an explicit layout flush before its write. The reservation and the swap both just changed layout, and Chromium and WebKit clamp a scroll against the stale layout and land at 0 while Firefox flushes on its own. The old conditional-suppression code got that flush by accident, since deciding clamped-or-landed read scrollY right after the write. Unconditional suppression removes the accident, so the read is now deliberate. The suite's fixture was building snapshots WITHOUT scrollHeight, which left the reservation inert and every assertion passing for the wrong reason. Fixed, and the fixture now models the real invariant that a recorded offset is always reachable within its own recorded height. The 8 clamp/chase cases are replaced by 4 reservation outcomes: the offset is reachable on the first frame with no clamp and no chase, the reservation leaves no residue, a second navigation releases it, and a deferred view-transition restore still lands on the offset. 212 routing browser tests green on Chromium, Firefox and WebKit; 233 unit. Re #1428.
Every case in this suite injects the recorded offset into the snapshot cache while the page sits at 0, so the browser has only ever recorded 0 for that entry. That is fine while the router owns the restore, but it makes the UA's own restoration unobservable, and the UA is a live participant now that scrollRestoration is left at auto. The new `uaRecords` fixture option scrolls before pushing the next entry, so the browser records a real offset, and the new case asserts the reader still lands on it through a SHORT snapshot swap. This is also the gate for the single-writer question, and it answers it NEGATIVELY. Disabling the router's own write reds 8 cases per engine on all three, so the browser cannot carry the restore alone even with the height reserved. Their code says why. Next restores from a cached tree (segmentCacheMap / bfcache in restore-reducer) and React RECONCILES, so the document is never torn down under the UA's restore. Turbo caches live DOM clones (cloneNode(true)) and separately owns scroll outright via manual. Remix 3 tolerates destruction by deferring the UA restore past the swap with event.intercept(). WebJs's replace tier destroys and rebuilds the range by design (a changed route-key is a remount, Next parity), and intercept() is the one mechanism measured to break the iOS gesture preview, so the router keeps the restore. The guard stays because it is what would catch this changing: if the swap ever becomes non-destructive, disabling the router write stops reding and the single-writer design reopens. Re #1428.
The clamp/chase paragraphs were the longest in both the skill reference and the docs site, and they described machinery that no longer exists. Both now describe the reservation instead: the snapshot records the page's settled height, the restore holds it across the swap so the offset is always reachable, and the anchoring window still covers the other half (content shifting within a constant height). Both state the release rules, including why user input deliberately does NOT release the reservation even though it closes the window. Also corrects the last two copies of the #1406 mechanism claim, missed in the earlier sweep: the docs site's how-it-works list and the unit suite's section docstring. Both asserted that WebKit binds the gesture snapshot when the entry is recorded, which #1428 measured false. The ordering is kept and each site now says why it is correct anyway. The published changelog for core 0.7.51 keeps the old claim, being a record of what that release believed. Re #1428.
This was referenced Aug 20, 2026
main landed #1429, which excludes frame-scoped responses from the forward-nav scroll block outright. That removes the stray scroll this guard was written against: a click-driven frame nav no longer runs the page scroll-to-top at all, so it can no longer disturb an open restore by that route. The case is kept, because it asserts the OUTCOME (a frame swap must never move a restore in progress) rather than the mechanism, and the outcome has to hold however the internals move. It is now defended twice, by #1429's guard and by the restore window. Only the comment changes; the assertions are untouched and still pass. Re #1428.
One writer instead of two. The router no longer replays the snapshot's offset on a popstate restore; under scrollRestoration 'auto' the browser replays the offset IT recorded, against a document the reservation holds at its recorded height, so the UA's write is simply correct and the router's was redundant. This is Next's and Remix 3's model. Neither scrolls on a traverse (Next's restore reducer sets scrollRef: null and its scroll handler bails; Remix 3 gates its only scrollTo on isNewEntry). Turbo is single-writer too but the other way round, taking 'manual' and replaying itself, which is exactly the choice that costs it the iOS gesture preview and is the bug this PR started from. An earlier gate concluded the opposite and it was WRONG. Disabling the router's write reds 8 cases, so I read that as the browser being unable to carry the restore. It was the fixture: 22 of 23 cases injected the offset into the snapshot cache while the page sat at 0, so the browser had recorded 0 for those entries and had nothing real to replay. The fixture now scrolls before pushing the next entry, which is what a reader does, and with a realistic recording all 23 pass with the router's write removed, on all three engines. The whole routing suite passes too. What stays, each re-gated by counterfactual under the new design rather than assumed: - the height reservation, without which 4 cases red per engine (it is what makes the UA's replay land on a document that can hold the offset); - the anchoring window, without which 11 to 12 cases red per engine (content still SHIFTS above the viewport within a constant height as the restored components render, and anchoring would add that shift to the offset the UA just replayed); - the window's write-back, which now guards the restore's span against a programmatic intruder rather than against the UA. The #601 restore assertion is re-pointed rather than deleted: it asserted the router's instant scrollTo form, and the router no longer writes. That guarantee survives and strengthens, since native scroll restoration is not a scrolling API call and `scroll-behavior: smooth` cannot animate it. The forward-nav half of #601 is a separate write and keeps its own assertions. Re #1428.
The scroll-restoration docs described the two-writer arrangement that the previous commit deleted: the browser replaying a frame after the router, with the restore window absorbing the difference. There is one writer now, and it is the browser. All three surfaces that describe the mechanism now say so, and each keeps the part authors actually need: do not set scrollRestoration to 'manual' yourself, because that suppresses the per-entry recording the iOS gesture preview is composed from. Turbo is named precisely rather than loosely. It is single-writer too, but its writer is the APP, and that is exactly the choice that costs it the blank preview, so "single writer" alone does not distinguish the two designs. Next and Remix 3 are the browser-owned precedent WebJs now matches. AGENTS.md needs no change: it says only that scroll is restored on back/forward, which was and remains true, and never described the mechanism. Re #1428.
CI caught a real ordering difference this test had stopped describing. It was written against the two-writer design, where the router replayed the offset SYNCHRONOUSLY inside the popstate handler, so "input on the next frame" was unambiguously after the restore. The browser owns the restore now and its replay arrives a frame or so later, so a reader modelled as input-on-the-next-frame can outrun the restore itself. The scroll then lands before the replay does, the replay overwrites it, and the case reads as the router fighting a reader when nothing of the sort happened. It now polls for the restore to land before interrupting, rather than waiting a fixed number of frames, so the case does not encode one engine's replay latency. That is also the honest statement of the property: a reader can only take over once the page has actually come back, which is exactly what the on-device cell exercises. Reproduced only on CI's Chromium and never across six local full-suite runs, which is why it survived to CI. Green three times on all three engines after the fix. Note for anyone reading CI on this branch: the Bun matrix job is ALSO red, and it is red on origin/main at 5268785 too, with the same stack overflow in server-side form-action attribute serialization. It is unrelated to this PR and predates it. Re #1428.
Bun 1.4.0 regressed Array.prototype.join's cycle guard, which ECMA-262 requires. Six lines, no framework involved: const a = []; a.push(a); String(a) node 26 "" bun 1.3.14 "" bun 1.4.0 RangeError: Maximum call stack size exceeded CI installs bun-version: latest, so it got 1.4.0 and this assertion, which exists to pin that a self-referential array renders rather than overflowing, started failing. The same job is red on origin/main at 5268785 for the same reason, so it is neither new nor caused by this branch. Scoped rather than worked around. The alternative is a cycle-safe stringify on the per-attribute SSR hot path, which is real cost carried forever for someone else's bug, and the case is only reachable by deliberately building a self-referential array, so nothing an app does hits it. The skip is keyed to the BEHAVIOUR rather than to a version, so the assertion stays live on every spec-compliant engine and returns on Bun automatically once the regression is fixed. Verified on node 26, bun 1.3.14, and bun 1.4.0.
Thirteen findings from the review round. The two that were defects: The router now writes history.scrollRestoration = 'auto' EXPLICITLY on enable rather than relying on it being the default. The restore has no writer of its own any more, so an app that had set 'manual' (the first line of most ported scroll-restoration recipes, and what this router did until #1428) would have got NO Back restore at all: the UA replays nothing, the reservation prevents the clamp that would fire a scroll event, and the write-back is scroll-event-driven so it never runs. Prose in the docs cannot prevent that; the line can. And the suite's fixture options were dead. `injectOffset` was read but never passed, `uaRecords` was passed but never read, so the case the PR presented as the single-writer proof was configured identically to the default and proved nothing extra. The option is deleted and the case now states what it actually covers. The write-back is now armed for 250ms rather than for the restore window's whole life. It reconciles ONE event, the UA's replay landing about a frame after the popstate handler, and standing guard for up to two seconds would also revert legitimate programmatic scrolls in that span: a component's scrollIntoView() during upgrade, an autofocus on a below-fold control, or find-in-page from the browser chrome, which fires no page keydown and so does not close the window the way a key press would. Bounded by time rather than by a correction count, and that distinction was earned: a single-shot bound (the first thing tried) can be spent on an unrelated scroll event arriving before the UA's replay, leaving the stale replay uncorrected. It reproduced on Firefox about one run in three. Also removed a test added earlier in this round that modelled a snapshot offset diverging from the UA's recording. That divergence cannot occur (the router snapshots the offset at the same navigation the UA records it), and the case only passed when an incidental scroll event happened to fire, so it was flaky by construction. The deterministic write-back case covers the mechanism. The rest were comments and docs that outlived the code they described: the fetch-apply branch comments asserting a router write that is gone and a UA ordering that is backwards; the write-back's rationale citing the removed Navigation API interception; the teardown comment naming the deleted catch-up; the cache-miss claim that the UA landing its offset is "strictly better than top" (it is not guaranteed to be, and the trade is now stated); a leftover A/B-lever section header; two adjacent skill bullets giving opposite answers about who restores scroll; and a docs claim that the back/forward restore is forced behavior:'instant' by the router, which no longer writes it. Plus the missing JSDoc on suppressScrollAnchoring's two new params and a Snapshot typedef violation in the legacy-string branch. Verified: 220 routing browser tests green on Chromium, Firefox and WebKit across four consecutive runs; 233 unit; disabling the write-back still reds 5 cases, so it remains load-bearing. Re #1428.
The first pass at this fixed only test/bun/form-action-guard.mjs, which is the file CI happened to name, and did not grep for the same assertion elsewhere. Two more test files build a self-referential array and assert the render survives it, and both are in the Bun matrix: packages/core/test/rendering/form-action-attr-guard.test.js packages/core/test/rendering/form-action-attr-guard-client.test.js Same cause: Bun 1.4.0 regressed Array.prototype.join's cycle guard, so `String(a)` throws RangeError for `const a = []; a.push(a)`. Node and Bun 1.3.14 both return ''. Same treatment: keyed to the behaviour rather than to a version, so each returns automatically once the engine is fixed. Verified by running the WHOLE matrix the way CI does, on 1.4.0 rather than on the 1.3.14 that could not see the bug: BUN=/tmp/bun140/bin/bun node scripts/run-bun-tests.js -> 331 pass, 2 fail Both remaining failures are the documented linked-worktree artifacts (the asset() ?v= case and test/bun/listener.test.mjs), which fail identically at origin/main in this worktree and pass in CI. The lesson, since it cost a round trip: a CI job reports the first file that fails, not every file with the defect. Fixing what the log names and pushing is how a two-instance bug becomes two red builds.
…inating Round two of review, and its two real findings were both defects the round-one fixes introduced. The router forces scrollRestoration to 'auto' (the restore is the browser's now, and 'manual' means no restore at all), but it did so WITHOUT saving what the app had. So `disableClientRouter()`, the documented runtime opt-out, left an app that had its own popstate restoration stranded on 'auto' forever, double-restoring with no way to detect why. The value is saved at enable and put back on disable, the same contract the anchoring window and the height reservation keep for the inline styles they touch. And the test guarding that write was vacuous: its mock started at 'auto', so the assertion passed whether the router wrote 'auto' or wrote nothing. It even carried a title asserting the opposite of the implementation. Seeded with 'manual' now, and the counterfactual confirms it: deleting the write reds both halves. Cache-miss popstate is deterministic again. The handler scrolls to top, but the browser replays its own recorded offset a frame later, measured against the OUTGOING document and landing before the fetched content arrives, and `fetchAndApply` skips its scroll block on `recordHistory: false`, so nothing corrected it. The fallback is now re-asserted after the response commits, guarded on still being the active navigation. Under the old 'manual' mode this path was deterministic; that property is restored rather than traded away. Two smaller mechanism fixes. The height reservation now supersedes a held one BEFORE its own height guard, so a restore with no recorded height (a legacy string snapshot) cannot leave the previous page's min-height pinned until the ceiling. And the reservation is released two frames AFTER the anchoring window rather than in the same tick, because the revalidation's swap re-inserts short markup and dropping the height alongside anchoring can clamp the reader down and then anchor the regrowth on top. The write-back comment claimed a protection it does not provide. A component's scrollIntoView() or an autofocus during upgrade runs inside the 250ms arming span and IS reverted. That is the deliberate precedence on a Back (the reader asked for the page they left), but the comment asserted the opposite, so it now states the real trade and what the bound actually buys. Rest were drift: comments still naming the deleted catch-up, a test comment citing the removed Navigation API interception, and two doc surfaces saying the router "leaves scrollRestoration at its default" when it now writes it and overrides the app. Verified: 220 routing browser tests green on three engines across three consecutive runs; full browser suite green; 233 unit; 4461/4468 node (six known worktree artifacts); e2e nested-layout 2/2 and form-submission-and-race 8/8; webjs check clean on all three apps; and the Bun matrix on 1.4.0 at 331 pass with only the two documented worktree artifacts. Re #1428.
Inline review of the previous commit found its determinism fix incomplete. The re-assert ran synchronously after fetchAndApply resolved, which wins only when the fetch is slower than the UA's replay. A popstate cache-miss CAN consume a prefetched entry (GET, no body, no refresh, no noPrefetch), and a warmed entry resolves the whole fetch-and-apply inside the popstate task, so the re-assert landed in that same task and the UA's replay a frame later overwrote it, exactly the ordering the fix claimed to correct. Deferred two frames instead, which is past the replay on every tested engine whichever path resolved the fetch, with the active-navigation guard moved INSIDE the deferred callback so a superseded miss never scrolls the page that replaced it. Coverage note, stated rather than hidden: no browser test drives the cache-miss-plus-prefetch-hit combination, so this ordering is covered by the reasoning above and the suites' absence of regression, not by a dedicated case. Building that fixture needs a warmed prefetch keyed to a back-entry URL and was judged not worth a new rig in this PR. Re #1428.
This was referenced Aug 20, 2026
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
1 participant
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.
Closes #1428.
The iOS edge back-swipe previewed a blank page for the whole gesture on any
scrolled page. Confirmed fixed on a real iPhone, plain URL, no lever.
Cause
The router set
history.scrollRestoration = 'manual'on boot. That mode alsostops the browser RECORDING a scroll position per history entry, and the
recorded offset is what WebKit composes the gesture preview from, so every
scrolled page previewed blank.
Measured across frameworks on the device:
scrollRestorationmanualmanualmanualautoautoOne property explains all five rows. Push ORDERING runs the other way: both
frameworks pushing ahead of the swap blank, and the one pushing after does not.
#1410 was not the fix and could not have been. It shipped Turbo's exact
ordering, and Turbo blanks. The ordering is defensible on its own merits and is
NOT reverted here, but its stated mechanism was wrong, and this PR corrects that
claim everywhere it is written down.
Lineage: the
manualcall arrived with the client router itself, which wasborrowed from Turbo and then enhanced (
231cd8f8citesturbo/src/core/drive/history.js). WebJs inherited Turbo's blind spot alongwith its architecture, which is why
turbo.hotwired.devpreviews blank the sameway when tested identically. This is the first divergence from Turbo on that
path.
The fix
The attribute is left alone. The race
manualexisted to silence is settledwhere it happens instead: the UA's own restore lands a frame AFTER the popstate
handler, inside the restore window that is already open, so that window now
writes back an off-target programmatic displacement for its duration.
It cannot fight a reader. Every window release event is a CAPTURE-phase input
listener, and an input event precedes the scroll it causes, so a user-driven
scroll always arrives with the window already closed. What remains is exactly a
programmatic write inside the restore's own span. Both properties are asserted,
and removing the write-back reds tests on all three engines.
Two #1310 assertions now read at frame granularity rather than task
granularity. The contract is the offset the reader sees by the next paint; the
stale UA write and its correction land in the same task, so a task-granularity
read could catch a transient between two writes that was never user-visible.
A measured dead end, recorded so nobody re-treads it
A Navigation API rearchitecture (
navigation.addEventListener('navigate')+event.intercept(), the Remix 3 model) was built, tested green on all threeengines, and removed.
intercept()does defer the UA's scroll restorationon Chromium and WebKit (and not on Firefox), but on the device the interception
BROKE the very preview being fixed. It is not in this branch. #1433 is corrected
accordingly.
Two void device rounds, and the trap behind them
Two rounds reported "still blank" against code that was never running. The rig
serves the core bundle at a content-hashed URL whose hash is memoized at server
boot (
packages/server/src/asset-hash.js,_hashCache, cleared only by the devfs.watchrebuild; the rig runswebjs start), and the response isCache-Control: max-age=31536000, immutable. Rebuildingdistunder the runningserver changed the bytes while the HTML kept advertising the old
?v=, so thephone answered from its own year-long cache and ran the FIRST lever build
throughout.
Verification had been grepping the bundle on the server's disk, which is the
wrong cache layer: it proves what a NEW client would get and says nothing about
this device. The check that settles it is to restart the server, read the
?v=the HTML now advertises, and fetch that exact URL. Every device observation is
consistent with the stale build, so the original
?scrollautopositive wasnever a false positive.
The architecture, beyond the fix
The fix removed
manual. Two further changes finish the scroll model, and therouter ends up SMALLER than it started.
The clamp class is gone. A restore re-inserts an outerHTML snapshot, and that
markup is shorter than the page it came from until its components upgrade, so the
recorded offset used to be unreachable, the browser clamped, and the router
healed it afterwards by CHASING the offset as the page grew. The snapshot now
records the settled
scrollHeightand the restore reserves that height acrossthe swap, so the offset is reachable on the first frame.
One writer, and it is the browser. The router no longer writes scroll on a
traverse at all. With the height reserved, the UA's own per-entry replay lands
correctly, so a second writer of the same quantity was redundant. This is Next's
model (its restore reducer sets
scrollRef: nulland its scroll handler bails onrestores) and Remix 3's (its only
scrollTois gated onisNewEntry). Turbo issingle-writer too, but its writer is the APP, which is precisely the choice that
costs it the iOS gesture preview and is the bug this PR started from.
Writers on Back went 3 to 1. Deleted: the clamped branch,
catchUpToRestoredScroll,cancelScrollCatchUp, and the router's restore write.An earlier gate said this was impossible, and it was wrong. Disabling the
router's write red 8 cases, which I read as the browser being unable to carry the
restore. It was the FIXTURE: 22 of 23 cases injected the offset into the snapshot
cache while the page sat at scroll 0, so the browser had recorded 0 for those
entries and had nothing real to replay. The one case with a genuine browser
recording passed under that same gate. With the fixture scrolling before pushing
the next entry, the way a reader does, all 23 pass with the router's write
removed on all three engines, and so does the whole routing suite.
What survives, each re-gated by counterfactual under the new design rather than
assumed:
One deliberate omission. A forward navigation scrolls to top, which matches
all three frameworks, but each offers a per-link opt-out for the "back to list"
case (
<Link scroll={false}>,rmx-reset-scroll="false",refreshScroll === "preserve") and WebJs has none. Filed as #1436, not folded in.Test plan
linked-worktree baseline (three differential-elision assertions, both Bun
listener tests, one
asset()prod-handler assertion), none client router.engines.
nested-layout-partial-swap2/2 (the sidebarscrollTopand DOMidentity case),
form-submission-and-race8/8 including the dogfood: back-button scroll restores ~763px too low on pages that grow after swap #1310 back-buttonrestore and the fix: record the history entry before the swap, not after #1410 sibling, blog e2e 96/96. The
#1406e2e that was red inthis worktree earlier was a stale-dist artifact and is green with
distrebuilt here.
webjs checkclean ingallery/,examples/blog/andwebsite/.serializer, SSR dispatch, stream or
node:*surface.(WebKit), desktop (Chromium and Firefox), and Android Chrome (Blink, mobile),
four cells each: back-swipe preview on a scrolled
/blog;/ui/buttonBack-button landing accuracy (the page dogfood: back-button scroll restores ~763px too low on pages that grow after swap #1310 was measured on, where the original
defect was landing 763px too low); nested-layout sidebar scroll across a swap;
and a reader scrolling DURING the restore keeping their own position. All pass.
Firefox failures that four subsequent clean runs (~3,600 executions) could not
reproduce or name. Both fell in the window right after a counterfactual
patch-and-restore cycle, so contamination is the likely cause. Recorded rather
than dropped.
Docs
Every surface that named the mechanism is corrected, which is the #1410
doc-gate lesson applied:
.agents/skills/webjs/references/client-router-and-streaming.mdandmuscle-memory-gotchas.md: the router no longer claims to be the sole scrollauthority, and both now tell an app NOT to set
manualitself, since that isthe first line of most ported scroll-restoration recipes and re-breaks the
preview app-wide.
website/app/docs/client-router/page.ts: the same contract, plus why thewrite-back cannot fight a reader.
constants.js,swap.js,fetch-apply.jsand the fix: record the history entry before the swap, not after #1410 browser guard: thefalse "WebKit binds the gesture snapshot when the entry is recorded" claim is
corrected in place, including what that guard does NOT prove.
blog/client-router-turbo-drive-style.md: a live page was publishing the sameclaim; it now carries the Turbo divergence instead.
changelog/core/0.7.51.mdrepeats the old claim and is leftalone, being a historical record of what that release believed.
Closing #1428
The acceptance criteria are met and the verdict is recorded on the issue, so
merging closes it.
The device matrix is complete against the shipped architecture: iPhone Safari
(WebKit), desktop (Chromium and Firefox), and Android Chrome (Blink, mobile),
four cells each. The reference was held at a plain
#1428while the on-deviceresult was still outstanding, which is no longer the case.
The issue remains the home for the measurement record (the framework
comparison, the two void rounds and the immutable-cache trap behind them, the
Navigation API dead end, and the corrected single-writer conclusion). Closing it
does not remove any of that; the record is in the comments.