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 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.
…fully The suppression installed on landing started a fresh floor-length timer, so the clamped path could hold anchoring off for nearly twice the floor measured from the restore, and stopped being the tighter of the two windows, which is the whole point of the bound. It rides the chase's own deadline now, so the total is the floor from the restore either way. The doc correction in the previous commit described the behaviour that commit had just replaced: it said anchoring stays on for this path and late growth is added, while the same commit made landing suppress. All three surfaces now say what the code does, which is that anchoring is left on only while the offset is out of reach, and the chase protects it from the moment it lands. Nothing observed the landed window either, so deleting it left everything green but the staged-growth case. There is a case for its open and its close now, and removing the suppression reds both.
The deadline change shipped without a counterfactual, and the case added with it could not provide one: it landed early and observed late, which both forms satisfy, since both suppress at landing and are shut once the page has settled. Landing LATE is what separates them. A landing at ~380ms leaves roughly 120ms of the restore's window, where a fresh floor started at landing would run to ~880ms, so observing in between distinguishes the two. Reverting to the fresh timer reds it. The commit that made the change also claimed "removing the suppression reds both" of its assertions. Only the open assertion reds; the close one expects an empty value and passes trivially when no window was installed. All three surfaces still called this window shorter than the suppression one, two sentences before saying both end together on the same bound. The change is what made that false: the landed suppression IS this window now. They say so.
It grew at 380ms against a 500ms deadline, leaving about 115ms for the tick that had to land it. Miss that and the chase has already stopped, no scroll is written, and the precondition fails. It is the only case in the file with no slack, which is the shape that produced the one-in-three Firefox flake the file already records. Growing at 250ms leaves about 250ms for that tick, and the sample sits 120ms past the restore deadline and 135ms short of where a fresh floor started at landing would expire, so both directions keep margin. Five consecutive Firefox runs clean, and reverting the source to the fresh timer still reds it.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Last round. Two findings, one of them a real liability in a test I had just added.
The deadline case grew at 380ms against a 500ms deadline, so about 115ms was all the margin the tick that lands the chase had. Miss it and the chase has already stopped, nothing is written, and the precondition fails rather than the assertion. It was the only case in the file with no slack, in a file that already records a one-in-three Firefox flake from exactly that shape. Growing at 250ms leaves about 250ms for the tick, and the sample now sits 120ms past the restore deadline and 135ms short of where a fresh floor would expire, so both directions have room. Five consecutive Firefox runs clean, and reverting the source still reds it.
The second is a correction to my own commit message rather than to the code. I claimed all three surfaces carried a clause the change had falsified. Only the two prose surfaces did; the JSDoc named the non-clamped window explicitly and was true before and after, so its edit was a rewording, not a correction. Recording that here since the message is not rewritable.
Stopping the cycle here, and saying why rather than just stopping. This round found no defect in the mechanism, and neither did the two before it: what they found was my evidence not proving what I claimed, and then a margin in the test that proved it. The change itself has been stable and green for several rounds. Continuing now would be the cycle polishing its own artifacts rather than protecting the change, which is a bad trade for the time it costs.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Clean. Nothing left open.
This round was scoped to the implementation: does the shipped code behave wrongly anywhere reachable, and can any test actually observe the bug it exists for. Wording, comments and the PR body were explicitly out of scope, because the last several rounds had been spending themselves on those and a cycle that fixes its own prose does not terminate.
What it went after and could not break: the release and identity accounting, where every release is identity-guarded and the supersede-before-install ordering means no window can strand overflow-anchor on ; the landed window really riding the chase deadline from the restore rather than its own; the restore counter staying clear of loadFrame, so a frame self-load cannot cancel a restore; the view-transition predicate being read against the same post-head-merge state both inside applySwap and at the restore branch, so the synchronous and deferred choices cannot disagree; and each browser case discriminating against the bug it names rather than passing for an unrelated reason.
For the record on how this converged: across the cycle roughly a third of the findings were real implementation defects, the 763px double count itself, the clamp regression that stranded a bottom-of-page reader, the fast revalidation closing the window early, the navigation that failed to close one, the frame exemption applied to one of three cancels, and the staged growth drifting off the offset. The rest were my own prose and evidence, and each of those bought a full round. Narrowing the bar to the implementation is what made the cycle end.
…r-restore # Conflicts: # .github/workflows/ci.yml
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.
…-swipe (#1430) * chore: add guarded on-device levers to A/B the iOS back-swipe blank #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. * fix: guard the deferred back-swipe scroll on the navigation token 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. * test: abort the superseding navigation instead of leaving it in flight 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. * chore: add a scrollRestoration lever for the back-swipe A/B 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. * docs: correct the phantom-entry claim in the raf lever guard 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. * fix: stop taking manual scroll restoration, so iOS previews the back-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. * docs: correct the scroll-restoration contract and the #1406 mechanism 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. * chore: remove the back-swipe diagnostic levers 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. * test: pin that a frame nav cannot scroll a restore in progress 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. * refactor: reserve the restored height instead of chasing a clamped offset 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. * test: pin the restore against a browser-recorded offset 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. * docs: describe the height reservation, and drop the chase it replaced 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. * test: retarget the frame guard's rationale at #1429 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. * refactor: let the browser own the Back/Forward scroll restore 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. * docs: the browser owns the Back/Forward restore 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. * test: wait for the restore to land before the reader interrupts it 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. * test: scope the cyclic-array case to engines with a working join guard 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. * fix: apply the review findings, and bound the restore write-back 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. * test: scope the other two cyclic-array cases for Bun 1.4.0 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. * fix: put the app's scrollRestoration back, and make its guard discriminating 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. * fix: defer the cache-miss re-assert past the UA's replay 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.
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: 23 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 alone; a clamped restore survives growth arriving in stages; landing on the offset opens a window that closes on the chase deadline; and that window ends on the restore's deadline rather than one of its ownexpected ~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.