diff --git a/frontend/src/components/ui/Chip.dom.test.tsx b/frontend/src/components/ui/Chip.dom.test.tsx index ff30b4fb..b49344bb 100644 --- a/frontend/src/components/ui/Chip.dom.test.tsx +++ b/frontend/src/components/ui/Chip.dom.test.tsx @@ -3,7 +3,7 @@ // E28 S3b: the chip family (docs/DESIGN.md "One primitive per button family"). One geometry for a // filter, an applied narrowing, a tag and a suggestion, where five files had five. import { afterEach, describe, expect, it, vi } from "vitest"; -import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { act, cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; import { useState } from "react"; import { Chip, DismissibleChip } from "./Chip"; import { chipClass } from "./chipStyles"; @@ -70,6 +70,37 @@ describe("Chip", () => { expect(document.activeElement).toBe(screen.getByRole("button", { name: "Remove News" })); }); + // Round 3: the neighbour it hands the focus to can go in the same removal — the "Clear all" that + // only shows beside two filters. The focus then goes to the chip before, not to . + it("a removing chip whose neighbour goes with it hands the focus to the chip before", async () => { + function Row() { + const [chips, setChips] = useState(["Music", "News"]); + return ( +
+ {chips.map((c) => ( + setChips((all) => all.filter((x) => x !== c))} + > + {c} + + ))} + {chips.length > 1 && } +
+ ); + } + render(); + const news = screen.getByRole("button", { name: "Remove News" }); + act(() => news.focus()); + fireEvent.click(news, { detail: 0 }); + expect(screen.queryByRole("button", { name: "Clear all" })).toBeNull(); + await waitFor(() => + expect(document.activeElement).toBe(screen.getByRole("button", { name: "Remove Music" })), + ); + }); + it("the measuring mirror lays out exactly like the chip it measures", () => { render( diff --git a/frontend/src/components/ui/Chip.tsx b/frontend/src/components/ui/Chip.tsx index 1f9b0716..33049528 100644 --- a/frontend/src/components/ui/Chip.tsx +++ b/frontend/src/components/ui/Chip.tsx @@ -3,7 +3,7 @@ import type { ButtonHTMLAttributes, ReactNode } from "react"; import clsx from "clsx"; import { X } from "lucide-react"; import { FocusSafeButton } from "./FocusSafeButton"; -import { focusNeighbour } from "../../lib/useFocusKeeper"; +import { handOffFocus } from "../../lib/useFocusKeeper"; import { chipClass, type ChipSize, type ChipVariant } from "./chipStyles"; // The chip family (docs/DESIGN.md "One primitive per button family"): a small rounded-full token that @@ -61,8 +61,8 @@ export const Chip = forwardRef(function Chip( // went to (round 2, F6 — the active-filter chips of the feed and Plex). Every // removal chip gets it here, once, instead of each caller remembering. if (removeLabel && e.currentTarget === document.activeElement) - focusNeighbour(e.currentTarget); - onClick?.(e); + handOffFocus(e.currentTarget, () => onClick?.(e)); + else onClick?.(e); }} {...rest} > @@ -129,8 +129,8 @@ export function DismissibleChip({ // next chip (else the previous one, else what follows the row) first, not to (E28 // review F5). A mouse press on it leaves the focus where it was. const chip = e.currentTarget.parentElement; - if (chip && e.currentTarget === document.activeElement) focusNeighbour(chip); - onDismiss(); + if (chip && e.currentTarget === document.activeElement) handOffFocus(chip, onDismiss); + else onDismiss(); }} aria-label={dismissLabel} title={dismissLabel} diff --git a/frontend/src/components/ui/PageScroller.dom.test.tsx b/frontend/src/components/ui/PageScroller.dom.test.tsx index a0da1d53..6980d13f 100644 --- a/frontend/src/components/ui/PageScroller.dom.test.tsx +++ b/frontend/src/components/ui/PageScroller.dom.test.tsx @@ -125,6 +125,67 @@ describe("PageScroller scroll memory", () => { expect(main.scrollTop).toBe(450); }); + // Round 3: 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[] = []; + 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(x); + const main = sizedMain(container); + main.scrollTop = 500; + fireEvent.scroll(main); + setEntry(2); + notifyNav(); + flush(1); + Object.defineProperty(main, "scrollHeight", { configurable: true, value: 928 }); // no spacer + setEntry(1); + notifyNav(); + flush(12); + expect(main.scrollTop).toBe(428); + Object.defineProperty(main, "scrollHeight", { configurable: true, value: 1000 }); // spacer up + flush(1); + expect(main.scrollTop).toBe(500); + }); + + // Round 3: the give-up put the page at its end 1.5 s in, wherever the reader had scrolled to. + it("stops where the reader takes the page over, even for a spot it has not reached", () => { + 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(x); + const main = sizedMain(container); + main.scrollTop = 450; + fireEvent.scroll(main); + setEntry(2); + notifyNav(); + flush(1); + Object.defineProperty(main, "scrollHeight", { configurable: true, value: 600 }); // far short + setEntry(1); + notifyNav(); + flush(5); + fireEvent.wheel(main); + main.scrollTop = 30; // where the reader scrolled + flush(100); + expect(main.scrollTop).toBe(30); + Object.defineProperty(main, "scrollHeight", { configurable: true, value: 1000 }); // loaded + flush(1); + expect(main.scrollTop).toBe(30); + }); + it("restores per content key within one entry: a new key resets to top, returning restores", () => { setEntry(10); const { container, rerender } = render(x); diff --git a/frontend/src/components/ui/PageScroller.tsx b/frontend/src/components/ui/PageScroller.tsx index 92234d85..a4813094 100644 --- a/frontend/src/components/ui/PageScroller.tsx +++ b/frontend/src/components/ui/PageScroller.tsx @@ -31,7 +31,12 @@ function rememberScroll(key: string, top: number): void { // A spot the page can no longer reach — the last stretch of a page scrolled to its very end, whose // Back-to-top spacer only shows once scrolled — goes as far as the page does: it used to be left at // the top (round 2, F8). Only a spacer's worth short, and only once the height has held still, is -// that answer early; a list still loading is further short than that, and gets the full wait. +// that answer early; a list still loading is further short than that, and gets the full wait. And +// landing there is not the end of it: that scroll is what shows Back to top, whose spacer then +// makes the spot reachable, so the loop watches a few frames more and finishes the way (round 3). +// +// The reader scrolling meanwhile — a wheel, a finger, a key, the scrollbar — takes the page over: +// the restore stops where they are, instead of pulling them back or dropping them at the end. const SETTLED_FRAMES = 10; const SPACER_REM = 6; function restoreScroll(el: HTMLElement, top: number): () => void { @@ -43,27 +48,42 @@ function restoreScroll(el: HTMLElement, top: number): () => void { let frames = 0; let still = 0; let lastMax = -1; + let landedAt = -1; let cancelled = false; + const stop = () => { + cancelled = true; + cancelAnimationFrame(raf); + el.removeEventListener("wheel", stop); + el.removeEventListener("touchstart", stop); + el.removeEventListener("pointerdown", stop); + document.removeEventListener("keydown", stop, true); + }; + el.addEventListener("wheel", stop, { passive: true }); + el.addEventListener("touchstart", stop, { passive: true }); + el.addEventListener("pointerdown", stop); + document.addEventListener("keydown", stop, true); const tick = () => { if (cancelled) return; const max = el.scrollHeight - el.clientHeight; if (max >= top) el.scrollTop = top; - if (el.scrollTop >= top - 1) return; // reached - still = max === lastMax ? still + 1 : 0; - lastMax = max; - const rem = parseFloat(getComputedStyle(document.documentElement).fontSize) || 16; - const nearEnd = max > 0 && top - max <= SPACER_REM * rem; - if ((nearEnd && still >= SETTLED_FRAMES) || frames++ > 90) { - el.scrollTop = max; // as near as the page reaches now - return; + if (el.scrollTop >= top - 1) return stop(); // reached + if (landedAt >= 0) { + if (frames - landedAt > SETTLED_FRAMES) return stop(); // no spacer came: the end it is + } else { + still = max === lastMax ? still + 1 : 0; + lastMax = max; + const rem = parseFloat(getComputedStyle(document.documentElement).fontSize) || 16; + const nearEnd = max > 0 && top - max <= SPACER_REM * rem; + if ((nearEnd && still >= SETTLED_FRAMES) || frames > 90) { + el.scrollTop = max; // as near as the page reaches now + landedAt = frames; + } } + frames++; raf = requestAnimationFrame(tick); }; raf = requestAnimationFrame(tick); - return () => { - cancelled = true; - cancelAnimationFrame(raf); - }; + return stop; } // The page's scroll container — the ONE scroller in the app, owned by PageShell. The feed, the diff --git a/frontend/src/lib/useFocusKeeper.ts b/frontend/src/lib/useFocusKeeper.ts index 9f915742..5292d324 100644 --- a/frontend/src/lib/useFocusKeeper.ts +++ b/frontend/src/lib/useFocusKeeper.ts @@ -49,6 +49,27 @@ export function focusNeighbour(el: HTMLElement): boolean { return false; } +/** Run `remove`, which takes `el` off the page, without dropping `el`'s focus: it goes to the + * neighbour first (`focusNeighbour`). That neighbour may go in the same removal — the "Clear all" + * that only shows beside two filters, the "+1" of the last hidden one (E28 review round 3) — so + * once the removal has rendered, a focus left on goes to what preceded `el`, else to the + * first control left in its row. Only when the focus was lost: a focus the reader moved stays. */ +export function handOffFocus(el: HTMLElement, remove: () => void): void { + const before = firstFocusable(el.previousSibling, true); + const row = el.parentElement; + focusNeighbour(el); + remove(); + // A timer, not a frame: a page the browser is not painting runs no requestAnimationFrame. + window.setTimeout(() => { + const now = document.activeElement; + if (now && now !== document.body && now.isConnected) return; + const stayed = before?.isConnected && shown(before) ? before : null; + (stayed ?? (row?.isConnected ? firstFocusable(row.firstChild) : null))?.focus({ + preventScroll: true, + }); + }, 0); +} + // The element the keeper last put the focus on, while it still holds it. A Back that swaps the page // or a sub-view removes the held control too, and the keeper's stand-in then sat where // `restoreNavPosition` looks for an untouched : the restore gave up and Back left the focus on