fix(ui): a removed chip's focus outlives its neighbour; a scroll restore yields to the reader
A chip handed its focus to a Clear all or +N that went in the same removal, and the focus fell to the body. The restore now stops when the reader scrolls, and finishes the way once the Back-to-top spacer brings the saved spot within reach.
This commit is contained in:
@@ -3,7 +3,7 @@
|
|||||||
// E28 S3b: the chip family (docs/DESIGN.md "One primitive per button family"). One geometry for a
|
// 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.
|
// filter, an applied narrowing, a tag and a suggestion, where five files had five.
|
||||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
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 { useState } from "react";
|
||||||
import { Chip, DismissibleChip } from "./Chip";
|
import { Chip, DismissibleChip } from "./Chip";
|
||||||
import { chipClass } from "./chipStyles";
|
import { chipClass } from "./chipStyles";
|
||||||
@@ -70,6 +70,37 @@ describe("Chip", () => {
|
|||||||
expect(document.activeElement).toBe(screen.getByRole("button", { name: "Remove News" }));
|
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 <body>.
|
||||||
|
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 (
|
||||||
|
<div>
|
||||||
|
{chips.map((c) => (
|
||||||
|
<Chip
|
||||||
|
key={c}
|
||||||
|
variant="token"
|
||||||
|
removeLabel={`Remove ${c}`}
|
||||||
|
onClick={() => setChips((all) => all.filter((x) => x !== c))}
|
||||||
|
>
|
||||||
|
{c}
|
||||||
|
</Chip>
|
||||||
|
))}
|
||||||
|
{chips.length > 1 && <button onClick={() => setChips([])}>Clear all</button>}
|
||||||
|
</div>
|
||||||
|
);
|
||||||
|
}
|
||||||
|
render(<Row />);
|
||||||
|
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", () => {
|
it("the measuring mirror lays out exactly like the chip it measures", () => {
|
||||||
render(
|
render(
|
||||||
<Chip variant="tag" size="xs">
|
<Chip variant="tag" size="xs">
|
||||||
|
|||||||
@@ -3,7 +3,7 @@ import type { ButtonHTMLAttributes, ReactNode } from "react";
|
|||||||
import clsx from "clsx";
|
import clsx from "clsx";
|
||||||
import { X } from "lucide-react";
|
import { X } from "lucide-react";
|
||||||
import { FocusSafeButton } from "./FocusSafeButton";
|
import { FocusSafeButton } from "./FocusSafeButton";
|
||||||
import { focusNeighbour } from "../../lib/useFocusKeeper";
|
import { handOffFocus } from "../../lib/useFocusKeeper";
|
||||||
import { chipClass, type ChipSize, type ChipVariant } from "./chipStyles";
|
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
|
// 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<HTMLButtonElement, ChipProps>(function Chip(
|
|||||||
// went to <body> (round 2, F6 — the active-filter chips of the feed and Plex). Every
|
// went to <body> (round 2, F6 — the active-filter chips of the feed and Plex). Every
|
||||||
// removal chip gets it here, once, instead of each caller remembering.
|
// removal chip gets it here, once, instead of each caller remembering.
|
||||||
if (removeLabel && e.currentTarget === document.activeElement)
|
if (removeLabel && e.currentTarget === document.activeElement)
|
||||||
focusNeighbour(e.currentTarget);
|
handOffFocus(e.currentTarget, () => onClick?.(e));
|
||||||
onClick?.(e);
|
else onClick?.(e);
|
||||||
}}
|
}}
|
||||||
{...rest}
|
{...rest}
|
||||||
>
|
>
|
||||||
@@ -129,8 +129,8 @@ export function DismissibleChip({
|
|||||||
// next chip (else the previous one, else what follows the row) first, not to <body> (E28
|
// next chip (else the previous one, else what follows the row) first, not to <body> (E28
|
||||||
// review F5). A mouse press on it leaves the focus where it was.
|
// review F5). A mouse press on it leaves the focus where it was.
|
||||||
const chip = e.currentTarget.parentElement;
|
const chip = e.currentTarget.parentElement;
|
||||||
if (chip && e.currentTarget === document.activeElement) focusNeighbour(chip);
|
if (chip && e.currentTarget === document.activeElement) handOffFocus(chip, onDismiss);
|
||||||
onDismiss();
|
else onDismiss();
|
||||||
}}
|
}}
|
||||||
aria-label={dismissLabel}
|
aria-label={dismissLabel}
|
||||||
title={dismissLabel}
|
title={dismissLabel}
|
||||||
|
|||||||
@@ -125,6 +125,67 @@ describe("PageScroller scroll memory", () => {
|
|||||||
expect(main.scrollTop).toBe(450);
|
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(<PageScroller scrollKey="e:k1">x</PageScroller>);
|
||||||
|
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(<PageScroller scrollKey="e:k1">x</PageScroller>);
|
||||||
|
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", () => {
|
it("restores per content key within one entry: a new key resets to top, returning restores", () => {
|
||||||
setEntry(10);
|
setEntry(10);
|
||||||
const { container, rerender } = render(<PageScroller scrollKey="e10:a">x</PageScroller>);
|
const { container, rerender } = render(<PageScroller scrollKey="e10:a">x</PageScroller>);
|
||||||
|
|||||||
@@ -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
|
// 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
|
// 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
|
// 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 SETTLED_FRAMES = 10;
|
||||||
const SPACER_REM = 6;
|
const SPACER_REM = 6;
|
||||||
function restoreScroll(el: HTMLElement, top: number): () => void {
|
function restoreScroll(el: HTMLElement, top: number): () => void {
|
||||||
@@ -43,27 +48,42 @@ function restoreScroll(el: HTMLElement, top: number): () => void {
|
|||||||
let frames = 0;
|
let frames = 0;
|
||||||
let still = 0;
|
let still = 0;
|
||||||
let lastMax = -1;
|
let lastMax = -1;
|
||||||
|
let landedAt = -1;
|
||||||
let cancelled = false;
|
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 = () => {
|
const tick = () => {
|
||||||
if (cancelled) return;
|
if (cancelled) return;
|
||||||
const max = el.scrollHeight - el.clientHeight;
|
const max = el.scrollHeight - el.clientHeight;
|
||||||
if (max >= top) el.scrollTop = top;
|
if (max >= top) el.scrollTop = top;
|
||||||
if (el.scrollTop >= top - 1) return; // reached
|
if (el.scrollTop >= top - 1) return stop(); // reached
|
||||||
still = max === lastMax ? still + 1 : 0;
|
if (landedAt >= 0) {
|
||||||
lastMax = max;
|
if (frames - landedAt > SETTLED_FRAMES) return stop(); // no spacer came: the end it is
|
||||||
const rem = parseFloat(getComputedStyle(document.documentElement).fontSize) || 16;
|
} else {
|
||||||
const nearEnd = max > 0 && top - max <= SPACER_REM * rem;
|
still = max === lastMax ? still + 1 : 0;
|
||||||
if ((nearEnd && still >= SETTLED_FRAMES) || frames++ > 90) {
|
lastMax = max;
|
||||||
el.scrollTop = max; // as near as the page reaches now
|
const rem = parseFloat(getComputedStyle(document.documentElement).fontSize) || 16;
|
||||||
return;
|
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);
|
||||||
};
|
};
|
||||||
raf = requestAnimationFrame(tick);
|
raf = requestAnimationFrame(tick);
|
||||||
return () => {
|
return stop;
|
||||||
cancelled = true;
|
|
||||||
cancelAnimationFrame(raf);
|
|
||||||
};
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// The page's scroll container — the ONE scroller in the app, owned by PageShell. The feed, the
|
// The page's scroll container — the ONE scroller in the app, owned by PageShell. The feed, the
|
||||||
|
|||||||
@@ -49,6 +49,27 @@ export function focusNeighbour(el: HTMLElement): boolean {
|
|||||||
return false;
|
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 <body> 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
|
// 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
|
// or a sub-view removes the held control too, and the keeper's stand-in then sat where
|
||||||
// `restoreNavPosition` looks for an untouched <body>: the restore gave up and Back left the focus on
|
// `restoreNavPosition` looks for an untouched <body>: the restore gave up and Back left the focus on
|
||||||
|
|||||||
Reference in New Issue
Block a user