feat: add data-preserve-scroll to opt out of the forward-nav scroll - #1446
Conversation
A forward navigation always scrolls to the top and an author had no way to
say otherwise. That default is right and matches Next and Remix 3, but there
was no escape hatch for a filter, sort, or tab link whose control sits below
the fold, or a form that re-renders in place with validation errors. WebJs
has an extra reason to want one: a searchParams-only navigation already
morphs the deepest shared boundary and preserves hydrated component state,
so the scroll was the only thing such a navigation still threw away.
The attribute resolves through closest(), so a wrapper marks a whole region
and a single link opts back out with ="false". A hash anchor still wins,
because the reader named a target. navigate(url, { scroll: false }) is the
programmatic twin, spelled the way Next spells it.
The browser file is the headline layer: linkedom has no layout, so a position assertion there would pass vacuously whatever the code did. The unit file therefore covers attribute resolution only, plus one wiring assertion on navigate()'s option that catches a typo in the option name faster than a browser round trip would.
…ributes
Five surfaces: the skill's client-router reference (the attribute, the
closest() walk, the three inert cases, and a line keeping it apart from the
Back/Forward restore, which is the browser's and has no per-link knob), the
docs site, the muscle-memory gotchas (Next's scroll={false} is the one scroll
reflex that ports, and the section around it still stands), AGENTS.md's
attribute-family index, and the gallery.
The gallery's metadata demo is the honest first consumer: its ?topic= links
re-render the page you are on, so the control you just clicked scrolls out
from under you. The mark sits on the <ul> so the demo shows the ancestor walk
rather than describing it.
|
Design rationale: why the motivating link is the one link I did not mark This issue was filed off the blog's "All posts" link, and the gallery comment on Preserving scroll carries the reader's CURRENT offset onto the destination. It
Marked, they land at offset 3000 in the index, which corresponds to nothing they What the reader actually wants there is a forward-navigation snapshot restore The gallery's own "Back to page one" link has the same shape, and additionally So the demo moved to the metadata card, whose One correction while I was in there. The issue's prior-art table listed Turbo's |
|
Decision: the hash carve-out, and why it is a branch rather than a guard The obvious way to implement this is one guard on the whole scroll block: if (recordHistory && !frameId && !preserveScroll) { ... }That is wrong, and it is wrong in a way no default-path test can catch, because A reader who writes Note the arm where the hash names an element the RESPONSE does not contain. That I ran the wrong version deliberately: gating the whole block and dropping the |
The guard has two assertions and I read only the first. The floor test passes at 70 against a floor of 69, but a second test asserts the floor EQUALS the count, deliberately, so that adding an export is a conscious act rather than something that quietly widens the tolerance. Also promotes the new docs section to an h2 and moves it out of the Back/Forward section. As an h3 it captured the revalidate() paragraph that belongs to the snapshot cache, and it nested the forward-nav knob inside the Back/Forward section, which is the exact conflation the section's own prose warns against.
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole diff cold against AGENTS.md and the per-package guides.
The shape of the change is right. Putting the resolver in scroll.js and leaving the scroll WRITE in fetch-apply.js keeps the leaf module a leaf, and moving refresh and noPrefetch into an opts bag was the correct call at eleven positionals rather than adding a twelfth. The hash carve-out being a branch rather than a guard on the whole block is the detail I would have most expected to get wrong, and the browser file has a case pinning exactly that.
Two real problems and one bad claim, all inline or below.
The one that matters is the barrel floor. That guard has two assertions and only the first is a floor; the second asserts equality on purpose, so adding an export is meant to be a conscious act. Missing it turned two required CI jobs red.
The second is that the PR body called the branch green when it was not. The local run's failing-test grep matched file-level lines only, so a test-level failure inside an otherwise-passing file went unseen and got folded into "known pre-existing artifact". The lesson is about the grep, not the artifact: the artifact claim is real, but it was only ever verified for one of the four files it was asserted over. Now verified for all four, each failing identically at origin/main in a clean worktree.
Docs placement is the third. Worth catching because the misplacement contradicts the section the same PR added.
The new barrel export makes the underscore seam count 64. Two adjacent numbers in the same comment were already stale before this branch (the checked total drifted to 172 and the runtime total to 236), so all three are re-derived with the test's own entryPairs and checkedNames rather than left half-corrected and self-contradicting.
vivek7405
left a comment
There was a problem hiding this comment.
Delta read over the barrel-floor and docs-move fix, traced for blast radius rather than read in isolation.
The floor is right: the branch barrel exports exactly 70, so both assertions hold, and BARREL_FLOORS is the only place that number lives. The .d.ts coverage guards are unaffected because _-prefixed exports are exempt, so the checked total does not move.
The docs move is clean where it matters most, which is the referrer: #preserving-scroll has exactly one, in Opt-out per link / form, and it says "above" and still is. Nothing keys on heading level or order either, since extractHeadings scans any depth and the docs-links test strips fragments before checking.
One stale comment, inline. It is comment-only and fails nothing, but the same commit raised a count for the same export and left this one behind, which is exactly how a number rots.
…e falsifiable Two problems the final review found. A submitter is form-associated by form="id", not by containment, so closest() from a detached button never passes through the form it submits and the form's own mark was dropped. Every doc surface promised otherwise. The form is now a FALLBACK, consulted only when the trigger resolves no carrier, so a marked form reaches its detached buttons while data-preserve-scroll="false" on a button inside one still wins. The frame case was a tautology: with the #1427 guard intact the block never runs, and with it deleted preserveScroll suppresses the write, so the offset was START_Y either way and deleting that guard left all eight cases green. An unmarked sibling link is the discriminating half and now carries the claim.
The lookup is submitter-then-form-fallback now, not a single walk up from the submitter, so the two surfaces that described the mechanism say what the code does. The Next migration table listed data-prefetch but not the scroll knob, which left the docs site silent on a reflex the agent-facing gotchas page had just been updated to say does port.
vivek7405
left a comment
There was a problem hiding this comment.
Final read over the whole diff at its current head, cold.
The core mechanism holds up. The hash carve-out is a branch rather than a guard, every fetchAndApply call site was converted (the popstate revalidation correctly stops at nine positionals so the tenth stays undefined), and the barrel floor now matches the real count.
Two real problems, both inline, and one missed doc surface.
The one worth the round is that the frame case could not fail. Both code paths produce the same offset, so it observed neither the #1427 rule it claimed nor the resolver, and I confirmed it by deleting the frame guard and watching all eight cases stay green. The file's own header says every fixture rule is load-bearing and easy to get wrong in a way that leaves the test green either way, which is exactly what happened to it.
The second is a real behaviour gap rather than a test one: a submitter attached with form="id" from outside the form loses the form's mark, while three doc surfaces promise it does not.
…allback cannot The submitter-to-form fallback made two header claims false. Dropping closest() no longer reds case 6, because the fallback catches the form's mark, so nothing was left proving the walk on the submit path. And the guard toggle now reds case 9 as well. Both restated from a real toggle rather than reasoning, and the walk is pinned by a marked fieldset inside an unmarked form, which the fallback cannot rescue.
vivek7405
left a comment
There was a problem hiding this comment.
Check over the two fix commits, traced rather than read in isolation.
The signature change is consistent: every caller agrees with the new contract, the fallback is ordered so it fills in a missing carrier without overriding a resolved one, and the frame case is falsifiable now.
But the fix decayed two claims in the same file it fixed, which is the thing that file's own header rule exists to catch. Both are inline. The header says to re-run the toggle and restate the list when a later commit touches the resolver, and this commit touched the resolver and appended two bullets without revisiting the one it invalidated.
Worth being precise about what was lost rather than just wrong: with case 6 no longer reding under the walk toggle, NO case at either layer proved the contained-submitter walk any more. The claim decaying and the coverage disappearing were the same event.
Every toggle in the list was re-run against the current code, so the stamp names that commit rather than the one where the list was first written. The note now cites the decay that actually happened here instead of describing the risk in the abstract.
vivek7405
left a comment
There was a problem hiding this comment.
Check over the counterfactual restatement, verifying each claim against the code rather than reading the prose.
Every bullet is now true as written. I reduced findScrollCarrier to a bare hasAttribute and confirmed only case 3 reds, since case 4's link carries the attribute itself and cases 6 and 9 are rescued by the fallback. The guard toggle reds 2, 3, 6, 7, 9 and nothing else. The new fieldset assertion is genuinely walk-only: closest() from the button reaches the marked <fieldset>, which an unmarked <form> cannot supply through the fallback, so it reds under the same mutation, and the sibling outside the region returns false through both lookups.
The doc surfaces that carried the falsified "the submitter passes through the form on its way up" wording were resynced, so nothing in the repo still states the retracted mechanism.
Nothing wrong in the code or the tests. One stale claim in the PR body, which is the record a cold reader resumes from, so it is worth correcting rather than leaving to contradict the merged test header on the one point the fix exists to retract.
The three numbers on this file were stale twice over: two had drifted before this branch touched anything, and the third went stale the moment a new export landed. A count in a comment fails nothing when it rots, so nothing catches it, and re-correcting them only resets the clock. The assertion message now derives its number from the exemptTotal it already has in scope, so it cannot disagree with reality, and the prose keeps the rationale while dropping the frozen totals. The message was verified by forcing the assertion, not by reading it.
|
Decision: deriving the export counts instead of correcting them again The review chain turned up three hand-written export counts in I corrected all three first, and that was the wrong fix. The counts sit in So the numbers are gone instead. The assertion message derives its count from I verified the derived message by forcing the assertion to fire rather than by Worth stating the general shape, because it recurred three times on this PR: a |
vivek7405
left a comment
There was a problem hiding this comment.
One more pass over the whole diff at head, end to end.
Walked the resolver, the scroll block, and every call site of the three changed signatures. All four fetchAndApply callers are converted, the background revalidation is byte-identical in effect (an absent opts bag means exactly what the two absent positionals meant), and nothing outside navigator.js and events.js calls any of the changed functions, so the signature change has no hidden consumers. The opts?.scroll === false strict test keeps an omitted option and an explicit true on today's behaviour, and no @ts-expect-error in the route-types test is invalidated by the widened declaration.
One semantic edge I checked deliberately: a detached submitter whose own ancestor chain carries ="false" wins over its marked form, which is the nearest-carrier-to-the-trigger rule applied consistently, and the docs say exactly that.
The docs now agree with the code on the one sentence that changed mechanism (submitter first, form as fallback), the #preserving-scroll anchor has a real target, and the code-block escape renders a literal hole. Nothing left to fix.
#1446 (data-preserve-scroll) landed on main and touches the same client-router files. Two real conflicts, both additive rather than contradictory. events.js: keep both imports, resolvePreserveScroll from scroll.js and absorbFragmentClickPopState from navigator.js. The two changes sit in different parts of onClick, since the fragment bow-out returns before the preventDefault that the preserve-scroll opts bag rides on. navigator.js: performSubmission gains their opts parameter and keeps the clearFragmentNav() this branch added at its head. The docs, the test file and the rest merged cleanly with both sides present.
Closes #1436
A forward navigation always scrolls to the top of the page, and an author had
no way to say otherwise. That default is right and matches every comparable
framework, but there was no escape hatch for the cases where the author knows
better: a filter, sort, or tab link whose control sits below the fold, a pager,
or a form that re-renders in place with validation errors. WebJs has an extra
reason to want one that the other frameworks do not, since a searchParams-only
navigation already morphs the deepest shared boundary and preserves hydrated
component state, so scrolling to top is the only thing such a navigation still
throws away.
data-preserve-scrollon a link or form keeps the reader's current offset.It resolves through
closest(), so a wrapper marks a whole region at once anda single link opts back out with
data-preserve-scroll="false". A hash anchorstill scrolls to its target, because the reader named one. The programmatic
twin is
navigate(url, { scroll: false }), spelled the way Next spells it.Decisions worth knowing
data-no-routeranddata-prefetch, sincedata-webjs-*is reserved for names that mark framework machinery. Named forwhat is preserved rather than what is disabled:
data-no-scrollwould read assuppressing the hash-anchor scroll too, which it does not.
falseis the only value thatmeans anything, matching Remix 3's
rmx-reset-scroll !== 'false'and thenormalization
prefetchModealready applies here. It exists so one link insidea marked wrapper can opt back out, which the ancestor walk makes necessary.
data-webjs-framerather thandata-no-router. Theprecedents differ by the size of the hammer:
data-no-routerturns the routeroff, and an ancestor doing that silently would surprise, while this is a soft
preference whose natural authoring unit is a region.
recordHistory && !frameId(dogfood: a <webjs-frame> swap scrolls the whole page to top #1427), so a frame swap writes no scroll and theattribute asks for something already true. No branch and no warning were added:
a marked
<nav>over a mixed set of links is correct authoring, and warning oncorrect authoring is noise.
fetchAndApplythreading.refreshandnoPrefetchmoved into anoptsbag alongside the new flag, rather than adding a twelfth positional. The nine
that remain are request inputs; these three are response policy.
What is deliberately NOT here
The issue was filed off a "back to list" link, and marking that link would be
wrong. Preserving scroll carries the reader's CURRENT offset onto the
destination; it does not restore the destination's remembered offset. On the
blog journey those are different numbers, so the attribute would be either a
no-op (clicking the header copy, where the offset is already 0) or worse than
the default (clicking the footer copy, which would carry the article's offset
into the index). The full arithmetic is in the issue under Correction 2. The
blog links and the gallery's "Back to page one" link are left alone on purpose.
Also out of scope, and noted in the issue:
navigate(url, { replace: true })passes
replaceinto theisPopStateslot, and there is noreplaceStatecallanywhere in
packages/core/src, so it currently records no history entry andleaves the URL unchanged. Real, adjacent, and a separate change with its own
history semantics. Not tidied while threading this flag.
Test plan
packages/core/test/routing/browser/nav-preserve-scroll.test.js: 9 cases witha real
window.scrollY, green on Chromium, Firefox and WebKit. Covers themarked link, the unmarked default, the ancestor walk, the
="false"escape,the hash carve-out, both form halves, the programmatic option with its
optionless and explicit-
truecontrols, the frame case (both an unmarked anda marked link, see below), and a submitter attached from outside its form.
carries them, each red on exactly the cases the test file's header names.
Three corrections came out of running them rather than assuming, and every one
was a case of a test looking like it proved something it did not:
deleted,
preserveScrollsuppressed the write anyway, so the offset wasunchanged either way and all eight cases stayed green. It now pairs an
unmarked link (the falsifiable half) with the marked one, and the header
says why the marked half cannot fail so nobody trims it as redundant.
closest()bulletand took the only coverage of the walk with it: case 6 stopped reding under
that toggle because the fallback catches the form's mark. The walk is now
pinned at the unit layer by a marked
<fieldset>inside an UNMARKED form,which the fallback cannot rescue.
packages/core/test/routing/router-client.test.js: ten resolvertests covering value forms, the ancestor walk, the nearest-carrier escape, the
submitter/form fallback in all its orderings, and the walk-only fieldset case,
plus one wiring assertion on
navigate's option.linkedom has no layout, so the file says why it asserts no positions.
packages/core/dist. No new file; thechange ships no server-side code and the browser suite covers three engines
where an e2e run would cover one.
router code, none of it on the runtime-sensitive list, and none matches the
parity hook's filename regex.
npm test: 4491/4498, with 6 failures across four files(
test/bun/listener.test.mjs,test/bun/listener-overhead.test.mjs,packages/server/test/dev/asset-helper-serve.test.js,packages/server/test/elision/differential-elision.test.js). All six are theknown linked-worktree artifact: every one of those four files fails
identically at
origin/mainin a clean baseline worktree with none of thisbranch's changes. CI installs from the branch and is unaffected.
modulepreload hints (
/,/docs/client-router,/ui,/ui/button,/features/client-router,/features/metadata?topic=webjs), and the attributeis in the served bytes on exactly the pages that should carry it.
webjs checkclean in gallery, website and examples/blog;webjs doctorcleanin gallery and website.
the updated skill, and passes
webjs check. No new demo route or module, sothe
gallery:clearlists, the coverage manifest and the scaffold tests need nochange (all green).
Docs
Five surfaces: the skill's
client-router-and-streaming.md(the attribute, thewalk, the three inert cases, and a line keeping it apart from the Back/Forward
restore, which is the browser's and has no per-link knob),
website/app/docs/client-router/page.ts,muscle-memory-gotchas.md(Next'sscroll={false}is the one scroll reflex that ports, and the surroundingdo-not-hand-roll-a-restore rule still stands),
AGENTS.md's attribute-familyindex, the
next/linkrow onwebsite/app/docs/migrating-from-nextjs/page.ts,and the gallery.
Folded in
test/types/dts-export-coverage.test.mjscarried three hand-written exportcounts in its comments. The new barrel export made one stale, and the other two
had already drifted before this branch touched anything. Re-correcting them only
resets the clock, since a number in a comment fails nothing when it rots and so
nothing catches it. The assertion message now derives its count from the
exemptTotalalready in scope, and the prose keeps the rationale while droppingthe frozen totals. The message was verified by forcing the assertion rather than
by reading it.
Filed separately as #1449:
navigate(url, { replace: true })passesreplaceinto the
isPopStateparameter slot, so the URL never changes (bothpushStatesites are gated on
recordHistory, and there is noreplaceStateanywhere inpackages/core/src) and the call takes the back-button snapshot-restore path.Adjacent to this work and found while threading the new option through the same
call, but it has its own history semantics and is not tidied here.
MCP: N/A, no change to the route table, action hashes, component registration or
a check rule. Editor plugins: N/A, no template-grammar, snippet or language-service
change. README: N/A, not a headline capability. Version bump: N/A, left for a
release PR.