From 7d45c4f59f9945412427dc68e3a1903e304bec06 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 24 Oct 2026 22:45:26 +0200 Subject: [PATCH] fix(keys): a list inside the topmost layer walks by keyboard The list keys are the page's and an open layer silences them, so the player's related videos could only be reached by Tab. A layer now names its root element; with the focus on a row inside the topmost one, j/k, the arrows and Enter walk that list, the walk stays inside the layer, and the player's seek and volume arrows leave a focused row alone. --- docs/DESIGN.md | 5 +- frontend/src/components/ui/Modal.tsx | 2 +- .../features/player/PlayerModal.dom.test.tsx | 70 ++++++++++++++ frontend/src/features/player/PlayerModal.tsx | 9 +- frontend/src/lib/keymap.ts | 9 +- frontend/src/lib/layerStack.ts | 20 +++- frontend/src/lib/listNav.dom.test.tsx | 29 ++++++ frontend/src/lib/listNav.ts | 92 ++++++++++++------- 8 files changed, 194 insertions(+), 42 deletions(-) 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.