diff --git a/.agents/skills/webjs/references/client-router-and-streaming.md b/.agents/skills/webjs/references/client-router-and-streaming.md index 3e7bd378e..e9533e0bb 100644 --- a/.agents/skills/webjs/references/client-router-and-streaming.md +++ b/.agents/skills/webjs/references/client-router-and-streaming.md @@ -72,7 +72,7 @@ revalidate('/products/123'); // evict one URL from the snapshot revalidate(); // clear the entire snapshot cache ``` -The router keeps a URL-keyed snapshot cache (LRU, cap 16) so Back/Forward restores instantly, then refetches in the background. Call `revalidate(path)` after a server action mutates data a cached page depends on. Wire bytes are minimized by an `X-Webjs-Have` header, so the server returns only the divergent layout fragment. Concurrent navigations abort the prior in-flight fetch, and scroll is restored on Back/Forward. +The router keeps a URL-keyed snapshot cache (LRU, cap 16) so Back/Forward restores instantly, then refetches in the background. Call `revalidate(path)` after a server action mutates data a cached page depends on. Wire bytes are minimized by an `X-Webjs-Have` header, so the server returns only the divergent layout fragment. Concurrent navigations abort the prior in-flight fetch, and scroll is restored on Back/Forward. The popstate an in-page fragment CLICK produces is absorbed rather than re-navigated (#1437), so an anchor click restores nothing and re-fetches nothing, the repeat click of one anchor included (that one REPLACES its entry rather than pushing, so it arrives with the url unchanged). The gate is PROVENANCE, the router marking the click it bowed out of and the next popstate consuming that mark, rather than any comparison of urls: a Back between two entries differing only by fragment can still need a re-render, because `getSubmitAction` prefers the raw `action` ATTRIBUTE and that carries no fragment, so a bound-submitter form declaring `action="/p"` pushes its 422 re-render at `/p` while the reader sits at `/p#sec`. A popstate with no click behind it therefore stays on the normal path, which means an ordinary Back or Forward between two fragment states still re-renders. Telling those apart would need to know whether the DOM was replaced between the two ENTRIES, which is per-entry state the router does not keep. **In-place refresh of the page you are on.** `refreshPage(mode)` re-renders the CURRENT url on the server and applies it without a page load. @@ -159,9 +159,9 @@ html`…contents…` On click the router walks `closest('webjs-frame')` from the target. If a frame is found and the response carries a matching ``, the swap is scoped to that frame's children, and the server returns ONLY that subtree. A link that drives a frame participates in link prefetch like any other, in that frame's own dimension (#1407), so a hovered or viewport-warmed frame link swaps on click with no round trip. A `` SELF-load is the exception: it neither reads nor keeps that cache, since asking a frame to load its own src is a freshness request rather than a hover being followed. See the prefetch section above for the frame dimension's rules. -**A frame swap never moves the window scroll.** A page navigation scrolls to top, the way a browser does; a frame swap changes one region and leaves the rest of the document standing, the reader's scroll offset included. That holds for a nested link, an external `data-webjs-frame` trigger, a frame-targeted form submission, and a `src` self-load alike, and it holds for a `#hash` on a frame link too, which rides the URL without moving the viewport. It does NOT cover a pure fragment link to a NAMED anchor (`#section`, same path and query), because the router never sees one: the click handler bows out before `preventDefault`, so the browser does its own native fragment jump and the window moves. +**A frame swap never moves the window scroll.** A page navigation scrolls to top, the way a browser does; a frame swap changes one region and leaves the rest of the document standing, the reader's scroll offset included. That holds for a nested link, an external `data-webjs-frame` trigger, a frame-targeted form submission, and a `src` self-load alike, and it holds for a `#hash` on a frame link too, which rides the URL without moving the viewport. It does NOT cover a pure fragment link whose path and query match the page it sits on, because the router never sees one: the click handler bows out before `preventDefault`, so the browser does its own native fragment jump and the window moves. -The EMPTY fragment is the trap, and it goes the other way. `href="#"` (and `href=""`) parse to an empty `URL.hash`, and the 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 this rule, leaves the window still. So a bare `Back to top` INSIDE a frame does nothing visible. Give it a real target (`href="#top"`), which the bow-out then honours, or a click handler that scrolls (its `preventDefault` runs first and the router stands down). +**Every spelling of a fragment link is the browser's, the bare `#` included** (#1437). `href="#"` is the back-to-top idiom and it serializes with an EMPTY fragment, which reads identically to no fragment at all through `URL.hash`, so the bow-out tests the `href` for a `#` instead. A `Back to top` therefore scrolls to top natively, inside a frame as well as outside one. `href=""` is NOT a fragment link: it resolves to the current url with the fragment REMOVED, which the spec reloads rather than jumps, so the router navigates it like any other link. The escapes are page navigations and DO scroll: `data-webjs-frame="_top"`, and an id `resolveTargetFrameId` cannot match to a live frame, which warns and degrades to a normal nav. Do not read that second one as covering a RESPONSE that lacks the requested frame (the `webjs:frame-missing` warning). There the frame resolved and the nav stayed frame-scoped, so the offset holds and only the panel is left unchanged. Turbo's `autoscroll` opt-in, which scrolls the frame itself into view on swap, has no WebJs equivalent; the router simply never writes scroll for a frame. diff --git a/AGENTS.md b/AGENTS.md index 0d7c96ed3..222d886c8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -417,7 +417,7 @@ Derive the type at every boundary: a DB row from the schema (`typeof todos.$infe ## Client navigation: automatic, nothing to opt into -The router auto-enables when `@webjsdev/core` loads (any page with a component), so there is nothing to opt INTO. An app that wants plain full-page (MPA) navigation can opt OUT app-wide with `{ "webjs": { "clientRouter": false } }` (#629), or per-moment at runtime with `disableClientRouter()`. SSR auto-emits KEYED boundary comment pairs around each layout's children AND the page itself (open ``, close ``; the route-key is the resolved path with param values percent-encoded, #1015). The router strictly scans both DOMs (any truncated, mispaired, or duplicated boundary poisons the scan) and applies a two-tier swap with Next.js remount parity: a changed route-key REPLACES (remounts) at the PARENT of the shallowest changed boundary (the range that contains the changed layout's own markup, exact Next remount scope), an unchanged one MORPHS the deepest shared boundary in place (a searchParams-only nav preserves hydrated component state). A poisoned or disjoint scan degrades to a full page load, never a guessed recovery, so silent DOM corruption is structurally impossible; outer-layout DOM identity is preserved on every soft path. **Every degradation dispatches `webjs:navigation-fallback` on `document` in ALL environments** (detail `{ cause, href, willReload }`, not cancelable), so a full page load on a click is observable in production rather than silent (#1114). Form submissions ride the same pipeline (`data-no-router` opts out). Wire bytes are minimized via the `X-Webjs-Have` header (`segment:route-key` entries, so a dynamic layout held for OTHER params is re-rendered rather than short-circuited; the server returns only the divergent fragment, served `private` so a shared cache can never store the reduced body and serve it to a full-page navigation, and additionally marked `Vary: X-Webjs-Have` for caches that honour it (#1140; `Vary` alone was not enough, since Cloudflare honours only `Accept-Encoding`)); scroll is restored on back/forward. **The link-prefetch cache is ANCHOR-VALIDATED** (#1114): a reduced fragment begins at the boundary the server short-circuited on, and on consume the router checks that boundary is still live with the same route-key. A root-anchored fragment therefore survives an unrelated navigation (still a cache hit), while one anchored deeper is discarded once that layout is gone, because applying it would share no boundary with the live DOM and force a full page load. The router also never prefetches the page it is already on (#1106): that request can never serve a later navigation and only occupies a capped cache slot. **The cache also carries a FRAME dimension (#1407):** a link driving a `` is prefetched with the `X-Webjs-Frame` header its click will send, so the warm entry is the subtree the swap needs and the click costs no round trip. The key is URL plus frame id, so a page fragment can never be applied into a frame region nor a frame subtree into a page swap, and a frame entry is validated by its `` still being live rather than by an anchor (a subtree carries no boundary comment). The server marks the sliced response `X-Webjs-Frame: `, so an unmarked answer to a framed request is a whole document (a streamed render, or an id absent from the output) and its body is discarded rather than stored, while the REFUSAL is memoed outside the fragment cache so the link re-asks about once per TTL rather than on every hover (that memo set is capped too, so a page with many distinct refused frame links can re-ask sooner) (only a deploy or an in-place `refreshPage` drops those early, never `revalidate()`, which is the post-mutation api); a framed link to the CURRENT url is a refresh and is not prefetched at all. A non-GET `
` that BINDS a server action (`action=${fn}`) is the no-JS write-path (with JS the router posts the same body to the same url and applies the response in place: a `422` swaps without reload, a `303` is followed via fetch). **The router ENCODES that body per the declared `enctype`** (#1307), resolved with native precedence (a submitter's `formenctype` over the form's, decided on PRESENCE rather than on the value being non-empty, #1322: a `formenctype=""` or `formmethod=""` overrides the form and then falls to its OWN invalid-value default, urlencoded and GET respectively, while `formtarget=""` is a plain string reflection with no enumerated states, so it overrides the form and means the current browsing context): `multipart/form-data` sends `FormData`, and `application/x-www-form-urlencoded`, the HTML default and therefore what a plain `` means, sends `URLSearchParams` (a `File` serializes as its name, as the platform does). It previously built `FormData` for everything, so an ordinary POST form sent a urlencoded body with JS off and a multipart body with JS on. A `text/plain` POST is the one encoding the server cannot parse, so the router declines it and lets the browser submit natively, which keeps both paths doing the same thing. A failed navigation recovers in place (a cancelable `webjs:navigation-error` event, else a minimal in-place alert), never a destructive full reload. +The router auto-enables when `@webjsdev/core` loads (any page with a component), so there is nothing to opt INTO. An app that wants plain full-page (MPA) navigation can opt OUT app-wide with `{ "webjs": { "clientRouter": false } }` (#629), or per-moment at runtime with `disableClientRouter()`. SSR auto-emits KEYED boundary comment pairs around each layout's children AND the page itself (open ``, close ``; the route-key is the resolved path with param values percent-encoded, #1015). The router strictly scans both DOMs (any truncated, mispaired, or duplicated boundary poisons the scan) and applies a two-tier swap with Next.js remount parity: a changed route-key REPLACES (remounts) at the PARENT of the shallowest changed boundary (the range that contains the changed layout's own markup, exact Next remount scope), an unchanged one MORPHS the deepest shared boundary in place (a searchParams-only nav preserves hydrated component state). A poisoned or disjoint scan degrades to a full page load, never a guessed recovery, so silent DOM corruption is structurally impossible; outer-layout DOM identity is preserved on every soft path. **Every degradation dispatches `webjs:navigation-fallback` on `document` in ALL environments** (detail `{ cause, href, willReload }`, not cancelable), so a full page load on a click is observable in production rather than silent (#1114). **A same-document fragment jump is the browser's on the CLICK** (#1437): a link whose path and query match the page it sits on is never intercepted (in every spelling, the bare `#` back-to-top included, though `href=""` carries no fragment and so is a normal navigation), and the popstate an in-page fragment CLICK produces is absorbed rather than re-navigated, so clicking `` (or the bare `#` back-to-top idiom, or either one repeatedly) never re-fetches, never re-swaps, and leaves live DOM identity and hydrated state intact. The gate is PROVENANCE, the router marking the click it bowed out of, not a url comparison: a repeat click REPLACES its entry so it arrives with the url unchanged, and a Back between two entries that differ only by fragment can still need a re-render (the no-JS write path reaches that shape, since `getSubmitAction` prefers the raw `action` ATTRIBUTE, which carries no fragment, so a bound-submitter form declaring `action="/p"` pushes its 422 re-render at `/p` while the reader sits at `/p#sec`). So a popstate with no click behind it is left on the normal path, which means an ordinary Back or Forward between two fragment states still re-renders. Form submissions ride the same pipeline (`data-no-router` opts out). Wire bytes are minimized via the `X-Webjs-Have` header (`segment:route-key` entries, so a dynamic layout held for OTHER params is re-rendered rather than short-circuited; the server returns only the divergent fragment, served `private` so a shared cache can never store the reduced body and serve it to a full-page navigation, and additionally marked `Vary: X-Webjs-Have` for caches that honour it (#1140; `Vary` alone was not enough, since Cloudflare honours only `Accept-Encoding`)); scroll is restored on back/forward. **The link-prefetch cache is ANCHOR-VALIDATED** (#1114): a reduced fragment begins at the boundary the server short-circuited on, and on consume the router checks that boundary is still live with the same route-key. A root-anchored fragment therefore survives an unrelated navigation (still a cache hit), while one anchored deeper is discarded once that layout is gone, because applying it would share no boundary with the live DOM and force a full page load. The router also never prefetches the page it is already on (#1106): that request can never serve a later navigation and only occupies a capped cache slot. **The cache also carries a FRAME dimension (#1407):** a link driving a `` is prefetched with the `X-Webjs-Frame` header its click will send, so the warm entry is the subtree the swap needs and the click costs no round trip. The key is URL plus frame id, so a page fragment can never be applied into a frame region nor a frame subtree into a page swap, and a frame entry is validated by its `` still being live rather than by an anchor (a subtree carries no boundary comment). The server marks the sliced response `X-Webjs-Frame: `, so an unmarked answer to a framed request is a whole document (a streamed render, or an id absent from the output) and its body is discarded rather than stored, while the REFUSAL is memoed outside the fragment cache so the link re-asks about once per TTL rather than on every hover (that memo set is capped too, so a page with many distinct refused frame links can re-ask sooner) (only a deploy or an in-place `refreshPage` drops those early, never `revalidate()`, which is the post-mutation api); a framed link to the CURRENT url is a refresh and is not prefetched at all. A non-GET `` that BINDS a server action (`action=${fn}`) is the no-JS write-path (with JS the router posts the same body to the same url and applies the response in place: a `422` swaps without reload, a `303` is followed via fetch). **The router ENCODES that body per the declared `enctype`** (#1307), resolved with native precedence (a submitter's `formenctype` over the form's, decided on PRESENCE rather than on the value being non-empty, #1322: a `formenctype=""` or `formmethod=""` overrides the form and then falls to its OWN invalid-value default, urlencoded and GET respectively, while `formtarget=""` is a plain string reflection with no enumerated states, so it overrides the form and means the current browsing context): `multipart/form-data` sends `FormData`, and `application/x-www-form-urlencoded`, the HTML default and therefore what a plain `` means, sends `URLSearchParams` (a `File` serializes as its name, as the platform does). It previously built `FormData` for everything, so an ordinary POST form sent a urlencoded body with JS off and a multipart body with JS on. A `text/plain` POST is the one encoding the server cannot parse, so the router declines it and lets the browser submit natively, which keeps both paths doing the same thing. A failed navigation recovers in place (a cancelable `webjs:navigation-error` event, else a minimal in-place alert), never a destructive full reload. The advanced client-router surface is in `references/client-router-and-streaming.md`: **link prefetch** (on by default, device-adaptive default: `intent` on a hover pointer, `viewport` (dwell-gated, cancel-on-scroll-out) on touch, per-link `data-prefetch` override, and frames participate: a link driving a frame is warmed in that frame's own dimension), **`data-preserve-scroll`** (per link, or per wrapping element, keeping the reader's offset instead of the default forward-nav scroll-to-top, with `navigate(url, { scroll: false })` as its programmatic twin), **``** partial-swap regions, **View Transitions** (opt-in via ``, where the router checks that exact `content` value and a bare tag enables nothing, plus `data-webjs-permanent` to persist a live element), **stream actions** (`` element-level updates, #248), and the **opt-in nav-loading indicator** (`` exposes a `data-navigating` attribute during a nav so you can style a CSS-only progress affordance, off by default because toggling a root attribute re-resolves `oklch()` tokens to a one-frame repaint flash on iOS WebKit, #610). Production benefits from HTTP/2 at the edge; `npm run start` speaks plain HTTP/1.1 (put a reverse proxy in front for TLS + HTTP/2). diff --git a/packages/core/src/router-client/events.js b/packages/core/src/router-client/events.js index bfc245298..f3aae7f85 100644 --- a/packages/core/src/router-client/events.js +++ b/packages/core/src/router-client/events.js @@ -8,12 +8,12 @@ */ import { findAnchorInPath } from './anchors.js'; import { NON_HTML_EXTENSIONS } from './constants.js'; -import { enabled } from './state.js'; +import { enabled, markFragmentNav } from './state.js'; import { warnIfActionSubmissionCannotDeliver } from './diagnostics.js'; import { buildSubmitFormData, encodeSubmitBody, getSubmitAction, getSubmitEnctype, getSubmitMethod } from './form-encoder.js'; import { resolveTargetFrameId } from './frames.js'; import { resolvePreserveScroll } from './scroll.js'; -import { performNavigation, performSubmission } from './navigator.js'; +import { absorbFragmentClickPopState, performNavigation, performSubmission } from './navigator.js'; /** @param {MouseEvent} e */ export function onClick(e) { @@ -24,7 +24,6 @@ export function onClick(e) { const anchor = findAnchorInPath(e); if (!anchor) return; if (anchor.hasAttribute('download')) return; - if (anchor.hasAttribute('data-no-router')) return; if (anchor.target && anchor.target !== '_self') return; const href = anchor.href; @@ -32,7 +31,31 @@ export function onClick(e) { const url = new URL(href); if (url.origin !== location.origin) return; - if (url.pathname === location.pathname && url.search === location.search && url.hash) return; + // `href.includes('#')` rather than `url.hash`, because the URL serializer + // reports BOTH a null fragment and an empty one as `''`, and only the second + // is a fragment navigation. `href="#"` keeps its `#` in `href` and per the + // spec navigates to the document element (the back-to-top idiom), while + // `href=""` resolves to the current url with the fragment REMOVED, which the + // spec reloads rather than jumping, so it must stay a router navigation. A + // `#` cannot appear anywhere else in a serialized url: the parser encodes it + // in the path and starts the fragment at it in the query (#1437). + if (url.pathname === location.pathname && url.search === location.search && url.href.includes('#')) { + // Leave a mark for the popstate this jump is about to fire. A REPEAT click + // replaces its history entry rather than pushing, so that popstate arrives + // with an unchanged url and is indistinguishable by comparison from a real + // Back between two entries that share one. Provenance is the only thing + // that separates them, and this is where the router has it (#1437). + markFragmentNav(url.href); + return; + } + // Checked AFTER the fragment bow-out on purpose. `data-no-router` opts out of + // ROUTING, and the bow-out above routes nothing either way, but the browser + // still performs the native jump and still fires the popstate that has to be + // recognised. Returning here first would leave EVERY click of a + // `data-no-router` in-page anchor unmarked, first and repeat alike, since the + // mark is the only thing that absorbs one, so each would be re-navigated + // destructively (#1437). + if (anchor.hasAttribute('data-no-router')) return; if (NON_HTML_EXTENSIONS.test(url.pathname)) return; e.preventDefault(); @@ -50,6 +73,14 @@ export function onClick(e) { /** @param {PopStateEvent} _e */ export function onPopState(_e) { + // The popstate an in-page fragment CLICK produces is not a navigation: the + // browser has already done the jump, so re-fetching and re-swapping would + // destroy live DOM identity and undo it (#1437). Absorbed only when the + // router MARKED that click on its way out (the bow-out above); a popstate + // with no mark behind it is a traversal and stays on the normal path, + // whatever its url. The callee's docstring has the full reasoning, including + // why no url comparison can stand in for the mark. + if (absorbFragmentClickPopState(location.href)) return; // popstate has no DOM anchor, so no frame context: restore via cache or // refetch the whole document. performNavigation(location.href, true, null); diff --git a/packages/core/src/router-client/navigator.js b/packages/core/src/router-client/navigator.js index 02a4650cf..83778f7ae 100644 --- a/packages/core/src/router-client/navigator.js +++ b/packages/core/src/router-client/navigator.js @@ -19,7 +19,7 @@ import { clearPrefetchHover, clearPrefetchRefused, clearPrefetchViewTimers, onPr // through bumpRestoreGeneration(), since ESM forbids assigning an import. import { afterTwoFrames, bumpRestoreGeneration, releaseHeightReservation, releaseScrollAnchor, reserveRestoredHeight, restoreGeneration, suppressScrollAnchoring } from './scroll.js'; import { snapshotCache, snapshotCurrent, snapshotGet } from './snapshot-cache.js'; -import { _setEnabled, bumpNavToken, currentNavigationToken, enabled, hardNavigate } from './state.js'; +import { _setEnabled, bumpNavToken, clearFragmentNav, consumeFragmentNav, currentNavigationToken, enabled, hardNavigate } from './state.js'; import { _swapCommit, applySwap } from './swap.js'; import { ensureUpgradeObserver } from './upgrade.js'; import { viewTransitionsEnabled } from './view-transition.js'; @@ -50,6 +50,82 @@ let activeAbortController = null; */ let currentPageUrl = null; +/** + * Absorb the popstate produced by a fragment click the router bowed out of. + * + * The spec's "navigate to a fragment" ends by firing popstate, so an ordinary + * `` click reaches `onPopState`, which treated every + * popstate as back/forward and re-navigated. That re-fetched the page and + * re-swapped the DOM, destroying live node identity and hydrated state outside + * the anchor and undoing the jump the reader had just asked for (#1437). + * + * The gate is PROVENANCE, and it is the only thing that works. The router SAW + * the click it bowed out of, and it never sees a traversal, so `onClick` leaves + * a mark and the next popstate consumes it (`state.js` documents the lifetime). + * Two earlier attempts tried to decide this from the urls instead and both were + * measured wrong, because the urls carry no signal that separates the cases: + * + * - a REPEAT click REPLACES its entry rather than pushing, so it arrives with + * `location.href` UNCHANGED, exactly like a Back between two distinct + * entries that share a url. (Two clicks of one `#sec` link give two + * popstates at the same href, `history.length` unchanged, measured in + * Chromium.) So an unchanged url cannot mean "absorb". + * - a CHANGED url cannot mean "absorb" either. Same pathname and search does + * prove the two entries resolve to the same server response, but not that + * they hold the same DOM, and a swap in between makes them differ. The + * no-JS write path reaches that shape: `getSubmitAction` prefers the raw + * `action` ATTRIBUTE (`form-encoder.js:33`), which carries no fragment, so + * a bound-submitter form declaring `action="/p"` pushes its 422 re-render + * at `/p` while the reader sits at `/p#sec`. Back from that validation + * error differs only by fragment and still has to re-render. + * + * So a popstate the router did not cause is left alone, whatever its url. + * + * What that does NOT cover, deliberately: an ordinary Back or Forward between + * two fragment states of one page, which is a traversal with no click behind + * it, so it still re-navigates as it does today. Telling that apart from the + * 422 case needs to know whether the DOM was replaced between the two ENTRIES, + * which is per-entry state the router does not keep (`history.pushState` is + * called with `null` throughout). Turbo tags its entries for exactly this + * reason. Adding that here is a larger change than this fix, and getting it + * wrong reintroduces a swallowed Back, so this stops at the click. + * + * It RECORDS as well as deciding, so `currentPageUrl` tracks the fragment the + * reader is on. That field is otherwise written only in the `finally` of a + * completed navigation, and `snapshotCurrent` keys through `cacheKey`, which + * strips the fragment, so the record cannot disturb the snapshot cache. + * + * @param {string} href The destination, i.e. `location.href` at popstate time. + * @returns {boolean} True when the popstate was absorbed and the caller must do + * nothing further. + */ +export function absorbFragmentClickPopState(href) { + // Consume unconditionally, so a mark can never outlive the popstate it was + // left for, whatever this one turns out to be. + if (!consumeFragmentNav(href)) return false; + if (!currentPageUrl) return false; + /** @type {URL} */ let prev; + /** @type {URL} */ let next; + try { + prev = new URL(currentPageUrl); + next = new URL(href, location.href); + } catch { + return false; + } + // Defense in depth, and UNREACHABLE by construction today, so it carries no + // counterfactual: `onClick` marks only when the anchor's pathname and search + // already match `location`, and every writer of `currentPageUrl` clears the + // mark before it writes (`performNavigation`, `performSubmission`) or nulls + // it (`disableClientRouter`, with enable re-seeding). It is kept because that + // invariant lives in three separate files: a fourth writer of + // `currentPageUrl` added without clearing the mark would make it reachable, + // and this is what keeps that mistake from absorbing a popstate on a page the + // mark was never left for. If you add such a writer, clear the mark there. + if (prev.pathname !== next.pathname || prev.search !== next.search) return false; + currentPageUrl = next.href; + return true; +} + /** * The app's own `history.scrollRestoration`, captured at enable so * `disableClientRouter()` can put it back. @@ -153,6 +229,12 @@ export function disableClientRouter() { document.removeEventListener('click', onClick, false); document.removeEventListener('submit', onSubmit, false); window.removeEventListener('popstate', onPopState); + // A mark left by a click whose popstate has not fired yet must not survive + // the router stepping aside, or it would absorb the first same-url popstate + // after a re-enable, which is the swallowed-Back failure this exists to + // prevent (#1437). Every other pending piece of router state is torn down + // here too. + clearFragmentNav(); document.removeEventListener('pointerover', onPrefetchIntent, true); document.removeEventListener('focusin', onPrefetchIntent, true); document.removeEventListener('touchstart', onPrefetchIntent, /** @type any */ ({ capture: true })); @@ -437,6 +519,9 @@ export async function refreshPage(mode) { * a failure. Every other caller ignores the value. */ export async function performNavigation(href, isPopState, frameId, opts) { + // Same reasoning as in performSubmission: a real navigation invalidates any + // pending fragment mark (#1437). + clearFragmentNav(); const refresh = (opts && opts.refresh) || undefined; const preserveScroll = !!(opts && opts.preserveScroll); // #1008 / #936: a forward, main-document nav fired while the document is @@ -733,6 +818,10 @@ export async function performNavigation(href, isPopState, frameId, opts) { * failed. */ export async function performSubmission(href, method, body, frameId, form, opts) { + // A submission is real work, so any pending fragment mark is stale and must + // not survive into the popstate a later Back produces. This is exactly the + // 422 duplicate-entry path (#1437). + clearFragmentNav(); if (activeAbortController) activeAbortController.abort(); activeAbortController = new AbortController(); const signal = activeAbortController.signal; diff --git a/packages/core/src/router-client/prefetch.js b/packages/core/src/router-client/prefetch.js index d1405e4bb..f63156587 100644 --- a/packages/core/src/router-client/prefetch.js +++ b/packages/core/src/router-client/prefetch.js @@ -213,8 +213,9 @@ function relTokens(anchor) { /** * Decide whether an anchor is a same-origin in-app target the router can - * navigate, returning its absolute href or null. Shared by onClick and - * the prefetch listeners so eligibility never drifts between them. + * navigate, returning its absolute href or null. Used by the prefetch + * listeners, and it MIRRORS the filtering `onClick` performs inline rather + * than being shared with it, so the two must be changed together. * * @param {Element | null} anchor * @returns {string | null} @@ -229,8 +230,14 @@ export function eligibleAnchorHref(anchor) { let url; try { url = new URL(href); } catch { return null; } if (url.origin !== location.origin) return null; - // A pure same-page hash jump is not a navigation we fetch. - if (url.pathname === location.pathname && url.search === location.search && url.hash) return null; + // A pure same-page fragment jump is not a navigation we fetch. Tested by + // `href` rather than by `hash` for the reason spelled out at the matching + // line in `events.js` (a null and an empty fragment both read as `''`). These + // two lines must stay in lockstep: `onClick` keeps its own copy rather than + // calling this, so drift here means a link the click path ignores becomes + // prefetch-eligible. #1106 already refuses the current url in every + // dimension, so this is drift prevention, not a change in what is fetched. + if (url.pathname === location.pathname && url.search === location.search && url.href.includes('#')) return null; if (NON_HTML_EXTENSIONS.test(url.pathname)) return null; return href; } diff --git a/packages/core/src/router-client/state.js b/packages/core/src/router-client/state.js index 03e53ae34..41a6f6f69 100644 --- a/packages/core/src/router-client/state.js +++ b/packages/core/src/router-client/state.js @@ -90,3 +90,66 @@ export let enabled = false; export function _setEnabled(v) { enabled = v; } + +/** + * The href a bowed-out same-document fragment CLICK is navigating to, pending + * the popstate that click is about to produce. + * + * This exists because URLs alone cannot separate the two ways a popstate can + * arrive carrying the url the reader is already on, and the two need opposite + * treatment (#1437). + * + * - A REPEAT click of one in-page anchor REPLACES its history entry rather + * than pushing, so it fires popstate with `location.href` unchanged. The + * browser has already done the jump and there is nothing to fetch. + * - A Back between two DISTINCT entries that happen to share a url is a real + * traversal that must re-render. The no-JS write path produces that pair: a + * bound `` emits no `action` attribute (invariant 12), so + * `form.action` reflects the node document's URL and the 422 re-render + * pushes a duplicate entry at it. + * + * An earlier attempt tried to tell them apart by whether the url carried a + * fragment, on the reasoning that a repeat anchor click always has one and a + * 422 entry never does. The second half is false: `form.action` returns the + * document URL WITH its fragment (measured in Chromium, for a missing `action` + * attribute and an empty one alike), so a reader who used an in-page anchor + * before submitting produces a 422 entry carrying `#sec`, and the Back out of + * the validation error was swallowed. + * + * So the signal is not the url, it is provenance: the router SAW the click it + * bowed out of, and it never sees a traversal. `onClick` records the href here + * on its way out, the very next popstate consumes it, and anything that starts + * a real navigation or submission drops it so it cannot leak across. + * + * Underscore-prefixed because no source module reads the binding (they go + * through the three functions below); its only reader is the test suite, and + * this file's own convention is that an unprefixed export here reads as public + * API. + * + * @type {string | null} + */ +export let _pendingFragmentNav = null; + +/** + * Record that a bowed-out fragment click is about to fire a popstate. + * + * @param {string} href The absolute href the click is navigating to. + */ +export function markFragmentNav(href) { _pendingFragmentNav = href; } + +/** + * Consume the pending mark if it matches, and clear it either way. Clearing on + * a MISS matters as much as on a hit: a mark that outlived its popstate must + * not sit around waiting to swallow an unrelated one. + * + * @param {string} href `location.href` at popstate time. + * @returns {boolean} True when this popstate is the one that click produced. + */ +export function consumeFragmentNav(href) { + const hit = _pendingFragmentNav !== null && _pendingFragmentNav === href; + _pendingFragmentNav = null; + return hit; +} + +/** Drop the pending mark. Any real navigation or submission invalidates it. */ +export function clearFragmentNav() { _pendingFragmentNav = null; } diff --git a/packages/core/test/routing/browser/fragment-jump.test.js b/packages/core/test/routing/browser/fragment-jump.test.js new file mode 100644 index 000000000..afdc8ff67 --- /dev/null +++ b/packages/core/test/routing/browser/fragment-jump.test.js @@ -0,0 +1,415 @@ +/** + * Real-browser test for #1437: a same-document fragment jump belongs to the + * browser, so the router must not fetch, must not swap, and must leave live + * DOM identity alone. + * + * The defect had two halves. `onPopState` treated EVERY popstate as + * back/forward and re-navigated unconditionally, and the spec's "navigate to a + * fragment" ends by firing popstate, so an ordinary `` click + * entered the full navigation pipeline: it re-fetched the current URL, re-swapped + * the page, and took the reader's in-progress component state with it. And the + * click path's own bow-out tested `url.hash` for truthiness, which an empty + * fragment fails, so `href="#"` was intercepted rather than left alone. + * + * This MUST run in a real browser. linkedom implements no layout, no scrolling + * and no history traversal, so `window.scrollY` never moves there and + * `history.back()` drives nothing, which makes every assertion below pass + * vacuously against the bug. The bow-out DECISION is pure and IS unit-tested, + * beside the other popstate cases in `../router-client.test.js`; what cannot go + * there is the observable behaviour, which is the whole point of the fix. + * + * Fixture discipline, each item load-bearing: + * + * - **DOM survival is asserted with an INJECTED node, not only an expando.** + * Measured against the bug: an expando on a node the incoming response ALSO + * contains survives the destructive re-swap, because the morph reconciles + * that node in place and keeps its identity. Only a node the response does + * not contain is removed. So an expando alone is a test that passes with the + * bug present. The injected node is the real assertion and the expando is + * the secondary signal. + * - Every href keeps the page's OWN query string, built from the LIVE + * `location.href`. A link that replaces the search string pushes the page + * out of its web-test-runner session and takes down the whole run while + * every test still reports passing, so it reads as an infrastructure blip. + * - The target sits between two tall spacers, so the document can hold a + * non-zero offset both before and after the jump and the viewport moves by + * an unmistakable margin. + * - Clicks go through `el.click()` from page context, NEVER a harness click + * API. Those scroll the target into view before dispatching, and an in-page + * anchor is usually off screen exactly when this bug matters. That artifact + * produced a confidently wrong diagnosis during #1429. + * - `currentPageUrl` is seeded explicitly. `enableClientRouter()` seeds it + * from `location.href` on a real page load, and setup reproduces that rather + * than depending on when the ambient router was last enabled. + */ +import { disableClientRouter, enableClientRouter, _setCurrentPageUrl } from '../../../src/router-client.js'; + +import { assert } from '../../../../../test/browser-assert.js'; +import { installNavGuard } from '../../../../../test/browser-nav-guard.js'; + +/** Tall enough that the target can reach the viewport top from either side. */ +const SPACER = 3000; +/** Where the reader is before a bare-`#` click. Comfortably clear of 0. */ +const START_Y = 900; + +const frame = () => new Promise((r) => requestAnimationFrame(() => r())); +const tick = () => new Promise((r) => setTimeout(r, 0)); +/** Past a fetch, a swap, and the layout either produces. */ +async function settle() { for (let i = 0; i < 4; i++) await tick(); await frame(); } + +/** + * This page's url carrying `frag` as its fragment. Built from the LIVE url so + * the session query string survives (see the header note), which also makes + * pathname and search match by construction, the precondition the bow-out reads. + * + * @param {string} frag Fragment WITHOUT the `#`. Empty gives the bare `#`. + * @returns {string} + */ +function fragHref(frag) { + const u = new URL(location.href); + u.hash = frag; + // `URL` drops a `#` it considers empty, and the bare-`#` case is precisely + // the one under test, so append it rather than trusting the serializer. + return frag === '' ? u.href.replace(/#$/, '') + '#' : u.href; +} + +/** A DIFFERENT page, for the cross-document control. */ +function otherHref() { + const u = new URL(location.href); + u.searchParams.set('wj1437', 'other'); + return u.pathname + u.search; +} + +/** + * The live page: the links, then a tall spacer, the fragment target, and a + * second spacer. The boundary comments are what keep a swap (which only the + * control cases should ever produce) inside this container rather than + * replacing the whole body and taking the harness DOM with it. + */ +function liveHtml() { + return '' + + '' + + `
spacer above
` + + 'target' + + `
spacer below
` + + ''; +} + +/** + * A page response carrying its own spacers, so a swap that DOES happen cannot + * shorten the document and clamp `scrollY` to 0 for the wrong reason. It + * repeats the live boundary key, so the router morphs rather than degrading to + * a full page load, and echoes the live head so the merge cannot drop + * web-test-runner's session scripts. + * + * It deliberately CONTAINS `#wj-frag-stamp`, which is what makes the expando + * a weak signal and the injected node the real one (see the header note). + */ +function pageResponse() { + return '' + document.head.innerHTML + '' + + '' + + 'stamp' + + `
swapped spacer
` + + 'target' + + `
swapped spacer below
` + + '' + + ''; +} + +suite('Client router: a same-document fragment jump is the browser\'s (#1437)', () => { + let navGuard, container, origFetch, origScrollBehavior, origUrl; + /** Every fetch the router issued, tagged with whether it asked for a frame. */ + let fetched; + /** `webjs:navigation-fallback` events seen. A fragment jump must produce none. */ + let fallbacks; + /** A node the incoming response does NOT contain. The real survival probe. */ + let injected; + /** A node the response DOES contain, carrying an expando. The weak probe. */ + let stamped; + + function setup() { + navGuard = installNavGuard(); + enableClientRouter(); + origUrl = location.href; + origScrollBehavior = document.documentElement.style.scrollBehavior; + // The assertions are about position, not animation, and a smooth scroll + // would not have landed by the time they run (#601). + document.documentElement.style.scrollBehavior = ''; + + container = document.createElement('div'); + container.innerHTML = liveHtml(); + document.body.appendChild(container); + + // The survival probes, both added AFTER the fixture, the way a hydrated + // component or an app script would add live state the server never sent. + injected = document.createElement('span'); + injected.id = 'wj-frag-injected'; + injected.textContent = 'client-only'; + container.querySelector('#wj-frag-links').appendChild(injected); + injected.wjLive = 'injected-expando'; + + stamped = document.createElement('span'); + stamped.id = 'wj-frag-stamp'; + stamped.textContent = 'stamp'; + container.querySelector('#wj-frag-links').appendChild(stamped); + stamped.wjLive = 'stamp-expando'; + + fetched = []; + origFetch = window.fetch; + window.fetch = (u, init) => { + const url = String(typeof u === 'string' ? u : (u && u.url) || u); + const headers = (init && init.headers) || {}; + fetched.push((headers['x-webjs-frame'] ? 'frame:' : 'page:') + url); + return Promise.resolve(new Response(pageResponse(), { + headers: { 'content-type': 'text/html', 'x-webjs-build': '' }, + })); + }; + + fallbacks = []; + document.addEventListener('webjs:navigation-fallback', onFallback); + + // What `enableClientRouter()` does on a real load. Stated here so the case + // does not inherit whatever url the ambient router was last enabled at. + _setCurrentPageUrl(location.href); + window.scrollTo({ top: 0, left: 0, behavior: 'instant' }); + } + + function onFallback(e) { fallbacks.push(e.detail && e.detail.cause); } + + async function teardown() { + document.removeEventListener('webjs:navigation-fallback', onFallback); + if (origFetch) window.fetch = origFetch; + // Let anything in flight finish so it cannot swap during a later case. + for (let i = 0; i < 4; i++) await frame(); + disableClientRouter(); + _setCurrentPageUrl(null); + // Restore the EXACT url the page was served at, session query string and + // all. A fragment click pushes a real entry, so this is never a no-op. + if (origUrl && location.href !== origUrl) history.replaceState(null, '', origUrl); + if (container) container.remove(); + document.documentElement.style.scrollBehavior = origScrollBehavior; + window.scrollTo({ top: 0, left: 0, behavior: 'instant' }); + navGuard.remove(); + enableClientRouter(); + } + + /** Click from PAGE context. Never a harness click, which scrolls first. */ + function clickIt(id) { document.getElementById(id).click(); } + + /** How far the target is from the top of the viewport. */ + function targetTop() { + return document.getElementById('wj-frag-target').getBoundingClientRect().top; + } + + /** Drive a real traversal and wait for the router's handler to have run. */ + async function traverse(go) { + const popped = new Promise((r) => window.addEventListener('popstate', r, { once: true })); + go(); + await popped; + await settle(); + } + + test('a named-fragment click issues no fetch and leaves the live DOM alone', async () => { + setup(); + try { + // Assert the precondition, so a fixture that never scrolled cannot pass + // for the wrong reason. + assert.ok(targetTop() > 100, 'the target starts well below the viewport top'); + + clickIt('wj-named'); + await settle(); + + assert.deepEqual(fetched, [], 'a fragment jump needs no server response'); + assert.ok(injected.isConnected, 'the injected node must survive: this is the defect'); + assert.equal(injected.wjLive, 'injected-expando', 'and keep its live state'); + assert.equal(document.getElementById('wj-frag-injected'), injected, + 'the SAME node object, not a re-rendered copy'); + assert.equal(stamped.wjLive, 'stamp-expando'); + assert.ok(Math.abs(targetTop()) <= 2, 'the browser jumped the viewport to the target'); + assert.equal(new URL(location.href).hash, '#wj-frag-target'); + assert.deepEqual(fallbacks, [], 'a same-document jump is not a degradation'); + } finally { await teardown(); } + }); + + test('a bare href="#" scrolls to top natively and issues no fetch', async () => { + setup(); + try { + window.scrollTo({ top: START_Y, left: 0, behavior: 'instant' }); + assert.equal(window.scrollY, START_Y, 'the fixture is tall enough to scroll'); + + clickIt('wj-bare'); + await settle(); + + assert.deepEqual(fetched, [], 'the back-to-top idiom is not a navigation'); + assert.equal(window.scrollY, 0, 'the browser scrolled to the document element'); + assert.ok(location.href.endsWith('#'), 'the empty fragment is in the url'); + assert.ok(injected.isConnected, 'nothing was swapped, so the injected node stands'); + assert.deepEqual(fallbacks, []); + } finally { await teardown(); } + }); + + test('clicking the SAME fragment link twice still fetches nothing', async () => { + setup(); + try { + // The second click is the one that matters. Navigating to the url the + // page is already on REPLACES rather than pushes, and it still fires + // popstate, so it reaches the handler with `location.href` unchanged. + // A guard that required the two hrefs to differ read that as "not a + // fragment traversal" and fell through to a full navigation, which put + // the whole defect back on the second click of a back-to-top link. + clickIt('wj-named'); + await settle(); + assert.deepEqual(fetched, [], 'precondition: the first click is already handled'); + const entriesAfterFirst = history.length; + + clickIt('wj-named'); + await settle(); + + assert.deepEqual(fetched, [], 'the repeat click must not navigate either'); + assert.ok(injected.isConnected, 'and must not re-swap the live DOM'); + assert.equal(injected.wjLive, 'injected-expando'); + assert.equal(document.getElementById('wj-frag-injected'), injected, + 'the SAME node object, so nothing was re-rendered'); + assert.equal(history.length, entriesAfterFirst, + 'a repeat fragment click replaces rather than pushing'); + assert.deepEqual(fallbacks, []); + } finally { await teardown(); } + }); + + test('clicking a bare href="#" twice still fetches nothing', async () => { + setup(); + try { + // The same shape on the idiom a reader actually clicks repeatedly. + window.scrollTo({ top: START_Y, left: 0, behavior: 'instant' }); + clickIt('wj-bare'); + await settle(); + assert.deepEqual(fetched, [], 'precondition'); + + window.scrollTo({ top: START_Y, left: 0, behavior: 'instant' }); + clickIt('wj-bare'); + await settle(); + + assert.deepEqual(fetched, [], 'back to top, twice, is still not a navigation'); + assert.equal(window.scrollY, 0, 'and it still scrolls to the top'); + assert.ok(injected.isConnected); + assert.deepEqual(fallbacks, []); + } finally { await teardown(); } + }); + + test('href="" is NOT a fragment jump and still navigates', async () => { + setup(); + try { + // This is what keeps the empty-fragment decision narrow. `href=""` + // resolves to the current url with the fragment REMOVED, which the spec + // reloads rather than jumps, so the router must still handle it. + clickIt('wj-empty'); + await settle(); + + assert.equal(fetched.length, 1, 'an empty href carries no fragment, so it navigates'); + assert.ok(fetched[0].startsWith('page:'), 'and it is a page navigation'); + } finally { await teardown(); } + }); + + test('Back after a fragment click is a real traversal and still re-renders', async () => { + setup(); + try { + // Deliberately NOT absorbed, and this documents why. A Back between two + // fragment states looks identical to the Back out of a 422 re-render, + // which must re-render: `getSubmitAction` prefers the raw `action` + // ATTRIBUTE, which carries no fragment, so a bound-submitter form + // declaring `action="/p"` pushes its 422 entry at `/p` while the reader + // sits at `/p#sec`, and the two differ only by fragment. Separating them + // needs to know whether the DOM was replaced between the two ENTRIES, + // which is per-entry state the router does not keep. + // + // So the CLICK is fixed and the traversal is left exactly as it behaves + // without this fix. Absorbing it here would swallow that validation-error + // Back, which is strictly worse than re-rendering one fragment step. + clickIt('wj-named'); + await settle(); + assert.deepEqual(fetched, [], 'precondition: the click itself was absorbed'); + + await traverse(() => history.back()); + + assert.equal(fetched.length, 1, 'a traversal with no click behind it still re-renders'); + assert.ok(fetched[0].startsWith('page:')); + } finally { await teardown(); } + }); + + test('a data-no-router in-page anchor is left to the browser, every click', async () => { + setup(); + try { + // `data-no-router` opts out of ROUTING, and the fragment bow-out routes + // nothing either way, but the browser still performs the native jump and + // still fires the popstate. Since the mark is now the ONLY thing that + // absorbs one, checking that attribute before the bow-out would leave + // every click of such an anchor unmarked, first and repeat alike, and + // each would be re-navigated destructively. This is the coverage for that + // ordering in `events.js`. + assert.ok(targetTop() > 100, 'the target starts below the viewport top'); + + clickIt('wj-noroute'); + await settle(); + assert.deepEqual(fetched, [], 'first click: absorbed on the mark'); + assert.ok(Math.abs(targetTop()) <= 2, 'and the browser jumped natively'); + + clickIt('wj-noroute'); + await settle(); + assert.deepEqual(fetched, [], 'repeat click: absorbed on the mark too'); + + assert.ok(injected.isConnected, 'the live DOM is untouched throughout'); + assert.equal(document.getElementById('wj-frag-injected'), injected); + assert.deepEqual(fallbacks, []); + } finally { await teardown(); } + }); + + test('a genuine cross-document popstate still re-navigates', async () => { + setup(); + try { + // A real cross-document Back, end to end in a browser: the click + // navigates for real and the traversal back re-renders. It carries no + // mark (the click ran `performNavigation`, which clears one), so it takes + // the same path as any traversal. This is the whole-journey version of + // the unmarked unit cases, not a proof about url comparison: the absorber + // returns on the mark alone, before it compares anything. + clickIt('wj-other'); + await settle(); + assert.equal(fetched.length, 1, 'precondition: the click navigated'); + + await traverse(() => history.back()); + + assert.equal(fetched.length, 2, 'a changed search is a real traversal'); + assert.ok(fetched[1].startsWith('page:')); + } finally { await teardown(); } + }); + + test('a fragment click inside a does not drive a frame nav', async () => { + setup(); + try { + // The documented trap, inverted. The click bow-out runs BEFORE frame + // resolution, so the anchor never reaches `resolveTargetFrameId` and the + // browser performs its own jump, frame or no frame. + assert.ok(targetTop() > 100, 'the target starts below the viewport top'); + + clickIt('wj-in-frame'); + await settle(); + + assert.deepEqual(fetched, [], 'no page fetch and no frame fetch'); + assert.ok(Math.abs(targetTop()) <= 2, 'the window moved, natively'); + assert.equal(document.getElementById('wj-frame-content').textContent, 'ORIGINAL', + 'the frame was never swapped'); + assert.ok(injected.isConnected); + assert.deepEqual(fallbacks, []); + } finally { await teardown(); } + }); +}); diff --git a/packages/core/test/routing/browser/nav-guard.test.js b/packages/core/test/routing/browser/nav-guard.test.js index fa5daf4ef..28e01482f 100644 --- a/packages/core/test/routing/browser/nav-guard.test.js +++ b/packages/core/test/routing/browser/nav-guard.test.js @@ -23,7 +23,7 @@ */ import { html } from '../../../src/html.js'; import { render } from '../../../src/render-client.js'; -import { enableClientRouter } from '../../../src/router-client.js'; +import { enableClientRouter, _setCurrentPageUrl } from '../../../src/router-client.js'; import { assert } from '../../../../../test/browser-assert.js'; import { installNavGuard } from '../../../../../test/browser-nav-guard.js'; @@ -37,6 +37,12 @@ suite('Browser-test nav guard (#1135)', () => { enableClientRouter(); // idempotent; ensures the document listeners are attached guard = installNavGuard(); origHref = location.href; + // Seed the router's current-page tracker, the way `enableClientRouter()` + // does on a real load. Teardown puts the URL back but cannot put this back, + // so without it a case inherits whatever url the PREVIOUS case navigated + // to, and anything reading the tracker (the fragment bow-out, #1437) then + // compares against a page this one never visited. + _setCurrentPageUrl(location.href); container = document.createElement('div'); // A live keyed boundary pair (#1015) so an intercepted nav swaps softly // rather than degrading, which the guard could not block. @@ -65,8 +71,10 @@ suite('Browser-test nav guard (#1135)', () => { bClose.remove(); guard.remove(); // A committed soft nav pushState'd a fake URL. Put the runner's own URL - // back so it does not leak into the next test or file. + // back so it does not leak into the next test or file, and clear the + // tracker with it so the pair cannot drift apart. history.replaceState(null, '', origHref); + _setCurrentPageUrl(null); } /** Resolve when the router settles, so teardown never runs mid-swap. */ @@ -195,6 +203,38 @@ suite('Browser-test nav guard (#1135)', () => { } finally { teardown(); } }); + test('does NOT cancel a same-document fragment link, so the browser jumps (#1437)', async () => { + setup(); + try { + // The guard cancels an anchor's default so a lost interception race + // cannot navigate the session away. A same-document fragment link has no + // such risk (the page never unloads), and cancelling it suppresses the + // browser's own jump, which is the behaviour the router's fragment + // bow-out exists to preserve. So the guard has to let this one through. + const frag = new URL(location.href); + frag.hash = 'nav-guard-frag-target'; + render(html` +
spacer
+ target +
spacer
+ jump + `, container); + const target = container.querySelector('#nav-guard-frag-target'); + assert.ok(target.getBoundingClientRect().top > 100, 'the target starts below the fold'); + + container.querySelector('a').click(); + await tick(); + + assert.ok(Math.abs(target.getBoundingClientRect().top) <= 2, + 'the guard must leave a same-document fragment jump to the browser'); + assert.deepEqual(fetched, [], 'and the router must not have navigated it either'); + } finally { + history.replaceState(null, '', location.href.split('#')[0]); + window.scrollTo({ top: 0, left: 0, behavior: 'instant' }); + teardown(); + } + }); + test('does NOT suppress the router on a plain form submission', async () => { setup(); try { diff --git a/packages/core/test/routing/router-client.test.js b/packages/core/test/routing/router-client.test.js index e6cea4dc6..cbcae4a60 100644 --- a/packages/core/test/routing/router-client.test.js +++ b/packages/core/test/routing/router-client.test.js @@ -2366,6 +2366,32 @@ test('a second navigation closes an open scroll-anchor window (#1310)', async () } }); +test('disableClientRouter drops a pending fragment mark (#1437)', async () => { + // Pure module state, so it belongs at this layer rather than the browser one: + // no DOM, no layout, no history traversal is involved in the assertion, and + // this is the node suite CI's unit gate runs. Sibling of the scroll-anchor + // teardown below, which is the same obligation for a different piece of + // pending state. + // + // The failure it guards: a mark left by a click whose popstate has not fired + // yet survives the router stepping aside, then absorbs the first same-url + // popstate after a re-enable, which is the swallowed-Back this PR exists to + // prevent. + const { _pendingFragmentNav, clearFragmentNav, markFragmentNav } = + await import('../../src/router-client/state.js'); + try { + assert.equal(_pendingFragmentNav, null, 'precondition: nothing pending'); + markFragmentNav('http://localhost/p#x'); + const live = await import('../../src/router-client/state.js'); + assert.equal(live._pendingFragmentNav, 'http://localhost/p#x', 'precondition: the mark is set'); + disableClientRouter(); + assert.equal(live._pendingFragmentNav, null, 'disable must drop the mark'); + } finally { + clearFragmentNav(); + enableClientRouter(); + } +}); + test('disableClientRouter closes an open scroll-anchor window (#1310)', async () => { // The router must leave nothing of its own on after it is disabled. const origLoc = globalThis.location; @@ -3085,17 +3111,238 @@ test('onPopState: triggers a router navigation to location.href', async () => { { status: 200, headers: { 'content-type': 'text/html' } } ); }; + const prevPageUrl = _currentPageUrl(); + // Set the tracker explicitly rather than inheriting whatever a previous test + // left behind. The fragment bow-out below reads it, so a test that asserts a + // navigation must say which page it is navigating away from. + _setCurrentPageUrl('http://localhost/before-pop'); try { document.body.innerHTML = 'before'; _onPopState({}); await new Promise((r) => setTimeout(r, 10)); assert.equal(fetched, 'http://localhost/popped'); } finally { + _setCurrentPageUrl(prevPageUrl); globalThis.location = origLoc; globalThis.fetch = origFetch; } }); +/* ==================================================================== + * onPopState: the popstate a fragment CLICK produces is not a navigation (#1437) + * + * The DECISION is pure, so it belongs here. The observable half (no swap, + * live DOM survives, the viewport lands on the anchor) cannot be asserted + * in linkedom, which implements no layout, no scrolling and no history + * traversal, and lives in the browser layer at + * `packages/core/test/routing/browser/fragment-jump.test.js`. + * ==================================================================== */ + +/** + * Drive one popstate against a stubbed location and report whether the router + * fetched. Mirrors the stubbing the popstate tests above use. + * + * @param {string} trackerUrl What the router believes is the current page. + * @param {string} poppedUrl Where the browser has already moved location to. + * @param {{ viaFragmentClick?: string }} [opts] When set, stands in for the + * click the router bowed out of, marking that href the way `onClick` does. + * That mark is the only thing separating a repeat anchor click from a real + * traversal to the same url (#1437). + * @returns {Promise<{fetched: boolean, tracker: string | null}>} + */ +async function popTo(trackerUrl, poppedUrl, opts) { + const origLoc = globalThis.location; + const origFetch = globalThis.fetch; + const prevPageUrl = _currentPageUrl(); + const u = new URL(poppedUrl); + let fetched = false; + globalThis.location = /** @type {any} */ ({ + href: u.href, pathname: u.pathname, origin: u.origin, search: u.search, hash: u.hash, + }); + globalThis.fetch = async () => { + fetched = true; + return new Response( + '' + + 'popped' + + '', + { status: 200, headers: { 'content-type': 'text/html' } } + ); + }; + _setCurrentPageUrl(trackerUrl); + const { clearFragmentNav, markFragmentNav } = await import('../../src/router-client/state.js'); + clearFragmentNav(); + if (opts && opts.viaFragmentClick) markFragmentNav(opts.viaFragmentClick); + try { + document.body.innerHTML = 'before'; + _onPopState({}); + await new Promise((r) => setTimeout(r, 10)); + return { fetched, tracker: _currentPageUrl() }; + } finally { + _setCurrentPageUrl(prevPageUrl); + globalThis.location = origLoc; + globalThis.fetch = origFetch; + } +} + +test('onPopState: a fragment-only popstate does not navigate (#1437)', async () => { + const { fetched } = await popTo('http://localhost/p', 'http://localhost/p#x', + { viaFragmentClick: 'http://localhost/p#x' }); + assert.equal(fetched, false, 'the click the router bowed out of must not re-fetch'); +}); + +test('onPopState: a traversal with no click behind it still navigates (#1437)', async () => { + // An ordinary Back or Forward between two fragment states is NOT absorbed. + // It looks identical to the Back out of a 422 re-render, which must + // re-render, and separating them needs to know whether the DOM was replaced + // between the two ENTRIES. That is per-entry state the router does not keep, + // since every `pushState` here passes `null`. So this stays on the normal + // path, exactly as it behaves without this fix. + const { fetched } = await popTo('http://localhost/p#x', 'http://localhost/p'); + assert.equal(fetched, true, 'no click behind it, so it is a real traversal'); +}); + +test('onPopState: a FRAGMENTLESS same-url popstate still navigates (#1437)', async () => { + // Two DISTINCT entries can share a url, and the framework's own no-JS write + // path makes that pair: a bound `` emits no `action` + // attribute, so `getSubmitAction` falls back to `location.href` and the 422 + // re-render pushes a duplicate entry at the page's own url. Back from that + // validation error must keep falling through, so the cache branch's + // background revalidation can swap the fresh render in. This is the case an + // over-broad guard swallows, and the reader then has to press Back twice. + const { fetched } = await popTo('http://localhost/p', 'http://localhost/p'); + assert.equal(fetched, true, 'a fragmentless same-url traversal is still a navigation'); +}); + +test('onPopState: the REPEAT click of one anchor is absorbed (#1437)', async () => { + // That navigation REPLACES its entry rather than pushing, and it still fires + // popstate, so it arrives with `location.href` equal to what the tracker + // holds (measured in Chromium: two clicks of one `#sec` link give two + // popstates at the same href, `history.length` unchanged). An early version + // required the hrefs to DIFFER, which read as the conservative choice and + // instead let the second click of a `Back to top` fall + // through to a full navigation. + const { fetched } = await popTo('http://localhost/p#x', 'http://localhost/p#x', + { viaFragmentClick: 'http://localhost/p#x' }); + assert.equal(fetched, false, 'the router saw this click, so there is nothing to navigate to'); +}); + +test('onPopState: an identical-url popstate the router did NOT cause navigates (#1437)', async () => { + // Same urls as the case above, opposite answer, and the ONLY difference is + // provenance. This is the 422 duplicate entry: no click was bowed out of, so + // the popstate is a real traversal. A predicate that keyed on the url alone + // could not tell these two apart, which is what sank two earlier attempts. + const { fetched } = await popTo('http://localhost/p#x', 'http://localhost/p#x'); + assert.equal(fetched, true, 'no mark means a real traversal, which must re-render'); +}); + +test('onPopState: a fragment mark is consumed once, not left armed (#1437)', async () => { + const first = await popTo('http://localhost/p#x', 'http://localhost/p#x', + { viaFragmentClick: 'http://localhost/p#x' }); + assert.equal(first.fetched, false, 'precondition: the marked popstate is absorbed'); + const { _pendingFragmentNav } = await import('../../src/router-client/state.js'); + assert.equal(_pendingFragmentNav, null, 'the mark must not survive its popstate'); +}); + +test('onPopState: a mark that MISSES is dropped, not left armed (#1437)', async () => { + // The clear-on-miss half, which needs a mark whose href is not the one that + // pops. Without it a stale mark sits waiting to absorb an unrelated same-url + // popstate later, which is the swallowed-Back failure in slow motion. + const missed = await popTo('http://localhost/p', 'http://localhost/p#x', + { viaFragmentClick: 'http://localhost/p#SOMETHING-ELSE' }); + assert.equal(missed.fetched, true, 'a mark for a DIFFERENT href does not absorb this one'); + const { _pendingFragmentNav } = await import('../../src/router-client/state.js'); + assert.equal(_pendingFragmentNav, null, 'a mark that did not match must still be dropped'); + + // And prove the drop matters: the very next same-url popstate, which the + // stale mark would have absorbed, still navigates. + const next = await popTo('http://localhost/p#SOMETHING-ELSE', 'http://localhost/p#SOMETHING-ELSE'); + assert.equal(next.fetched, true, 'a stale mark must not absorb a later real traversal'); +}); + +test('performNavigation and performSubmission each drop a pending mark (#1437)', async () => { + // Both clears guard the same shape: a fragment click leaves a mark, real work + // starts, and the popstate a later Back produces must NOT inherit it. The + // submission one guards this PR's own headline case, the 422 duplicate entry. + const { _pendingFragmentNav, markFragmentNav } = await import('../../src/router-client/state.js'); + const origLoc = globalThis.location; + const origFetch = globalThis.fetch; + const prevPageUrl = _currentPageUrl(); + globalThis.location = /** @type any */ ({ + href: 'http://localhost/p', pathname: '/p', origin: 'http://localhost', search: '', hash: '', + }); + globalThis.fetch = async () => new Response( + 'x', + { status: 200, headers: { 'content-type': 'text/html' } }); + try { + document.body.innerHTML = 'before'; + + markFragmentNav('http://localhost/p#x'); + await navigate('http://localhost/p?nav=1'); + const { _pendingFragmentNav: afterNav } = await import('../../src/router-client/state.js'); + assert.equal(afterNav, null, 'performNavigation must drop a pending mark'); + + markFragmentNav('http://localhost/p#x'); + const form = document.createElement('form'); + form.setAttribute('method', 'post'); + form.setAttribute('action', 'http://localhost/p'); + document.body.appendChild(form); + const { performSubmission } = await import('../../src/router-client/navigator.js'); + await performSubmission('http://localhost/p', 'POST', new URLSearchParams(), null, form); + const { _pendingFragmentNav: afterSubmit } = await import('../../src/router-client/state.js'); + assert.equal(afterSubmit, null, 'performSubmission must drop a pending mark'); + form.remove(); + } finally { + _setCurrentPageUrl(prevPageUrl); + globalThis.location = origLoc; + globalThis.fetch = origFetch; + } +}); + +test('onPopState: an unmarked popstate on a different PATH navigates (#1437)', async () => { + // Reachable behaviour, and the shape a real cross-document Back has: no mark, + // because no click was bowed out of. The absorber returns on the mark alone, + // before it parses anything. + const { fetched } = await popTo('http://localhost/p', 'http://localhost/other'); + assert.equal(fetched, true, 'a different document must still be fetched'); +}); + +test('onPopState: an unmarked popstate on a different SEARCH navigates (#1437)', async () => { + // Same, for the query. A different search is a different server response, and + // this is the unit-layer pin for it. + const { fetched } = await popTo('http://localhost/p?a=1', 'http://localhost/p?a=2'); + assert.equal(fetched, true, 'a different query is a different server response'); +}); + +test('onPopState: the tracker cross-check rejects a mark from another page (#1437)', async () => { + // Exercises the DEFENSIVE branch in `absorbFragmentClickPopState`, which is + // unreachable today: `onClick` marks only when the anchor's pathname and + // search already match `location`, and every writer of `currentPageUrl` + // clears the mark first. So this drives it through `_setCurrentPageUrl` + // rather than through anything production can do, and it exists to keep the + // branch honest for whoever adds a fourth writer of that tracker. + const path = await popTo('http://localhost/p', 'http://localhost/other#x', + { viaFragmentClick: 'http://localhost/other#x' }); + assert.equal(path.fetched, true, 'a mark for another path must not absorb'); + const search = await popTo('http://localhost/p?a=1', 'http://localhost/p?a=2#x', + { viaFragmentClick: 'http://localhost/p?a=2#x' }); + assert.equal(search.fetched, true, 'nor a mark for another query'); +}); + +test('onPopState: an absorbed fragment traversal records the new url (#1437)', async () => { + // Regression test for the failure the first attempted patch actually showed: + // a bow-out that returns without recording leaves the tracker at the pre-jump + // url, so the REVERSE traversal compares two equal hrefs and re-navigates. + const { tracker } = await popTo('http://localhost/p', 'http://localhost/p#x', + { viaFragmentClick: 'http://localhost/p#x' }); + assert.equal(tracker, 'http://localhost/p#x'); +}); + +test('onPopState: the empty fragment is absorbed too (#1437)', async () => { + const { fetched } = await popTo('http://localhost/p', 'http://localhost/p#', + { viaFragmentClick: 'http://localhost/p#' }); + assert.equal(fetched, false, 'an empty fragment is still a fragment'); +}); + /* ==================================================================== * revalidate: snapshot-cache invalidation * ==================================================================== */ @@ -3887,6 +4134,14 @@ test('eligibleAnchorHref: rejects a pure same-page hash jump', async () => { await withPrefetchEnv(() => { // location is /, so /#foo is a same-page hash and must not prefetch. assert.equal(_eligibleAnchorHref(mkAnchor('http://localhost/#foo')), null); + // The EMPTY fragment is a fragment too (#1437). It serializes with `hash` + // === '' exactly like a url carrying no fragment at all, so testing `hash` + // here missed it and the back-to-top idiom stayed prefetch-eligible. + assert.equal(_eligibleAnchorHref(mkAnchor('http://localhost/#')), null, 'bare #'); + // And the pin for the other half: `href=""` resolves to the current url + // with the fragment REMOVED, which the spec reloads rather than jumping, so + // it is still a navigation and must stay eligible. + assert.equal(_eligibleAnchorHref(mkAnchor('http://localhost/')), 'http://localhost/', 'no fragment'); }); }); diff --git a/test/browser-nav-guard.js b/test/browser-nav-guard.js index 8e5a06aa9..47f601a7f 100644 --- a/test/browser-nav-guard.js +++ b/test/browser-nav-guard.js @@ -1,5 +1,21 @@ import { _setHardNavigate } from '../packages/core/src/router-client.js'; +/** + * Whether this href is a same-document fragment jump, which the guard leaves + * alone (see the header note). Mirrors the router's own bow-out predicate in + * `packages/core/src/router-client/events.js`. + * + * @param {string} href Absolute, as `HTMLAnchorElement.href` always is. + * @returns {boolean} + */ +function isSameDocumentFragment(href) { + let url; + try { url = new URL(href); } catch { return false; } + if (url.origin !== location.origin) return false; + if (url.pathname !== location.pathname || url.search !== location.search) return false; + return url.href.includes('#'); +} + /** * Shared navigation guard for browser tests (#1135). Sibling of * `test/browser-assert.js` (#777) and with the same "one source of truth for @@ -18,8 +34,24 @@ import { _setHardNavigate } from '../packages/core/src/router-client.js'; * instead of taking down the run. * * It is opt-in PER SUITE, not global, so a new suite that clicks a real link or - * submits a real form has to install it. A pure-fragment `href="#x"` link needs - * no guard, since it never navigates the page away. + * submits a real form has to install it. + * + * ## What it deliberately does NOT cancel: a same-document fragment link + * + * A link whose origin, pathname and query match the page it sits on, and which + * carries a fragment, does not navigate the page away: the browser scrolls to + * the target and fires `popstate`, and the session survives. So there is + * nothing here to guard against, and cancelling it does real harm, because + * `preventDefault` is exactly what suppresses that native jump. A suite testing + * the router's own fragment bow-out (#1437) would then see no jump at all and + * could not tell a working bow-out from a broken one. + * + * The test is `href`-based rather than `hash`-based for the same reason the + * router's is: the URL serializer reports a null fragment and an EMPTY one + * identically as `''`, and `href="#"` is a real fragment navigation, to the + * document element. `href=""` carries no fragment at all, resolves to the + * current url with the fragment removed, and the spec RELOADS it, so it is a + * genuine session risk and stays guarded. * * ## The phase is load-bearing: `window`, BUBBLE, never capture * @@ -85,7 +117,11 @@ export function installNavGuard() { // than the thing it backstops. const path = typeof e.composedPath === 'function' ? e.composedPath() : []; for (const el of path) { - if (el instanceof HTMLAnchorElement && el.hasAttribute('href')) { e.preventDefault(); return; } + if (el instanceof HTMLAnchorElement && el.hasAttribute('href')) { + if (isSameDocumentFragment(el.href)) return; + e.preventDefault(); + return; + } } }; diff --git a/website/app/docs/client-router/page.ts b/website/app/docs/client-router/page.ts index 84b4fccc9..f6f6b3c40 100644 --- a/website/app/docs/client-router/page.ts +++ b/website/app/docs/client-router/page.ts @@ -112,8 +112,8 @@ export default async function PostPage({ params }) {

A frame swap never scrolls the page

A page navigation scrolls to top, the way a browser does. A frame swap does not: it changes one region and leaves the rest of the document standing, the reader's scroll offset included. Without that, filtering a panel below the fold would throw the reader back to the top of the page, with the panel they just clicked in off screen. The rule covers every way a frame swaps, a nested link, an external data-webjs-frame trigger, a frame-targeted form submission, and a src self-load, and it covers a #hash on a frame link too, which rides the URL without moving the viewport.

-

One thing this rule does NOT cover, because the router never sees it: a pure fragment link to a named anchor (#section), whose path and query match the page it sits on. The click handler bows out before preventDefault, so the browser performs its own native fragment jump and the window does move.

-

The empty fragment is the trap, and it goes the other way. href="#" (and href="") parse to an empty hash, and the bow-out tests that hash for truthiness, so it does not fire. The click becomes an ordinary frame navigation, which re-fetches the frame and, under this rule, leaves the window still. So a bare <a href="#">Back to top</a> placed INSIDE a frame does nothing visible. Give it a real target (href="#top"), which the bow-out honours, or a click handler that scrolls, whose preventDefault runs first and makes the router stand down.

+

One thing this rule does NOT cover, because the router never sees it: a pure fragment link whose path and query match the page it sits on. The click handler bows out before preventDefault, so the browser performs its own native fragment jump and the window does move.

+

Every spelling of a fragment link is the browser's, the bare # included. href="#" is the back-to-top idiom, and it serializes with an empty fragment that reads identically to no fragment at all through URL.hash, so the bow-out tests the href for a # instead. A <a href="#">Back to top</a> therefore scrolls to top natively, inside a frame as well as outside one. href="" is not a fragment link at all: it resolves to the current url with the fragment removed, which the spec reloads rather than jumps, so the router navigates it like any other link.

Read "never scrolls" as "WebJs never writes a scroll", not as a promise the viewport cannot move. A swap that makes the panel shorter shortens the document with it, and a reader parked near the bottom is then holding an offset the document can no longer reach, so the browser clamps it. On the gallery's frames demo, filtering from All to Done at the bottom of the page moves the window from 474 to 405, exactly the 69px the document lost. The router writes no scroll there, and any DOM change that shortens a page does the same. Keeping the frame a stable height across its states avoids it.

The escapes are page navigations and DO scroll to top: data-webjs-frame="_top", and an id that cannot be matched to a live frame, which warns and degrades to a normal navigation. Do not read that second one as covering a response that lacks the requested frame (the webjs:frame-missing warning): there the frame resolved and the navigation stayed frame-scoped, so the offset holds and only the panel is left unchanged. Turbo's autoscroll opt-in, which scrolls the frame itself into view on swap, has no WebJs equivalent; the router simply never writes scroll for a frame.

@@ -188,6 +188,8 @@ connectWS('/posts/' + id + '/feed', { onMessage: (m) => renderStream(m) });Snapshot cache + back/forward

The router maintains a URL-keyed LRU cache of page snapshots (capacity 16). On back/forward via popstate, the cached DOM is applied instantly and the captured window-scroll position is restored. A background refetch then revalidates the snapshot quietly.

+

The popstate an in-page fragment click produces is the exception, and it restores nothing. The browser has already done the jump, so the router absorbs it and leaves live DOM identity and hydrated component state alone. That covers a repeat click of one anchor too, which replaces its history entry rather than pushing a new one and still fires popstate with the URL unchanged.

+

The gate is which popstate the router caused, not how the URL looks: it marks the click it bowed out of and the next popstate consumes that mark. A Back between two entries differing only by fragment can still need a re-render, because a form's raw action attribute carries no fragment, so a bound-submitter form declaring action="/p" pushes its 422 re-render at /p while the reader sits at /p#sec. So an ordinary Back or Forward between two fragment states still re-renders.

A back/forward restore reserves the page's recorded HEIGHT across the swap, then suppresses the browser's scroll anchoring (overflow-anchor) for the restore's duration, then puts both back. The reservation is what makes the saved offset reachable: the snapshot's markup is briefly shorter than the page it was serialized from, because its components have not rendered yet, so without it the browser clamps the restore to whatever the short document allowed and the reader lands short. The suppression covers the other half: content still SHIFTS above the viewport as those components render, and anchoring would add that shift to the offset just replayed, landing the reader below where they left. The window closes on the first real input (wheel, touchmove, keydown, pointerdown), so a reader who starts scrolling mid-restore gets normal anchoring back immediately. It also closes as soon as another page navigation starts, since the window outlives its own restore and must not be inherited by a page it was never meant for. Absent either of those it closes once the restore is over, which is the later of that restore's background revalidation settling and a short floor, and at the latest on a 2s ceiling. The floor matters because waiting on the revalidation alone would tie the window to network latency rather than to the growth it guards. Suppression only withholds a browser correction, it never moves the viewport. The height reservation releases on the same settle and ceiling, and on a superseding navigation, but deliberately NOT on user input: releasing the page's height under a reader mid-scroll is the one harm an early release could do. If your app sets overflow-anchor or an inline min-height on the root itself, each is saved and put back.

A frame-targeted navigation or submission is the one exception to the closing rule above: it swaps a single <webjs-frame> rather than the page, so the restored offset is still the right one and the restore is left running. Frame targeting here means what it means everywhere else, so a trigger that breaks out with data-webjs-frame="_top", or names an id that does not resolve, is a page navigation and does close the window.

The browser restores Back/Forward scroll, not the router. WebJs FORCES history.scrollRestoration to auto when the router starts, overriding an app that had set 'manual' (and putting that value back if you call disableClientRouter()), so setting it to 'manual' yourself does not take effect while the router is running. Only under auto does the browser record a scroll position per history entry, replay it on a traverse, and compose the iOS edge back-swipe gesture preview from that recording. The router writes no scroll of its own on a restore: it reserves the recorded height across the swap so the browser's replay lands on a document that can hold the offset, and that is the entire mechanism. One writer, which is the model Next and Remix 3 use as well. Taking manual control suppresses the recording, so every scrolled page previews blank for the whole duration of the gesture; that is a real bug WebJs shipped, inherited from Turbo Drive's assumeControlOfScrollRestoration, and it is why Turbo still has it (Turbo is single-writer too, but its writer is the app rather than the browser).