From e3a4593acf6e16d2bfc14f5cef21b66ece842eb9 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 29 Oct 2026 00:47:52 +0100 Subject: [PATCH] fix(nav): Back looks for a moved item only in the page's lists, once its own list has had its chance, and its reveal stops the scroll restore The cross-list match took a same-named row of the app-wide download tray, and took it at the first look while a virtualized list had yet to render the item. The reveal of a moved item ran before the scroll restore's first frame, which put the old offset back over it. The pending restore's name is readable, for a page that folds rows away. --- .../components/ui/PageScroller.dom.test.tsx | 33 +++++++ frontend/src/components/ui/PageScroller.tsx | 5 ++ .../src/lib/listNav.restoreMoved.dom.test.tsx | 85 ++++++++++++++++--- frontend/src/lib/listNav.ts | 52 ++++++++++-- 4 files changed, 155 insertions(+), 20 deletions(-) diff --git a/frontend/src/components/ui/PageScroller.dom.test.tsx b/frontend/src/components/ui/PageScroller.dom.test.tsx index 04317e5b..a4c199a2 100644 --- a/frontend/src/components/ui/PageScroller.dom.test.tsx +++ b/frontend/src/components/ui/PageScroller.dom.test.tsx @@ -17,6 +17,7 @@ vi.mock("react-i18next", async (importOriginal) => { import PageScroller from "./PageScroller"; import BackToTop from "./BackToTop"; import { notifyNav } from "../../lib/history"; +import { revealedNavItem } from "../../lib/listNav"; beforeAll(() => { // useScrollFade wires a ResizeObserver (watch-the-children); jsdom ships none. @@ -125,6 +126,38 @@ describe("PageScroller scroll memory", () => { expect(main.scrollTop).toBe(450); }); + // Back focuses an item the restore did not expect — one that moved to another list while the page + // was away — and scrolls it into view: the restore's next frame put the old offset back over it. + it("gives the page up to an item brought into view while it restores", () => { + let frames: FrameRequestCallback[] = []; + vi.stubGlobal("requestAnimationFrame", (cb: FrameRequestCallback) => frames.push(cb)); + const flush = (n: number) => { + for (let i = 0; i < n; i++) { + const run = frames; + frames = []; + for (const cb of run) cb(0); + } + }; + setEntry(1); + const { container } = render( + +
+ , + ); + const main = sizedMain(container); + main.scrollTop = 450; + fireEvent.scroll(main); + setEntry(2); + notifyNav(); + flush(1); + setEntry(1); + notifyNav(); // Back: the restore to 450 waits for its first frame + main.scrollTop = 120; // the item scrolled into view + revealedNavItem(container.querySelector("#moved")!); + flush(5); + expect(main.scrollTop).toBe(120); + }); + // Landing at the end shows Back to top, whose spacer then brings the spot within reach. it("finishes the way to a spot the spacer brings within reach after it lands", () => { let frames: FrameRequestCallback[] = []; diff --git a/frontend/src/components/ui/PageScroller.tsx b/frontend/src/components/ui/PageScroller.tsx index 66eb92c0..1f2fc90d 100644 --- a/frontend/src/components/ui/PageScroller.tsx +++ b/frontend/src/components/ui/PageScroller.tsx @@ -12,6 +12,7 @@ import { StickyTopProvider } from "./stickyTop"; import { useBackToTopShown, useCornerLift } from "./backToTopState"; import { CONTENT_ID } from "./landmarks"; import { ensureNavId, peekNavId, subscribeNav } from "../../lib/history"; +import { NAV_REVEAL_EVENT } from "../../lib/listNav"; // Saved scroll offsets, keyed by `${historyEntryId}:${scrollKey}`. The entry id ties an offset to the // exact history entry, so Back restores where you left it and a forward navigation (a new entry) opens @@ -63,11 +64,15 @@ function restoreScroll(el: HTMLElement, top: number): () => void { el.removeEventListener("wheel", stop); el.removeEventListener("touchstart", stop); el.removeEventListener("pointerdown", stop); + el.removeEventListener(NAV_REVEAL_EVENT, stop); document.removeEventListener("keydown", stop, true); }; el.addEventListener("wheel", stop, { passive: true }); el.addEventListener("touchstart", stop, { passive: true }); el.addEventListener("pointerdown", stop); + // So does an item the keyboard brought into view (`lib/listNav`): Back focusing one that is not + // where the restored offset shows. + el.addEventListener(NAV_REVEAL_EVENT, stop); document.addEventListener("keydown", stop, true); const tick = () => { if (cancelled) return; diff --git a/frontend/src/lib/listNav.restoreMoved.dom.test.tsx b/frontend/src/lib/listNav.restoreMoved.dom.test.tsx index 389f2124..b8ee90e7 100644 --- a/frontend/src/lib/listNav.restoreMoved.dom.test.tsx +++ b/frontend/src/lib/listNav.restoreMoved.dom.test.tsx @@ -9,13 +9,20 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { act, cleanup, render } from "@testing-library/react"; import { useRef } from "react"; -import { navItemAttrs, navPosition, restoreNavPosition, useListNav } from "./listNav"; +import { + NAV_REVEAL_EVENT, + navItemAttrs, + navPosition, + restoreNavPosition, + useListNav, +} from "./listNav"; afterEach(cleanup); -function List({ items }: { items: string[] }) { +// `count`: a virtualized list's, which counts rows it has yet to render. +function List({ items, count }: { items: string[]; count?: number }) { const ref = useRef(null); - useListNav(ref, { enabled: true }); + useListNav(ref, { enabled: true, count }); return (
{items.map((t, i) => ( @@ -27,12 +34,25 @@ function List({ items }: { items: string[] }) { ); } -const Page = ({ lists }: { lists: string[][] }) => ( -
- {lists.map((items, i) => ( - - ))} -
+// `tray`: a list of the app's own, outside the page (the running downloads), whose rows can share a +// name with the page's. +const Page = ({ + lists, + tray, + counts = [], +}: { + lists: string[][]; + tray?: string[]; + counts?: number[]; +}) => ( + <> +
+ {lists.map((items, i) => ( + + ))} +
+ {tray && } + ); const button = (name: string) => @@ -40,12 +60,17 @@ const button = (name: string) => const focused = () => document.activeElement?.closest("[data-nav-index]")?.textContent; const wait = (ms: number) => act(() => new Promise((r) => setTimeout(r, ms))); -async function leaveAndReturn(before: string[][], from: string, after: string[][]) { - const { unmount } = render(); +async function leaveAndReturn( + before: string[][], + from: string, + after: string[][], + tray?: string[], +) { + const { unmount } = render(); button(from).focus(); const pos = navPosition()!; unmount(); // the page switch - render(); // Back + render(); // Back const cancel = restoreNavPosition(pos); await wait(120); cancel(); @@ -78,15 +103,51 @@ describe("Back to an item that moved to another list", () => { // off screen. const reveal = vi.fn(); Element.prototype.scrollIntoView = reveal; + // ...and tells the page's scroll restore, which would otherwise put the old offset back over it. + const told = vi.fn(); + document.addEventListener(NAV_REVEAL_EVENT, told); await leaveAndReturn([["subscribed", "download"], ["older"]], "subscribed", [ ["download"], ["subscribed", "older"], ]); expect(reveal).toHaveBeenCalledTimes(1); + expect(told).toHaveBeenCalledTimes(1); cleanup(); reveal.mockClear(); + told.mockClear(); await leaveAndReturn([["subscribed", "download"]], "download", [["download", "subscribed"]]); expect(focused()).toBe("download"); expect(reveal).not.toHaveBeenCalled(); + expect(told).not.toHaveBeenCalled(); + document.removeEventListener(NAV_REVEAL_EVENT, told); + }); + + it("never takes it from a list outside the page: the download tray's row of that name", async () => { + // A feed card and its download in the tray are both named by the video's title. + await leaveAndReturn( + [["subscribed", "download"]], + "subscribed", + [["download"]], + ["subscribed"], + ); + expect(focused()).toBe("download"); + }); + + it("waits for its own list to render the place before looking in another one", async () => { + // A virtualized list puts its rows back a frame after the page: the item is still to come at + // the first look, and the one of that name in the rail below is not it. + const { unmount } = render(); + button("subscribed").focus(); + const pos = navPosition()!; + unmount(); + const { rerender } = render(); + const cancel = restoreNavPosition(pos); + await wait(60); + rerender(); + await wait(120); + cancel(); + expect(document.activeElement?.closest("main > div")).toBe( + document.querySelector("main")!.firstElementChild, + ); }); }); diff --git a/frontend/src/lib/listNav.ts b/frontend/src/lib/listNav.ts index ed1b241e..5b3e5df8 100644 --- a/frontend/src/lib/listNav.ts +++ b/frontend/src/lib/listNav.ts @@ -226,6 +226,14 @@ function itemName(el: HTMLElement): string { /** What a screen reader hears while the item holds the focus: its `label` when it gives one. */ const spokenName = (el: HTMLElement): string => el.getAttribute("data-nav-label") || itemName(el); +/** The event an item brought into view sends up its page: a scroll restore still under way gives the + * page up to it (`ui/PageScroller`) — on Back, an item found in another list than the one it was + * left in is not where the restored offset shows, and the restore's next frame scrolled it away. */ +export const NAV_REVEAL_EVENT = "navreveal"; +export const revealedNavItem = (el: HTMLElement): void => { + el.dispatchEvent(new Event(NAV_REVEAL_EVENT, { bubbles: true })); +}; + function focusItem(el: HTMLElement, reveal = true): void { if (roved !== el) { if (named) roved?.removeAttribute("role"); @@ -259,7 +267,10 @@ function focusItem(el: HTMLElement, reveal = true): void { el.focus({ preventScroll: true }); // `nearest` + the item's scroll margins: a visible item does not move, an item under the floating // header or past the bottom edge scrolls just far enough to clear it. - if (reveal) el.scrollIntoView?.({ block: "nearest" }); + if (reveal) { + el.scrollIntoView?.({ block: "nearest" }); + revealedNavItem(el); + } } function focusIndex( @@ -483,17 +494,24 @@ export function navPosition(): NavPosition | null { // look — on a timer, not animation frames: a hidden tab runs none. const RESTORE_MS = 2000; const RESTORE_POLL_MS = 50; +// The restore under way, while it looks for its item. +let pending: NavPosition | null = null; /** The item to hand the focus back to: in `list`, the saved place while it still holds the saved * name, else the nearest item of that name (names repeat — a log console's identical request * lines). Then the one item of that name in another of the page's lists: an item can change lists * while the page was away (a notification asked for the reader on the visit it was opened from and * sits in the stream on the next), and the place left behind is now its neighbour's. Only when the - * name is the page's alone — of two, either could be the one. Else the saved place. */ + * name is the page's alone — of two, either could be the one — and only the PAGE's lists: the + * running-downloads tray is the app's, on every page, and its row of a download is named like the + * feed card it came from. Not while `list` has yet to render a saved place it still counts (a + * virtualized list puts its rows back a frame after the page), until the `last` look: the item is + * likely still to come there. Else the saved place. */ function restoreTarget( all: readonly NavList[], list: NavList | undefined, pos: NavPosition, + last: boolean, ): HTMLElement | null { const at = list ? itemAt(list, pos.index) : null; if (list && (!pos.name || (at && itemName(at) === pos.name))) return at; @@ -504,7 +522,8 @@ function restoreTarget( const here = named(list).sort((a, b) => distance(a) - distance(b)); if (here[0]) return here[0]; } - const [only, ...more] = all.flatMap(named); + if (list && !at && pos.index < list.count() && !last) return null; + const [only, ...more] = all.filter((l) => l.el.closest("main")).flatMap(named); return only && more.length === 0 ? only : at; } @@ -522,8 +541,12 @@ function restoreTarget( export function restoreNavPosition(pos: NavPosition): () => void { const deadline = Date.now() + RESTORE_MS; let timer: ReturnType | undefined; + pending = pos; + const done = () => { + if (pending === pos) pending = null; + }; const attempt = () => { - if (!focusIsUnclaimed()) return; + if (!focusIsUnclaimed()) return done(); const all = ordered(); const list = pos.label ? all.find((l) => labelOf(l) === pos.label) @@ -536,20 +559,33 @@ export function restoreNavPosition(pos: NavPosition): () => void { // The saved place still holding the saved name wins, and among several items of that name the // nearest one: names repeat (a log console's `http.request GET /api/feed 200` lines, two // downloads of one video), and the first of them is rarely the one left behind. - const el = restoreTarget(all, list, pos); + const last = Date.now() + RESTORE_POLL_MS >= deadline; + const el = restoreTarget(all, list, pos, last); // Where the scroll restore put it — unless it is found in another list than the one it was left // in: the restored scroll is the old place's, and the item would hold the focus off screen. - if (el) return focusItem(el, !list?.el.contains(el)); - if (Date.now() < deadline) timer = setTimeout(attempt, RESTORE_POLL_MS); + if (el) { + done(); + return focusItem(el, !list?.el.contains(el)); + } + if (last) done(); + else timer = setTimeout(attempt, RESTORE_POLL_MS); }; // Not inside the popstate dispatch that landed: a sub-view's own listener (`useHistorySubview`) // may run after ours and has not switched the view yet — focus still sits in the view being left, // and "focus is already somewhere" would give up for good (measured in Chrome: // Plex show → Back left focus on ). One task later the view has rendered. timer = setTimeout(attempt, 0); - return () => clearTimeout(timer); + return () => { + clearTimeout(timer); + done(); + }; } +/** The name of the item a Back is still looking for, while it looks: a page that keeps some of its + * rows folded away (the notification center's Earlier) opens the fold holding it — a list it does + * not render is one the restore cannot find the item in. Read it as the page mounts. */ +export const pendingNavName = (): string | null => pending?.name ?? null; + /** Register `ref` as a keyboard-navigable list while `enabled`. A virtualized list must say how many * items it has (`count` — the loaded ones, for an infinite list), how many per row (`cols`) and how * to scroll an unmounted item into view (`reveal`). A list that mounts everything omits all three: