diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 10420072..041f49d5 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -498,7 +498,10 @@ of a full-width panel and nothing on the right. ## Interaction rules — _Decided_ - **Keyboard:** everything works by keyboard, in visual order, with no traps. A shortcut is a row in - `lib/keymap.ts`; list and grid navigation is `NavGrid`/`keyboardNav`. [WCAG 2.1.1, 2.4.3, 2.1.2] + `lib/keymap.ts`; list and grid navigation is `NavGrid`/`keyboardNav`. An open dialog or player + silences the page's list keys, but a list inside the topmost one walks while the focus is in it + (the player's related videos); the walk stays inside that layer, and its arrows there move the list, + not the video. [WCAG 2.1.1, 2.4.3, 2.1.2] - **Focus:** every control has a visible focus ring (the base ring in `index.css`; don't add a second). A sticky or fixed layer never covers the focused element. A list item the keyboard lands on rings 2 px outside itself where it has a gap round it (a card); a table row, a log line or a row flush in a diff --git a/frontend/src/components/ui/Modal.tsx b/frontend/src/components/ui/Modal.tsx index d3babaf4..4d3d5a94 100644 --- a/frontend/src/components/ui/Modal.tsx +++ b/frontend/src/components/ui/Modal.tsx @@ -147,7 +147,7 @@ export default function Modal({ // An effect event, so a new handler each render never re-registers: the stack id is assigned once per mount. const close = useEffectEvent(onClose); useEffect(() => { - const id = pushLayer(); + const id = pushLayer({ root: () => rootRef.current }); const onKey = (e: KeyboardEvent) => { if (takeEscape(e, id)) { e.stopPropagation(); diff --git a/frontend/src/features/player/PlayerModal.dom.test.tsx b/frontend/src/features/player/PlayerModal.dom.test.tsx index 3fee4b11..748e1b80 100644 --- a/frontend/src/features/player/PlayerModal.dom.test.tsx +++ b/frontend/src/features/player/PlayerModal.dom.test.tsx @@ -61,7 +61,11 @@ const base = { getDuration: () => clock.duration, getVideoData: () => null, loadVideoById: (o: { videoId: string; startSeconds: number }) => void loaded.push(o), + seekTo: (s: number) => void seeks.push(s), + setVolume: (v: number) => void volumes.push(v), }; +const seeks: number[] = []; +const volumes: number[] = []; // Anything else the modal calls on the player (volume, rate, destroy…) is a no-op. Not `then`: // a thenable would be swallowed by the first `await` that touched it. const fakePlayer = new Proxy(base, { @@ -100,6 +104,8 @@ afterEach(() => { vi.restoreAllMocks(); started.length = 0; loaded.length = 0; + seeks.length = 0; + volumes.length = 0; }); const lastStart = () => started[started.length - 1]; @@ -814,6 +820,70 @@ describe("PlayerModal — the side panel on a wide window", () => { expect(screen.getByRole("button", { name: "player.back" })).toBeTruthy(); }); + // The list keys are the page's, silenced while a layer is up — but this list is IN the layer. + const press = (key: string) => + act(() => void fireEvent.keyDown(document.activeElement ?? document.body, { key })); + const rowOf = (el: Element | null) => el?.closest("[data-nav-index]"); + + it("wide: the keyboard walks the related list, and its arrows are not the video's", async () => { + wideWindow(true); + vi.spyOn(api, "videoDetail").mockReturnValue(new Promise(() => {})); + vi.spyOn(api, "related").mockResolvedValue({ + items: [related("rel1"), related("rel2")], + has_more: false, + next_cursor: null, + }); + const A = vid("A", 0); + renderModal( + {}} onState={() => {}} />, + ); + act(() => playerEvents.onReady?.()); + fireEvent.click(screen.getByRole("tab", { name: "player.related.title" })); + const first = titleOf(await screen.findAllByRole("link", { name: "Video rel1" })); + const second = titleOf(screen.getAllByRole("link", { name: "Video rel2" })); + act(() => first.focus()); + // (jsdom lays nothing out, so the rows read as one row: ←→ step them, ↑↓ have no row to go to.) + press("ArrowRight"); + expect(document.activeElement).toBe(rowOf(second)); + press("ArrowLeft"); + expect(document.activeElement).toBe(rowOf(first)); + press("j"); + expect(document.activeElement).toBe(rowOf(second)); + press("k"); + press("j"); + // None of the arrows reached the video: no seek, no volume step. + press("ArrowUp"); + press("ArrowDown"); + expect(seeks).toEqual([]); + expect(volumes).toEqual([]); + // Enter on the row plays it, like its title's click. + press("Enter"); + expect(loaded).toEqual([{ videoId: "rel2", startSeconds: 0 }]); + }); + + it("stacked: the keyboard walks the list in the drawer too", async () => { + wideWindow(false); + vi.spyOn(api, "videoDetail").mockReturnValue(new Promise(() => {})); + vi.spyOn(api, "related").mockResolvedValue({ + items: [related("rel1"), related("rel2")], + has_more: false, + next_cursor: null, + }); + const A = vid("A", 0); + renderModal( + {}} onState={() => {}} />, + ); + act(() => playerEvents.onReady?.()); + fireEvent.click(screen.getByRole("button", { name: "player.related.title" })); + const drawer = await screen.findByRole("dialog", { name: "player.related.title" }); + const first = titleOf(await within(drawer).findAllByRole("link", { name: "Video rel1" })); + act(() => first.focus()); + press("j"); + expect(document.activeElement).toBe( + rowOf(titleOf(within(drawer).getAllByRole("link", { name: "Video rel2" }))), + ); + }); + it("stacked: a button opens the related videos in a drawer, and playing one closes it", async () => { wideWindow(false); vi.spyOn(api, "videoDetail").mockReturnValue(new Promise(() => {})); diff --git a/frontend/src/features/player/PlayerModal.tsx b/frontend/src/features/player/PlayerModal.tsx index 3eeb531d..51bb6d24 100644 --- a/frontend/src/features/player/PlayerModal.tsx +++ b/frontend/src/features/player/PlayerModal.tsx @@ -50,6 +50,7 @@ import { anyLayerAbove, escapeHandled, } from "../../lib/layerStack"; +import { focusInNavList } from "../../lib/listNav"; import { trapTab, useDialogChrome } from "../../components/ui/dialogFocus"; import DownloadButton from "../downloads/DownloadButton"; import ClipPanel, { ClipCropFrame, type ClipCrop } from "./ClipPanel"; @@ -709,10 +710,11 @@ export default function PlayerModal({ // over the player by doing so. const overlayOrder = useOverlayOrder(); // This player is a dismissable layer: register it so a Modal or popover opened ABOVE it (e.g. the - // download dialog, or an anchored menu) takes Escape first and this player defers. + // download dialog, or an anchored menu) takes Escape first and this player defers. Its root lets + // the keyboard walk a list inside it (the related videos). const layerIdRef = useRef(null); useEffect(() => { - const id = pushLayer(); + const id = pushLayer({ root: () => dialogRef.current }); layerIdRef.current = id; return () => { removeLayer(id); @@ -740,7 +742,9 @@ export default function PlayerModal({ // there: deferring would scroll something invisible and leave the viewer with no volume control at // all. Both flags, not just fullscreen: since F stopped taking the browser's lock, maximise is the // common case and checking only `isFullscreen` made the keys dead exactly where they matter most. + // On a row of a list in the player (the related videos) the arrows walk the list instead. const volume = (e: KeyboardEvent, up: boolean) => { + if (focusInNavList()) return; const covered = isFullscreenRef.current || maximized; if (!covered && canScrollBy(document.activeElement as HTMLElement | null, up ? -1 : 1)) return; @@ -748,6 +752,7 @@ export default function PlayerModal({ nudgeVolume(up ? 5 : -5); }; const seek = (e: KeyboardEvent, secs: number) => { + if (focusInNavList()) return; e.preventDefault(); nudgeSeek(secs); }; diff --git a/frontend/src/lib/keymap.ts b/frontend/src/lib/keymap.ts index ca7ad049..26878ffc 100644 --- a/frontend/src/lib/keymap.ts +++ b/frontend/src/lib/keymap.ts @@ -25,7 +25,8 @@ import type { Page } from "./urlState"; * `page`, `search` and `list` are the PAGE-LEVEL scopes: they act on the page underneath * every overlay, so `dispatch` keeps them quiet while a layer that owns the keyboard is open. That is * also what keeps them clear of the players' bare letters — the Plex player's `s` and `k` only ever - * run with the player's own layer up. */ + * run with the player's own layer up. The one exception is a list INSIDE the topmost layer, with the + * focus in it (`dispatch`'s `inTopLayer`, decided by lib/listNav). */ export type KeyScope = | "global" | "page" @@ -358,11 +359,13 @@ const stepsOf = (chord: string): string[] => chord.split(" "); * implementation cannot drift without tsc saying so. Skipped while typing unless the row opts in * with `allowInInput` (Escape, and only Escape), on a key a focused widget already handled, and — for a page-level scope — while a layer that * owns the keyboard is open. The rule lives here rather than in each listener so a new page-level - * surface cannot forget it. */ + * surface cannot forget it. `inTopLayer` is the one way past it: the caller has checked that the + * key acts inside that topmost layer, not on the page behind (a list in the player). */ export function dispatch( e: KeyboardEvent, scope: S, handlers: { readonly [K in ShortcutId]: (e: KeyboardEvent) => void }, + { inTopLayer = false }: { inTopLayer?: boolean } = {}, ): boolean { if (sequenceEvents.has(e)) return false; // A key a focused widget already handled is not a shortcut: every listener here is on `window`, @@ -374,7 +377,7 @@ export function dispatch( if (e.key === "Escape" && escapeHandled(e)) return false; const live = liveSequence(e); if (live && live.scope !== scope) return false; - const quiet = isPageLevelScope(scope) && anyBlockingLayer(); + const quiet = isPageLevelScope(scope) && anyBlockingLayer() && !inTopLayer; const typing = isTypingTarget(e); const run = (id: string) => (handlers as Record void>)[id]?.(e); diff --git a/frontend/src/lib/layerStack.ts b/frontend/src/lib/layerStack.ts index 439c9aaa..f86e27f8 100644 --- a/frontend/src/lib/layerStack.ts +++ b/frontend/src/lib/layerStack.ts @@ -26,12 +26,21 @@ const passive = new Set(); // none, and Tab from anything outside the prompt walked into the page behind the scrim. The two // questions are separate; a layer answers each for itself. const defers = new Set(); +// The element a layer draws into, for the layers that hold content of their own to navigate (a +// dialog, the player): a list inside the TOPMOST such layer is walked by the keyboard although a +// layer is open — it is the one the user is looking at, not the page behind. +const roots = new Map HTMLElement | null>(); -export function pushLayer(opts?: { passive?: boolean; trapsTab?: boolean }): number { +export function pushLayer(opts?: { + passive?: boolean; + trapsTab?: boolean; + root?: () => HTMLElement | null; +}): number { const id = nextId++; stack.push(id); if (opts?.passive) passive.add(id); if (opts?.trapsTab === false) defers.add(id); + if (opts?.root) roots.set(id, opts.root); return id; } @@ -39,6 +48,15 @@ export function removeLayer(id: number): void { stack = stack.filter((x) => x !== id); passive.delete(id); defers.delete(id); + roots.delete(id); +} + +/** The element of the topmost layer that owns the keyboard — null while none is open, or while that + * layer gave none (a menu: its own keys move inside it). */ +export function topBlockingLayerRoot(): HTMLElement | null { + const blocking = stack.filter((id) => !passive.has(id)); + const top = blocking[blocking.length - 1]; + return top === undefined ? null : (roots.get(top)?.() ?? null); } /** True while a layer that owns the keyboard is open — a modal, a player, a click-opened menu. The diff --git a/frontend/src/lib/listNav.dom.test.tsx b/frontend/src/lib/listNav.dom.test.tsx index 9df0b364..1256aa5f 100644 --- a/frontend/src/lib/listNav.dom.test.tsx +++ b/frontend/src/lib/listNav.dom.test.tsx @@ -202,6 +202,35 @@ describe("useListNav", () => { removeLayer(tooltip); }); + // A list INSIDE the topmost layer (the player's related videos) is the one the user is in. + it("walks a list inside the topmost layer, and only inside it", () => { + render( + <> + +
+ +
+ , + ); + const dialog = pushLayer({ root: () => screen.getByTestId("layer") }); + act(() => screen.getByTestId("b0-action").focus()); + press("j"); + expect(focused()).toBe("b1"); + press("k"); + press("k"); // its first row: the walk stops there, not on the page's list behind + expect(focused()).toBe("b0"); + // A menu opened over it moves with its own keys: the list stays still. + const menu = pushLayer(); + press("j"); + expect(focused()).toBe("b0"); + removeLayer(menu); + // Focus left behind on the page's list does not walk it while the layer is up. + act(() => screen.getByTestId("a0-action").focus()); + press("j"); + expect(focused()).toBe("a0-action"); + removeLayer(dialog); + }); + // A virtualized grid only mounts a screen of rows: the next item may not exist yet. it("asks the list to reveal an unmounted item, then focuses it once it renders", async () => { const reveal = vi.fn(); diff --git a/frontend/src/lib/listNav.ts b/frontend/src/lib/listNav.ts index daf261f1..a0db0539 100644 --- a/frontend/src/lib/listNav.ts +++ b/frontend/src/lib/listNav.ts @@ -1,5 +1,6 @@ import { useEffect, useEffectEvent, useState, type RefObject } from "react"; import { dispatch, useKeyScope } from "./keymap"; +import { anyBlockingLayer, topBlockingLayerRoot } from "./layerStack"; import { scrollParent } from "./listPaging"; import { focusIsUnclaimed } from "./useFocusKeeper"; @@ -73,11 +74,15 @@ function itemAt(list: NavList, index: number): HTMLElement | null { } function ordered(): NavList[] { + // Inside an open layer (the player) the walk stays in it: `k` on its first row must not land on + // the feed behind the scrim. + const layer = anyBlockingLayer() ? topBlockingLayerRoot() : null; return ( [...lists.values()] // Only lists on screen: a list can be mounted yet hidden (display:none in a collapsed panel or // an inactive tab), and `j` must not walk into rows nobody can see. .filter((l) => l.el.isConnected && l.el.checkVisibility?.() !== false) + .filter((l) => !layer || layer.contains(l.el)) .sort((a, b) => a.el.compareDocumentPosition(b.el) & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1, ) @@ -92,6 +97,10 @@ function current(): { list: NavList; index: number; item: HTMLElement } | null { return item && list ? { list, index: indexOf(item), item } : null; } +/** True while the focus is on an item of a keyboard-walked list (or a control inside one): its arrows + * move between the items, so a surface that also binds them (the player's seek) leaves them alone. */ +export const focusInNavList = (): boolean => current() !== null; + /** The mounted items that belong to `el` itself (not to a list nested inside it), in index order. */ function ownItems(el: HTMLElement): HTMLElement[] { return [...el.querySelectorAll(`[${INDEX}]`)] @@ -325,42 +334,57 @@ function onKey(e: KeyboardEvent): void { const arrow = (dx: -1 | 0 | 1, dy: -1 | 0 | 1) => (ev: KeyboardEvent) => { if (move(dx, dy)) ev.preventDefault(); }; - dispatch(e, "list", { - next: (ev) => { - ev.preventDefault(); - step(1); + // The list keys act on the page, so an open layer silences them — unless the focus is in a list + // INSIDE the topmost layer: that list is what the user is in (the player's related videos). + const layer = topBlockingLayerRoot(); + const cur = current(); + const inTopLayer = !!layer && !!cur && layer.contains(cur.item); + dispatch( + e, + "list", + { + next: (ev) => { + ev.preventDefault(); + step(1); + }, + prev: (ev) => { + ev.preventDefault(); + step(-1); + }, + left: arrow(-1, 0), + right: arrow(1, 0), + up: arrow(0, -1), + down: arrow(0, 1), + // Only on the item ROOT: Enter on a button inside the card must stay that button's Enter. + open: (ev) => { + const cur = current(); + if (!cur || document.activeElement !== cur.item) return; + const target = cur.item.querySelector("[data-nav-open]"); + if (!target) return; + ev.preventDefault(); + // A real click on the item's own "open" element, so Enter runs exactly the click path — not a + // second copy of what opening means that could drift from it. + target.click(); + }, }, - prev: (ev) => { - ev.preventDefault(); - step(-1); + { inTopLayer }, + ); + dispatch( + e, + "listDelete", + { + // On the item root, like Enter — and only in a list that registered a remover. + remove: (ev) => { + const cur = current(); + const remove = cur?.list.remove(); + if (!cur || !remove || document.activeElement !== cur.item) return; + ev.preventDefault(); + remove(cur.index); + refocusSlot(cur.list, cur.index, cur.item); + }, }, - left: arrow(-1, 0), - right: arrow(1, 0), - up: arrow(0, -1), - down: arrow(0, 1), - // Only on the item ROOT: Enter on a button inside the card must stay that button's Enter. - open: (ev) => { - const cur = current(); - if (!cur || document.activeElement !== cur.item) return; - const target = cur.item.querySelector("[data-nav-open]"); - if (!target) return; - ev.preventDefault(); - // A real click on the item's own "open" element, so Enter runs exactly the click path — not a - // second copy of what opening means that could drift from it. - target.click(); - }, - }); - dispatch(e, "listDelete", { - // On the item root, like Enter — and only in a list that registered a remover. - remove: (ev) => { - const cur = current(); - const remove = cur?.list.remove(); - if (!cur || !remove || document.activeElement !== cur.item) return; - ev.preventDefault(); - remove(cur.index); - refocusSlot(cur.list, cur.index, cur.item); - }, - }); + { inTopLayer }, + ); } // How long a removal gets to take its row out of the DOM before the focus gives up following it.