fix: back-button restore survives late layout growth - #1313
Conversation
A snapshot's scrollY is recorded against the page at its settled height. The restore replays that number onto a document that has only just been swapped in and is still shorter, because the components in the restored markup have not upgraded and re-rendered yet. When they do, content grows above the viewport, and the browser's scroll anchoring holds the visual position by adding that growth to scrollY. The recorded offset is counted twice, so the reader lands below where they left: 763px on /ui/button, exactly the settled-minus-swapped height delta. Suppress anchoring for the duration of the restore instead of re-asserting the scroll afterwards. The number being replayed already accounts for the growth, so withholding the browser's correction fixes the double count at its source. Suppression never moves the viewport, so it cannot yank a reader who has started scrolling, and it needs no settle detection, which a re-assert would (and which cannot be answered without fighting a streaming <webjs-suspense> boundary). The window closes on the first real input, on that restore's own revalidation settling plus two frames, or on a 2s ceiling.
The block was moved to /docs/routing because /ui/button reproducibly restored to 1563 instead of 800, and that was recorded as unrelated live website behaviour. It was this bug. A docs page never grows after its swap, so asserting there could not see the defect at all. It also waits long enough now for the page to finish growing and revalidating. The old 80ms landed before the growth, so the restore was correct at 80ms and wrong at 1200ms, which is exactly the failure.
The case needs real history entries so it can drive a real popstate, and it built them from location.pathname. The test page's own query string identifies the web-test-runner session, so dropping it rewrote the page out of its session: every test still passed and the whole run exited 1 with no failure to point at, which reads as an infrastructure blip rather than a test problem. Build the entries from the live url instead, and put the exact original url back in teardown.
|
Design rationale: why suppress anchoring rather than re-assert the scroll The instinct on a "back lands too low" bug is to re-scroll once the page settles, and that direction is wrong here for three separate reasons, so writing them down. The position is already wrong 65ms after the swap, and the revalidation does not settle until roughly 300ms. A re-assert would leave the reader watching the wrong position for a quarter second and then jump them. Worse, one re-assert is not even a fix, because anchoring keeps acting afterwards: the revalidation's own swap moves scrollY 1563 to 1239 and back, so a re-assert landing between those two events gets undone. Making it correct means re-asserting on every height change, and a settling restore cannot be distinguished from a Suppression has none of that. It withholds a browser correction rather than performing an action, so it structurally cannot move the viewport, and there is no "has the page finished growing" question to answer. Deferring the restore until a settle signal is what Remix v3 does, by handing the traverse case to the Navigation API and letting the UA's own Worth noting none of the four routers I read handles this. Turbo, both Next routers, Remix v2, and Astro all scroll exactly once, synchronously after the swap, and let the position drift if content grows. |
|
Context: the e2e was measuring the wrong tree, which is why it looked like site behaviour Worth recording, because it is the reason #1305 wrote this bug off as unrelated live website behaviour and moved the assertion to A worktree set up with Concretely: with the fix committed and This does not affect CI, which builds from the branch. It does mean a browser-facing core change cannot be trusted from a linked worktree unless the assertion imports the source relatively. The browser test here does ( |
|
Open question: the iOS back-swipe and the #1310 asked for this to be checked on a real device and reported here either way. I do not have one, so this is analysis plus what I could verify in WebKit, and it is the one item on this PR that is unverified rather than verified. The concern is that an interactive back-swipe fires I think it does not, and the reason is ordering. The window opens on What I did verify: Per the issue, I am not dropping the |
…early The window's close was scheduled off the revalidation settling, which made its length network latency plus two frames. That is only long enough while the revalidation is slower than the restored page's own upgrade and render. It is on a deployed site, where growth lands ~65ms after the swap and the revalidation's swap ~300ms after that, but that ordering is a property of one deployment: a local server, a 304, or a warm cache answers in single-digit milliseconds and closes the window before the growth it exists to absorb, restoring the bug in full. Close on the later of the revalidation and a floor instead. A real user input still closes it immediately, which is the case that actually matters for not holding anchoring off longer than a reader wants. The browser suite now covers the inverted ordering directly: an instant revalidation with content that grows several frames later. That case reproduces at 1563 against the previous commit on all three engines.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole diff fresh. The mechanism holds up and I confirmed it independently on all three engines, but one finding is real and it is the interesting one, so writing up what changed.
The suppression window was closing on the revalidation settling, which made its length network latency rather than the length of the growth it guards. That is fine on the deployed site, where the revalidation is comfortably the slower of the two, but it is a property of one deployment and not a guarantee. I built the inverted case (a revalidation answering instantly, content growing several frames later) and it reproduces #1310 in full at 1563 on Chromium, Firefox and WebKit. So the fix as first written would have been correct against production and wrong against a local server or a 304. The window now closes on the later of the revalidation and a floor, and that case is in the browser suite.
The other finding is about coverage rather than code, and it is the one worth a second opinion: the e2e that exercises this against a real growing page lives in a file CI does not run.
Suppressing the browser's scroll anchoring is only safe if it is given back, and the suite asserted that by checking an inline property was gone. That would miss a release that cleared the property while leaving anchoring broken some other way, and it said nothing about the paths the fix is not supposed to touch. Three cases: anchoring holds the reader's position again once the window closes, judged by behaviour rather than by the property; a forward navigation opens no window at all; and a revalidation that never answers still releases on the ceiling rather than leaving anchoring off for the life of the page. Breaking the release reds all three plus the two that already covered it.
Suppressing anchoring unconditionally introduced this bug's mirror image. A document that has not grown yet can be too short to scroll to the recorded offset at all, so the browser clamps to its current maximum. There the shortfall is exactly the growth still to come, and anchoring adding that growth is what carries the reader back down. Suppressing froze the clamp instead. Measured on /ui/button: a reader who left at the page bottom, 2002, restored to 2002 before this fix series and to 1239 after it, stranded a full 763px page-growth ABOVE where they left. The same error as the bug, pointing the other way, and deterministic rather than intermittent. Suppress only when the recorded offset was actually reached. The two situations want opposite things and are told apart by the one question that separates them: did the scroll land. Reading it back is safe because nothing can grow between the write and the read. An offset the un-grown page cannot reach still lands wherever anchoring carries it rather than on the exact number, which is unchanged behaviour rather than anything this series introduces.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Second read, scoped to the floor commit and its blast radius. It found a genuine regression that the first round could not have seen, because the floor introduced it.
Suppressing anchoring unconditionally produces this bug's mirror image. When the restored page has not grown enough to scroll to the recorded offset at all, the browser clamps, and the shortfall is exactly the growth still to come. Anchoring adding that growth is what carries the reader back down, so freezing it strands them ABOVE where they left by the same 763px. Measured on /ui/button: a reader leaving at the bottom (2002) restored to 2002 on main and to 1239 with the floor in place. Deterministic, not intermittent, and neither existing test could reach it because both sit mid-page.
Suppression is now conditional on the scroll having actually landed, which is the one question that separates the two situations. The clamped path installs nothing and behaves exactly as it did before this PR.
Also fixed a real flake risk in my own test: an assertion that had to be shorter than the floor was counting animation frames, and the runner puts test files in concurrent pages where a non-visible page has rAF throttled.
The window outlives its own restore on purpose, so a navigation starting inside that span used to inherit it. A second Back that CLAMPS opens no window of its own, so it ran its whole growth under the previous restore's suppression and froze its clamp; a forward navigation carried the suppression onto an unrelated page. Every navigation now closes an open window first and reopens only if it earns one. Also records why the clamp probe must stay synchronous, which cost a round trip to learn. Deferring it even by a microtask breaks the fix: the restored components' renders have been applied by then, and reading scrollY forces the layout that flushes them, so anchoring runs during the read and hands back the already-shifted offset. Measured on /ui/button, the suppression landed 19ms late with scrollY already 800 to 1563. What makes the synchronous read correct is not which document it sees but that it sees the same layout the scroll just landed in.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Third read, scoped to the clamp commit. Four findings, two of them real bugs, and one that sent me somewhere useful by being wrong about the remedy.
The real one: making suppression conditional meant a clamped restore stopped closing a window a PREVIOUS restore had left open, because the close lived inside the suppress call. The window deliberately outlives its own restore, so a second Back inside that span ran its whole growth under the old suppression and froze its clamp, and a forward nav carried it onto another page. Every navigation now closes an open window first and reopens only if it earns one.
The instructive one: the probe reads the document synchronously right after the swap, and under a view transition the swap is deferred a frame, so in principle it measures the outgoing page. I moved the read behind the documented _swapCommit seam, and it broke the fix outright on the live site: 400/800/1200 went straight back to 734/1563/1963. Reading scrollY off the synchronous path forces the layout that flushes the restored components renders, so anchoring runs DURING the read and hands back the already-shifted offset. Instrumented, the suppression landed 19ms late with scrollY already carried to 1563. What makes the synchronous read correct is not which document it sees, it is that it sees the same layout the scroll just landed in. Reverted, and the reasoning is now a comment so nobody makes the same move again.
Also chased the suggestion to move the property off the root, since a root mutation is the #610 flash mechanism. Suppressing on <body> works identically on all three engines, so it looked strictly better, but the RELEASE does not: on WebKit anchoring never resumes once suppressed there. Removing the property, setting it back to auto, and both in sequence were each measured and none brought it back. A placement that cannot be undone would leave every iOS reader with anchoring off for the life of the page, so the root stays and the reasoning is recorded.
|
Findings I did not act on, and why Two from this round that did not become changes, recorded so the reasoning is not lost. Moving the property off the root. A root mutation is exactly the #610 mechanism (it re-runs global style resolution, and on WebKit re-resolves The view-transition ordering. The clamp probe reads synchronously right after the swap, and Also worth knowing for anyone testing this file. A hidden document skips a real view transition, and the runner puts test files in concurrent pages, so the deferred swap path is not reachable from the browser suite at all. |
Three review findings, none of them behavioural. The comment on the release chain claimed the suppression decision was asynchronous, which was true only of a version that got reverted. The decision is synchronous, so the wrapper lambda it justified bought nothing, and the claim also contradicted the comment twenty lines above saying the read must stay synchronous. Dropped both. The docs site enumerates every close condition for the window, so it needed the new one too; only the skill reference had it. The nav-closes-window behaviour had a browser test but nothing at the unit layer, where deleting either call site left the suite green.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fourth read, scoped to the nav-close commit. Nothing behavioural this time, three real housekeeping defects, all fixed.
The one worth naming: a comment on the release chain claimed the suppression decision was asynchronous. That was true of a version I reverted, and stale comments about async ordering are exactly the kind that send the next person down a wrong path, especially since it contradicted the comment twenty lines above saying the read must STAY synchronous. The lambda it justified bought nothing either, so both are gone.
The other two are surface gaps: the docs page enumerates every close condition for the window and was missing the new one, and the nav-close had a browser test but nothing at the unit layer, where deleting either call site left the suite green. Both closed, and the unit test reds when the close is removed.
The nav-close landed at two call sites but only one was covered, so deleting the one in performSubmission left every suite green. The gap was not just a missing case: a form appended to the test's container never reaches the router at all, because the restore swaps the body wholesale and leaves that container detached, and an action pointing at the page's own url is skipped as a non-HTML extension since the runner serves test files from a .js path. Both are now spelled out where the next person will hit them. The docs sentence also dropped the half that matters. It said a navigation closes the window and stopped there, which reads as a second Back getting no suppression at all, the opposite of what the router does.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fifth read, scoped to the housekeeping commit. Two findings, both real, both mine, both fixed.
The first is the better catch: my own commit message claimed the nav-close had been uncovered at EITHER call site, and then I only covered one. Deleting the close in performSubmission still left the unit, browser and e2e suites green. Covering it turned up two things worth knowing about this file, now written down in it: a form appended to the test container never reaches the router at all, because the restore swaps the body wholesale and leaves that container detached, and an action pointing at the page own url is skipped as a non-HTML extension, since the runner serves test files from a .js path. Both call sites now red independently.
The second: the docs sentence I added to sync the two surfaces dropped the half that carries the meaning. It said a navigation closes the window and stopped, which reads as a second Back getting no suppression at all, the opposite of what the router does.
The comment added with the submission case explained the form action with a mechanism that does not exist. The runner serves the test page at `/`, not at a `.js` path, and `.js` is not in the router's non-HTML extension list anyway, so an action pointing at the page's own url would have reached performSubmission fine. Verified by using that url, which passes on all three engines, and it is the more realistic fixture since a bound form posts to its own page. The only real cause was the one the neighbouring comment already gave: the restore swaps the body wholesale, so a form inside the test container is detached before it can be submitted. The form also outlived a failing assertion, since its removal sat after the assert rather than in the finally that releases everything else.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Sixth read, scoped to the coverage commit. Three findings, all mine, all fixed, and the first is the one I am glad someone caught.
I had written an explanation into the test for why the form action could not point at the page own url: that the runner serves test files at a .js path and the router skips non-HTML extensions. Both halves are false. The runner serves the page at /, and .js is not in NON_HTML_EXTENSIONS at all, which form-action-submit.test.js already proves by submitting an action-less form on the same page. I verified it directly: the page own url works on all three engines, and it is the better fixture anyway since a bound form posts to its own page. The only real cause was the one the neighbouring comment already gave, that the restore swaps the body wholesale and leaves the test container detached. A wrong explanation in a test is worse than none, because the next person trusts it.
The other two: the form outlived a failing assertion, since its removal sat after the assert instead of in the finally that releases everything else in this file, and the PR body test plan had gone stale against its own branch.
The clamped path was left to scroll anchoring, which carries a reader back down as the page grows. That is right only at the very bottom, where the shortfall and the growth are the same number. Anchoring adds the FULL growth however far short the clamp fell, so everyone above the bottom overshot: leaving at 1902 on /ui/button came back at 2002, and the whole 1240 to 2002 band landed at the bottom regardless of where it started. Re-assert the recorded offset once the document can hold it. #1310 rejected re-asserting the scroll in the general case and that reasoning still holds; the difference here is that this knows exactly where it is going and can tell when it has arrived. It runs only on the clamped path, only while the offset is out of reach, writes once, and stops on the same inputs that close a suppression window, so it cannot fight a reader who has taken over. There is no settling-versus-streaming question to answer, which is what sank the general version. The whole range is now exact: 0, 400, 800, 1200, 1500, 1800, 1902 and 2002 all restore to themselves. Also wires the e2e that covers this into CI. It ran against the website rather than the blog, so it was outside the e2e job entirely, which meant the one assertion exercising a Back restore on a real growing page never ran on a PR.
The two catch-up cases were flaky on Firefox, failing their clamped precondition about one run in three: the restored page was already tall enough to reach the offset, so there was no clamp to chase. Both attempts to control that from the outside failed. Removing the leftover fixture in setup as well as teardown did not fix it, because a revalidation swap can land after teardown has run, and keying the grower per run made it worse. The precondition does not need to be a race at all. A grower that never grows on its own is 0px whatever else happened, so the test asserts the clamp and only then adds the height, which is the moment the catch-up is waiting for. Six consecutive Firefox runs clean, and the full browser suite is green on all three engines. The self-growing fixtures stay where late growth arriving by itself is the thing under test.
|
Deferred findings closed rather than filed Both of the out-of-scope items this PR had been carrying are now fixed here instead of becoming follow-ups. The e2e was outside CI. The clamped band was imprecise. Leaving that path to anchoring only works at the very bottom, where the shortfall and the growth are the same number; everywhere else anchoring adds the full growth regardless, so the whole 1240 to 2002 range landed at the bottom whatever the reader's real position. The router now chases the recorded offset there, re-asserting it once the document can hold it. The whole range is exact: 0, 400, 800, 1200, 1500, 1800, 1902 and 2002 all restore to themselves. That second one deliberately reopens something #1310 settled against, so it is worth being explicit about why it is admissible. The issue rejected re-asserting the scroll because a settling restore cannot be told apart from a One item stays open and cannot be closed here: the iOS back-swipe check needs a real device. Worth recording about the tests. The two new clamped cases were flaky on Firefox, about one run in three, and the flake was instructive. Their precondition needs the restored page to be SHORT at the moment of the restore, and that is not something the test can control from outside: removing the leftover fixture in setup as well as teardown did not fix it, because a revalidation swap can land after teardown has run, and keying the grower per run made it worse. The fix was to stop racing at all. A grower that never grows on its own is 0px whatever else happened, so the test asserts the clamp and only then adds the height. Six consecutive Firefox runs clean, and the full browser suite is green on all three engines. |
Three review findings, one of them a real defect on a real app shape. Under a view transition applySwap defers its DOM mutation a frame, so the restore wrote and measured the scroll against the OUTGOING page. That was not reachable from the earlier fixture, whose snapshot head was empty: the full-body restore merges the incoming head BEFORE deciding whether to run a transition, so the opt-in was stripped and the transition never engaged. A real snapshot carries it, since it is serialized from the live document. With the meta in the snapshot head it reproduces: a 60000px outgoing page made the scroll "land" at 20000, suppression opened, and the restored page then clamped to 2416 with anchoring held off, which is the stranding the clamped path exists to prevent. The decision now waits for the swap commit on that path only; the synchronous path is untouched, since deferring it there breaks the fix outright. The catch-up is bounded by the floor rather than the ceiling. Any growth past the target fires it, and growth is not exclusively the restore settling, so a two second window could scroll a reader who had landed and started reading, generating no input to cancel it. Its docstring no longer claims to escape the settling-versus-streaming question; the window is what makes it safe, not an ability to tell the two apart. The CI step no longer stalls or leaks. Left on the step's stdout, the background server held the log pipe open and outlived the script: run locally the identical script hung for ten minutes after the tests had passed. Redirecting to a file fixes that, and setsid plus a process-group kill stops the watcher child holding :5001, which a plain kill did not. Both paths measured: pass exits 0 in 14s, failure exits 1, and the port is free afterwards either way.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Seventh read, over the catch-up, the CI step and the fixture changes. Three findings, all real, all fixed, and the first one changes my mind about something I had written off two rounds ago.
I had looked at the view-transition ordering before and could not get a restore to take the deferred path, with a real engine or a stub, so I left it as defensive. The reason it never engaged is that the full-body restore merges the incoming head BEFORE it decides whether to run a transition, and my fixture snapshot had an empty head, so the opt-in was stripped every time. A real snapshot carries it, because it is serialized from the live document. With the meta where it actually lives it reproduces immediately: a tall outgoing page makes the scroll land, suppression opens, and the restored page then clamps with anchoring held off. That is the stranding the clamped path exists to prevent, arriving from the other side. The decision now waits for the swap commit on that path only, since deferring it on the synchronous path breaks the fix outright.
The catch-up now lives only as long as the floor rather than the ceiling. It fires on any growth past the target, and growth is not exclusively the restore settling, so a two second window could scroll a reader who had landed and started reading and so generates no input to cancel it. I also removed the claim that it escapes the settling-versus-streaming question, because it does not: the window is what makes it safe.
The CI step stalled and leaked, which I reproduced before fixing. Left on the step stdout the background server holds the log pipe open and outlives the script; the identical script hung for ten minutes locally after the tests had already passed. It redirects to a file now, and runs under setsid with a process-group kill, because a plain kill left the watcher child holding the port. Pass exits 0 in 14s, failure exits 1, port free either way.
Four review findings, one a real defect in the view-transition path added last commit. That path is the only place the restore outlives the call that scheduled it, and every cancel site in this feature runs at the START of the next thing. So a navigation, submission, or disableClientRouter arriving inside the deferred frame closed the window and then had the stale restore reopen it, keyed to the previous history entry, scrolling a page it was never meant for. It is token-guarded now, the same mechanism the rest of the file uses; the synchronous branch cannot outlive anything and needs none. The catch-up's window was a live behaviour constant with nothing pinning it: both existing cases grow the fixture immediately, so they passed at any bound. A case now drives growth AFTER the window with no input at any point, which is the reason the bound exists. Both doc surfaces named user input as the only thing that stops the chase and never mentioned the time box, so a component settling later than it would strand a clamped reader with nothing saying so. The source comment was wrong in the other direction, claiming the chase covers the same span as the suppression beside it; it is deliberately the shorter of the two, because it writes scroll. The CI step sent the server log to a file and never read it back, on a required check that boots the website and runs its Tailwind build. It now dumps the log on any failure and preserves the exit code exactly.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Eighth read, over the view-transition ordering, the catch-up bound and the CI step. Four findings, one of them a defect the previous commit introduced.
The view-transition path is the only place the restore outlives the call that scheduled it, and every cancel site in this feature runs at the start of the next thing. So a navigation, submission or disableClientRouter landing inside the deferred frame closed the window and then had the stale restore reopen it, keyed to the previous history entry, on the page that had just replaced it. Token-guarded now, the same mechanism the rest of the file uses, and there is a case for it: start a Back under a transition, start a navigation before the swap commits, assert the superseded restore does not reopen anything.
The catch-up bound was a live constant with nothing pinning it. Both existing cases grow the fixture immediately, so they passed at 500ms, at 2000ms, or with the timer deleted. The new case grows AFTER the window with no input at any point, which is precisely the reader the bound exists to protect.
Both doc surfaces said user input was the only thing that stops the chase and never mentioned the time box, so a component settling later than it would strand a clamped reader with nothing saying so. My own source comment was wrong the other way, claiming the chase covers the same span as the suppression beside it; it is deliberately the shorter of the two, because it writes scroll. Both corrected.
The CI step sent the server log to a file and never read it back, on a required check that boots the website and runs its Tailwind build, so a boot or build failure would have failed the check with the cause discarded. It dumps the log on any failure now, and I verified the exit code survives exactly: a test exiting 3 still exits 3, with the log printed and the port free.
The guard added last commit used currentNavigationToken, which is the obvious choice and the wrong one. loadFrame bumps that token too, and its own contract says a frame self-load is NOT a page navigation. An eager <webjs-frame src> inside a restored snapshot loads as part of the swap, so the guard could read a routine frame load as a supersede and drop the whole restore, leaving the reader at the outgoing page's offset. That is worse than the defect this change exists to fix. A counter that moves only for the three things which really do end a restore replaces it. Being straight about the test: the new case puts a self-loading frame in the restored page and asserts the restore still runs, and the frame does load, but it does NOT red under the token version. The bump lands after the guard reads in this environment, so the race resolves the safe way and the substitution is invisible to it. The case is worth keeping as a regression guard on the invariant; the reason for the change is the semantic one, that a frame load is not a navigation, not a red test. Removing the guard outright still reds the superseding-navigation case.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Ninth read, over the token guard added the round before. One real finding, and it is a good catch against my own fix.
I guarded the deferred restore with currentNavigationToken, which is the obvious choice and the wrong one. loadFrame bumps that token too, and its own contract says in as many words that a frame self-load is not a page navigation. An eager inside a restored snapshot loads as part of the swap, so the guard could read a routine frame load as a supersede and drop the entire restore, leaving the reader at the outgoing page offset. That is worse than the bug this whole change fixes. It is keyed to a counter that moves only for the three things which really do end a restore now.
Worth being straight about the test, because it does not do what a counterfactual should. The new case puts a self-loading frame in the restored page and asserts the restore still runs, and I confirmed the frame really does load, but it does NOT red under the token version: the bump lands after the guard reads here, so the race resolves the safe way and the substitution is invisible to it. I am keeping the case as a regression guard on the invariant, but the reason for the change is the semantic one rather than a red test, and it should be read that way. Removing the guard outright still reds the superseding-navigation case.
The other finding was that none of the recent commits have run CI. That is accurate and it is the blocker on this PR, but it is not something the branch can fix: no workflow run has been created for ANY branch in this repo since 18:25, and two other branches have runs that queued at 18:04 and never started. I cancelled the stale run on this branch to free its slot and closed and reopened the PR to fire a fresh pull_request event; neither produced a run. The workflow has no workflow_dispatch trigger, so there is no lever left that does not depend on Actions resuming.
The token to counter change shipped with nothing that reds when reverted, which I said at the time. There was a way to build it after all: every other case supersedes with navigate(), which moves BOTH counters, so none of them can tell the implementations apart. A case that moves only the nav token, which is exactly what a frame self-load does, separates them: under the old keying the restore is dropped and the reader is left at the outgoing offset, measured as 0 against an expected 800. The frame case asserted nothing about the frame, so it could not tell "the frame loaded and the restore survived" from "the frame never loaded", which is the failure mode its own commit message flagged. It counts the self-load now, like every other precondition in the file. The counter's rule also did not match the code. It claimed to move only for things that end a restore, while a frame-TARGETED nav or submission bumped it unconditionally, though the codebase already declares those equivalent to the src self-load the rule exempts. They swap one region and leave the page, so they are excluded too, and the comment is now true.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Tenth read, over the supersede counter. Three findings, and the first one corrected me on something I had already written off.
I had said the token to counter change could not be given a test that reds when reverted, and left it resting on the semantic argument. That was wrong, and the way to build it was sitting there: every existing case supersedes with navigate(), which moves BOTH counters, so none of them can separate the two implementations. A case that moves only the nav token, which is precisely what a frame self-load does, separates them cleanly. Under the old keying the restore is dropped and the reader is left at the outgoing offset: expected 800, got 0. So the change now has the counterfactual I said it could not have.
The frame case asserted nothing about the frame, so it could not distinguish the frame loading and the restore surviving from the frame never loading at all, which is the exact failure mode I had flagged as environment-dependent one commit earlier. It counts the self-load now, the way every other precondition in that file is pinned.
The counter also did not implement its own stated rule. It claimed to move only for things that genuinely end a restore, while a frame-TARGETED navigation or submission bumped it unconditionally, even though this codebase already declares those equivalent to the src self-load the rule exempts. Exempting one half of a pair and keeping the other is the split the rule exists to prevent. Both are excluded now and the comment is true as written.
The exemption landed on one of three sibling operations. The counter was guarded by !frameId while releaseScrollAnchor and cancelScrollCatchUp on the next two lines still ran unconditionally, so a frame-targeted nav or submission still closed the suppression window and aborted the catch-up. That is the same split the comment claims to avoid, moved one line down, and it brings the full double-count back. It needs no user input either: a component upgrading in the just-restored page can navigate or submit a frame on its own, and resolveTargetFrameId picks up the enclosing frame. All three move together now, with a case that clicks a link inside a frame while a window is open. Reverting to the split form reds it. Two test-quality fixes alongside. The bare-token case and the frame case both depend on the swap actually being deferred, and neither said so, so either would have passed vacuously if the simulated transition stopped engaging. They assert it now, the way the sibling case already did.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Eleventh read, over the frame exemption. Three findings, all mine, and the first is a real bug in the fix from the round before.
I exempted a frame-targeted navigation from ONE of three sibling operations. The counter was guarded, and the two lines under it, closing the suppression window and aborting the catch-up, still ran unconditionally. That is the same split the comment I wrote claims to avoid, moved one line down, and it brings the full double-count back. It needs no user input to reach: a component upgrading in the just-restored page can navigate or submit a frame on its own, and the frame id resolves from the enclosing element. All three move together now, and clicking a link inside a frame while a window is open reds against the split form.
The other two are about evidence rather than behaviour, and they are the second time this cycle has caught me shipping a test that could pass for the wrong reason. Both the bare-token case and the frame case depend on the swap actually being deferred, and neither said so, so either would have gone quietly vacuous if the simulated transition stopped engaging. They assert it now, which the sibling case had been doing all along.
Worth noting what the last few rounds have been finding: the defects are getting smaller but they are still real, and two of the last four were about whether a test proves what it claims rather than about the code. That is a reasonable shape for a cycle to be converging in, but it is not clean yet, so it continues.
The frame exemption contradicted both doc surfaces, which still said any navigation ends the window and named a click as the example. A click inside a frame is now precisely the case they got wrong, and both files were already open in this PR, so it was drift this change introduced rather than a pre-existing gap. The frame-nav case asserted only that the window stayed open, which is also true when the click never reaches the router at all, so it caught just one of the two failure directions. It counts the frame navigation now, using the same stubbed-fetch idiom the file already had. The third view-transition case had no precondition either. On the synchronous path the restore has already run and the navigation simply closes its window, so it would pass green while exercising none of the deferred supersede guard, which is the mechanism the frame carve-out reasons about and the one case least able to afford a quiet pass.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Twelfth read, over the frame exemption. First round where the code change itself came back sound: the three cancels moving as one unit matches disableClientRouter, which does all three, and loadFrame, which does none, and it leaves the deferred guard contract intact. All three findings were in the blast radius rather than the mechanism.
The exemption contradicted both doc surfaces, which still said any navigation ends the window and offered a click as the example. A click inside a frame is now exactly the case they got wrong, and both files were already open in this PR, so that was drift this change introduced rather than a gap it inherited.
The other two are the same lesson twice more. The frame-nav case asserted only that the window stayed open, which is equally true when the click never reached the router, so it caught one of the two failure directions; it counts the frame navigation now. And the third view-transition case had no precondition, so on the synchronous path the restore would have already run and the navigation would simply close its window, passing green while exercising none of the deferred supersede guard. That is the guard the frame carve-out reasons about, so it was the one case least able to afford a quiet pass.
Three rounds running, the findings have been about whether the evidence proves what it claims rather than about behaviour. That is where I would expect a cycle to end up, and it is the first sign here of actual convergence rather than just smaller defects.
The sentence added last commit enumerated the exempt cases and got the enumeration wrong in both directions. A click inside a frame carrying data-webjs-frame="_top" breaks out and IS a page navigation, as is one inside a frame with no id, while an external trigger naming a frame id is exempt without being "inside" anything. The source comments said "frame-targeted" and were right; the prose over-specified. It now defers to the same rule that decides frame targeting everywhere else, and names the two cases that are page navigations. The carve-out also claimed a frame-targeted submission as exempt with nothing exercising it: the page-level case uses a bare form, the frame case uses a link, so deleting frameId from performSubmission's guard left every suite green. There is a case for it now, and removing that guard reds it. Two more surfaces carried the old universal claim, both introduced by this PR: the unit test's comment and the PR body. Also moved the docs-site sentence, which had been inserted between "Absent both" and the two things "both" refers to.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Thirteenth read. Five findings, and the headline one is that the doc fix I shipped last round was itself wrong.
I enumerated the exempt cases in prose and got the enumeration wrong in both directions. A click inside a frame carrying data-webjs-frame="_top" breaks out and IS a page navigation, and so is one inside a frame with no id; meanwhile an external trigger naming a frame id is exempt without being "inside" anything. The source comments said "frame-targeted" and were correct. The lesson is narrow and worth keeping: the prose was wrong because it was more specific than the code, and the fix is to defer to the same rule that decides frame targeting everywhere else rather than restate it.
The carve-out also claimed a frame-targeted submission as exempt with nothing exercising it. The page-level case uses a bare form and the frame case uses a link, so deleting frameId from performSubmission left every suite green. There is a case for it now and removing that guard reds it.
Two more surfaces still carried the old universal claim, both introduced by this PR: the unit test comment and the PR body. And the docs-site sentence had been inserted between "Absent both" and the two things "both" refers to, so it broke a referent while fixing a fact.
On the shape of this: the last two rounds found nothing wrong with the mechanism, and everything wrong with how it was described and evidenced. That is a real change in kind from the earlier rounds, but I have now shipped two consecutive doc fixes that each needed fixing, so I am not going to claim the trend means much until a round comes back with nothing.
c3f8433 to
ed59998
Compare
…ctly The chase stopped on reachability: it wrote the recorded offset on the first frame the document could hold it, then tore itself down. Anchoring is deliberately left on for the clamped path, so every later stage of growth was added on top of that write and carried the reader back below the offset. Real growth arrives in stages, since its cause is components upgrading one at a time, while every fixture here grew in one assignment, so nothing caught it. A two-stage fixture that lands stage one exactly on the reachability threshold reproduces it: an offset of 4000 ends at 5000. Once the reader is ON the recorded offset the situation is identical to a restore that landed first time, so it now gets that case's protection for what remains, bounded by the same floor and closing on the same inputs. The cost of the bound was also documented backwards in three places. The bound stops the ROUTER writing scroll; it does not stop the browser. Anchoring stays on, so growth after the window is still added and the reader drifts BELOW the offset, which is main's behaviour, rather than sitting at the clamp as all three surfaces claimed.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Final pass over the whole diff, read as a finished change rather than a sequence of fixes. The core mechanism held up, and it found one behavioural defect that every earlier round had missed for the same reason.
The chase stopped on REACHABILITY. It wrote the recorded offset on the first frame the document could hold it and then tore itself down, and since anchoring is deliberately left on for the clamped path, every later stage of growth was added on top of that write and carried the reader back below the offset. Real growth arrives in stages, because its cause is components upgrading one at a time. Every fixture in this PR grew in a single assignment, and the measured table in the body is single-stage too, so the whole suite was blind to it in exactly the shape the real page is not. A two-stage fixture that lands stage one precisely on the reachability threshold reproduces it: an offset of 4000 ends at 5000.
Once the reader is ON the recorded offset the situation is identical to a restore that landed first time, so it gets that case protection for what remains, bounded by the same floor and closing on the same inputs. Removing that reds the new case.
The second finding is the cost of the bound being documented backwards in three places, including a paragraph I had written two rounds earlier specifically to make it precise. The bound stops the ROUTER writing scroll; it does not stop the browser. Anchoring stays on, so growth after the window is still added and the reader drifts BELOW the offset, which is main behaviour, rather than sitting at the clamp. The PR own test comment had the truth in it the whole time.
1 participant
Closes #1310
Pressing Back landed the reader roughly 763px too far down on a page whose content settles taller after the swap. The router was not restoring the wrong number, it was restoring the right number too early:
cached.scrollYis recorded at the page's settled height, the swapped-in DOM is still shorter until its components upgrade and render, and the browser's scroll anchoring then adds that late growth to the offset the router just replayed. The offset is counted twice.The fix suppresses scroll anchoring for the duration of the restore rather than re-asserting the scroll afterwards. The number being replayed already accounts for the growth, so withholding the browser's correction fixes the double count at its source. It never MOVES the viewport, so it structurally cannot yank a reader who has started scrolling, and it needs no settle detection, which a re-assert would and which cannot be answered without fighting a streaming
<webjs-suspense>boundary.Four conditions on that window came out of review, and each one is a defect that was found and fixed rather than a precaution:
The read that decides must be synchronous. Deferring it even by a microtask breaks the fix outright: the restored components' renders have been applied by then, and reading
scrollYforces the layout that flushes them, so anchoring runs during the read and returns the already-shifted offset. Measured, the suppression landed 19ms late withscrollYalready carried 800 to 1563. What makes the synchronous read correct is not which document it sees but that it sees the same layout the scroll just landed in.Suppression applies only when the recorded offset was actually reached. A page that has not grown yet can be too short to scroll that far, so the browser clamps. There the shortfall IS the growth still to come and anchoring adding it is what carries the reader back down, so suppressing would freeze the clamp and strand them a full page-growth ABOVE where they left. Measured: leaving at the bottom of
/ui/button(2002) restored to 2002 on main and to 1239 with unconditional suppression.The window is floored, not just ceilinged. Scheduling its close off the revalidation alone ties its length to network latency rather than to the growth it guards, so a server answering faster than the page renders closes it early and restores the bug in full. Reproduces at 1563 on all three engines.
A new PAGE navigation ends an open window. The window outlives its own restore by design, so without this a second Back that clamps would run its whole growth under the previous restore's suppression, and a forward nav would carry it onto an unrelated page. A FRAME-targeted navigation or submission is exempt on the same rule that decides frame targeting everywhere else: it swaps one region and leaves the restored offset meaningful, so closing there would hand anchoring back mid-restore and bring the double count straight back.
Scope is the popstate cache-hit branch. Every other scroll path lands at offset 0 (nothing above the viewport for anchoring to compensate) or targets an element rather than replaying a recorded number.
Measured on the local website
Exact at every offset, with no
overflow-anchorresidue on<html>after any of them. The right column is the clamped band, which on main lands at the bottom regardless of where it started.Test plan
packages/core/test/routing/router-client.test.js: the window opens on the restore, an instant revalidation does not close it alone, the floor does, a second navigation closes it, anddisableClientRouter()closes an open onepackages/core/test/routing/browser/nav-scroll-anchor-restore.test.js: 20 cases, green on Chromium, Firefox and WebKit. The restore opens a window; late growth does not push the reader down; a revalidation answering before the growth still holds; the window closes once the restore is over with no residue; a reader taking over closes it immediately; anchoring WORKS again once it has closed, asserted by behaviour rather than by the property; a forward nav opens no window; the ceiling releases a revalidation that never answers; a second navigation closes an open window, and a page-level form submission does too; a clamped restore is left alone rather than frozen, and is chased to the exact offset; the chase gives up after its window and does not move a settled reader, and a reader taking over cancels it; under a view transition the decision waits for the swap to commit, a navigation during that deferred window cancels the restore, a bare nav-token bump does not, and a frame self-load in the restored page does not; a frame-targeted navigation and a frame-targeted submission both leave an open window aloneexpected ~800, got 1563; breaking the release reds all five release cases; unconditional suppression reds the clamp case; removing the nav close reds at both the unit and browser layers, and each of its two call sites reds independentlytest/e2e/form-submission-and-race.test.mjsrepointed at/ui/button: 6/6/,/docs/client-router,/ui,/ui/button,/ui/cardin dist mode with no broken preloadswebjs checkclean;webjs doctorexits 0 forwebsiteandexamples/blogrouter-client.jsis browser-only client code and touches no runtime-sensitive surface. The matrix was run anyway and is greenThe clamped band
An offset the un-grown page cannot reach is a second, opposite problem, and leaving it to anchoring only solves it at the very bottom, where the shortfall and the growth happen to be the same number. Everywhere else anchoring adds the full growth regardless, so the whole 1240 to 2002 band landed at the bottom whatever the reader's actual position.
The router now CHASES the recorded offset there, re-asserting it once the document can hold it. #1310 rejected re-asserting the scroll in the general case and that reasoning still holds; what makes this admissible is that it knows exactly where it is going and can tell when it has arrived. It runs only on the clamped path, only while the offset is out of reach, writes once, and stops on the same inputs that close a suppression window, so it cannot fight a reader who has taken over. There is no settling-versus-streaming question to answer, which is what sank the general version.
Docs
.agents/skills/webjs/references/client-router-and-streaming.md, agent-facing guidance on the restore, every close condition, and what an app must not do around it.agents/skills/webjs/references/muscle-memory-gotchas.md, a new entry for the Remix<ScrollRestoration>/ NextuseEffect+scrollToreflexwebsite/app/docs/client-router/page.tsNot verified
The iOS back-swipe check #1310 asks for needs a real device, which I do not have. Analysis and what I could confirm in WebKit are in a comment below, along with the reasoning for two review findings that were investigated and deliberately not acted on.