fix: a <webjs-frame> swap no longer scrolls the page to top - #1429
Merged
Conversation
vivek7405
commented
Aug 18, 2026
vivek7405
left a comment
Collaborator
Author
There was a problem hiding this comment.
Read this one cold, looking for the ways a guard like this over-applies.
The change itself is the right shape and the smallest one available: frameId was already a parameter of fetchAndApply, so no plumbing, and the rule it introduces is one scroll.js already states for the restore window. What I wanted to be sure of is that a non-null frameId can never end up applying a PAGE swap, because then skipping the scroll would strand a reader mid-document on a page they have never seen. It cannot: applySwap's frame branch either swaps the subtree or returns 'none' with a cancelable webjs:frame-missing, and it deliberately refuses to fall through to a full-body replace. The build-mismatch hard reload at the top of the same function is likewise gated on !frameId. So a frame nav has exactly two outcomes, and neither one wants a scroll.
The one thing I did fix is in my own fixture, and it is the second time this file bit in the same way. Worth stating plainly since it is a fixture-authoring trap the next person will hit: a page-swap response with an empty <head> makes the router's head merge strip every live head element the response does not carry, web-test-runner's own session scripts included, so the file passes while a later unrelated file fails to start a test page on Firefox. The fixture now echoes the live head back. Same class of problem in the setup ordering, flagged inline.
Two behaviour notes I made deliberately and would want challenged if anyone disagrees. The hash branch is skipped for frames too, so a #anchor on a frame link no longer scrolls, which matches Turbo and keeps this to one rule instead of two. And Turbo's autoscroll has no equivalent here; the docs now say so, since an unexplained absence reads as an oversight.
Collaborator
Author
Review round 2Verified the guard against the source rather than the description: 1. Docs contradiction (3954ab8). The frame-prefetch paragraph still said a frame-missing response scrolls to top. That path keeps a truthy 2. Overstated 3. Untested hash half (8402c8f). All five cases were hashless, so the guard's deliberate hash exclusion had no counterfactual. Added a sixth: an anchored frame link, with the anchor target placed above the spacer so scrolling to it is unmistakable, plus a fixture-integrity assertion that the two offsets cannot coincide. Counterfactual, on all three engines: narrowing the guard to Also corrected: the Verification: Ending the review cycle here: the round found no must-fix defect, so per the short-cycle rule these fixes do not buy another round. |
Collaborator
Author
Review round 3 (scoped to the three commits since round 2)The round cleared its three targeted questions: neither new assertion can pass vacuously (both were verified to red on their own), the frame-missing case leaks no scroll state (its One finding was a real prose defect, introduced by round 2 and now fixed in 3241860. The back-to-top claim was false for the most common spelling. Round 2's correction said a back-to-top anchor inside a frame "behaves exactly as it does anywhere else". True for Measured in a real browser against the gallery rather than argued from the source:
So a bare This is a behaviour change this PR introduces, narrow but real: before the guard, that click took the no-hash branch and scrolled to top. It is consistent with the stated rule (it IS a frame swap, so it must not move the window), so it is documented rather than special-cased. Flagged for the owner as a design call rather than decided here. Two smaller fixes in the same commit. The escapes sentence blurred the two "frame not found" cases: an id resolving to no live frame is a page nav and scrolls, while a RESPONSE missing the frame stayed frame-scoped and holds the offset, which is what round 2 just added a test for. A reader hitting the Verification: Ending the cycle here. The round found no correctness defect in the code, and a prose fix does not buy another round. |
vivek7405
marked this pull request as ready for review
August 20, 2026 12:32
The router's scroll block was gated on `recordHistory` alone, which means "a foreground navigation the reader initiated". A click on a frame-driving link is exactly that, since a frame click advances the URL deliberately, so a frame swap fell into the page-navigation scroll by omission rather than by decision. On gallery.webjs.dev/features/frames, clicking a filter tab from 400px threw the reader back to the top with the panel they had just clicked in off screen. A frame swaps ONE region and leaves the rest of the document standing, the scroll offset included, so the router now writes no scroll for it. That is the same rule `restoreGeneration` already applies when deciding what ends a scroll-restore window, so the two paths agree on what a frame nav is. Turbo, which <webjs-frame> is modelled on, likewise never scrolls on a frame navigation. The hash branch is excluded too: a #anchor on a frame link is no more a request to move the document viewport than the swap itself is. `_top` and an unresolvable data-webjs-frame id both resolve to a null frame id, so they stay page navigations and still scroll to top.
A page swap merges the incoming head and removes any live head element the response does not carry, so the empty <head> in the page-navigation fixture stripped web-test-runner's own session scripts out of the page. The file itself still passed, having already loaded; the damage showed up as unrelated later files failing to start a test page on Firefox, which reads as an infrastructure blip rather than a fixture bug.
Its last statement is the precondition assertion, so a failure there left the fetch stub and the nav guard installed for the rest of the RUN rather than the rest of the file. That damage is invisible where it happens and surfaces as unrelated later files misbehaving, which is the same shape the head handling in this fixture already produced once. Teardown now guards each step so it can clean up a partially-installed setup.
The frame-prefetch paragraph still told readers that a frame-missing response scrolls the page to top. That path keeps a truthy frameId, so the new guard skips the scroll while history still advances: the URL changes and the reader keeps both their panel and their place. Left as-is the docs page contradicted its own new section 110 lines above. The "#hash holds too" claim was broader than the code. A pure fragment link (same path, same query) never reaches the router at all, since the click handler returns before preventDefault and the browser does its native jump, so a back-to-top anchor inside a frame does move the window. State the boundary rather than let an author discover it.
All five cases exercised the hashless path, so the guard's hash half was unprotected. Narrowing it to `recordHistory && (!frameId || url.hash)`, the obvious "an anchor is explicit intent" follow-up, left this suite, frame-targeting, frame-missing, and the e2e case green while restoring the scrollIntoView. Verified: the new case is the only one that reds under that change, on all three engines. Also corrects the head-echo rationale, which described a merge this fixture never reaches. A foreground nav with a shared boundary takes the add-only addNewHeadElements; the removing mergeHead belongs to the background snapshot paths. The echo stays as cheap insurance, and the comment now says so rather than misleading whoever copies the pattern.
The docs now state that a frame-missing response advances the URL while holding the reader's offset: the path keeps a truthy frameId, so the guard applies even though nothing was swapped. Nothing pinned that, which is how the sentence it replaced went stale. Extend the existing URL-advance case rather than adding a sibling, since both halves are one contract. The spacer gives it teeth: a document that cannot hold an offset reports 0 whether the router moved it or not. Verified the assertion reds when the guard is reverted. The prefetch-frame comment described the same path as scrolling to top. It is prose, not an assertion, so nothing failed, but it taught the model this change replaced.
The prose said a back-to-top anchor inside a frame behaves exactly as it does anywhere else. That holds for a named target and is false for the most common spelling. href="#" and href="" parse to an EMPTY URL.hash, and the same-page bow-out tests the hash for truthiness, so it does not fire: the click is an ordinary frame nav that re-fetches the frame and, under the new rule, leaves the window still. Verified in a real browser against the gallery: href="#" holds the offset at 214 and re-fetches, while href="#named" bows out and the browser jumps natively. Document the trap and give both workarounds. Also distinguishes the two "frame not found" cases, which the escapes sentence blurred: an id that resolves to no live frame is a page nav and scrolls, while a RESPONSE missing the frame stayed frame-scoped and holds the offset. A reader hitting the webjs:frame-missing warning would otherwise match it to the wrong half. Also tightens the head-echo comment: addNewHeadElements is add-only for links and scripts but ends in a keyed-meta reconcile that does remove, so "removes nothing" overstated it. Inert here either way.
The claim was absolute and the gallery falsifies it: filtering the frames demo from All to Done while scrolled to the bottom moves the window. The router writes nothing there (verified with scrollTo, scrollIntoView and the scrollTop setter instrumented, and it is not scroll anchoring, since overflow-anchor: none changes nothing). The cause is the browser clamping an offset the document is briefly too short to hold, because a swap removes content before inserting its replacement. What WebJs guarantees is that it writes no scroll for a frame swap, which is what the guard delivers. Say that, name the one case where the viewport still moves, and give the way to avoid it.
The previous commit claimed a swap leaves the document "briefly shorter than either end state". That was invented to explain a 474 -> 148 jump which turned out to be the test harness: the filter link sits above the viewport when the page is scrolled to the bottom, and playwright's click scrolls an element into view before dispatching. Clicking from page context, with no actionability scroll, gives 474 -> 405 against a document going 874 -> 805. The new maximum is exactly 405, so it is an ordinary clamp of precisely the height the document lost, with no transient involved.
vivek7405
force-pushed
the
fix/frame-swap-scroll
branch
from
August 20, 2026 12:33
5f8033d to
6543c37
Compare
14 tasks
vivek7405
added a commit
that referenced
this pull request
Aug 20, 2026
main landed #1429, which excludes frame-scoped responses from the forward-nav scroll block outright. That removes the stray scroll this guard was written against: a click-driven frame nav no longer runs the page scroll-to-top at all, so it can no longer disturb an open restore by that route. The case is kept, because it asserts the OUTCOME (a frame swap must never move a restore in progress) rather than the mechanism, and the outcome has to hold however the internals move. It is now defended twice, by #1429's guard and by the restore window. Only the comment changes; the assertions are untouched and still pass. Re #1428.
vivek7405
added a commit
that referenced
this pull request
Aug 20, 2026
…-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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1427
A
<webjs-frame>swap scrolled the whole window to top, so filtering a panel below the fold threw the reader back to the top of the page with the panel they had just clicked in off screen. Reproduced on https://gallery.webjs.dev/features/frames: scroll to 400, click a filter tab, land at 0.What changed
The scroll block in
fetchAndApplywas gated onrecordHistoryalone, which means "a foreground navigation the reader initiated". A click on a frame-driving link is exactly that, since a frame click advances the URL deliberately, so a frame swap fell into the page-navigation scroll by omission rather than by decision. It is now gated onrecordHistory && !frameId.A frame swaps ONE region and leaves the rest of the document standing, the scroll offset included, so the router writes no scroll for it. That is the same rule
restoreGenerationinscroll.jsalready applies when deciding what ends a scroll-restore window, so the two paths now agree on what a frame nav is. Turbo, which<webjs-frame>is modelled on, likewise never scrolls on a frame navigation.The hash branch is excluded too. A
#anchoron a frame link is no more a request to move the document viewport than the swap itself is, and one rule ("a frame swap never moves the window") beats two.Both callers that could reach the block with a frame id are covered: a link click through
performNavigation, and a frame-targeted form submission throughsubmitForm, which hardcodesrecordHistory: true.loadFrame(a<webjs-frame src>self-load) and the background revalidation already passedrecordHistory: false.Deliberately not in scope
Turbo's
autoscrollopt-in, which scrolls the frame itself into view on swap. This is only about the router not moving the reader unasked; the docs now say the absence is a decision.Test plan
packages/core/test/routing/browser/frame-swap-scroll.test.js, 6 cases across Chromium, Firefox, and WebKit: a nested frame link, an anchored (#hash) frame link, an externaldata-webjs-framelink, and a frame-targeted form submission all hold the offset; a_topbreakout and an unresolvable frame id are page navigations and still scroll to top.frame-missing.test.js: the frameless-response case now asserts the scroll half of its contract alongside the URL advance, since that path keeps a truthyframeIdand so holds the offset with nothing swapped.test/e2e/e2e.test.mjs, over the real wire against the blog's/frame-demo.scrollYto 0 and look exactly like the defect), the starting offset is asserted before the click, and the page-nav case asserts the destination is still tall enough to hold the old offset.recordHistory && (!frameId || url.hash), on all three engines).Docs
.agents/skills/webjs/references/client-router-and-streaming.md, the<webjs-frame>section.website/app/docs/client-router/page.ts, a new subsection under the frame docs.Note on the overlap with #1430
fix/ios-back-swipe-snapshot-ab(#1430) also editsfetch-apply.js, so this originally warned of a conflict. That is no longer true: its hunk there is now a comment block well above this one, andgit merge-treereports the two branches merge CLEAN in either order. No coordination needed.Worth knowing for what comes after, though: #1430 stops setting
history.scrollRestoration = 'manual'and leaves it'auto'. Nothing in this PR depends on that (the guard only decides whether a scroll is written, not who restores it), but any later work on the popstate path should be designed against #1430's world rather than this one.