From 3e7c79fa69adc5f9a9e5f327b4fe451aa8924cad Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 26 Sep 2026 20:01:04 +0200 Subject: [PATCH] fix(design): the keyboard ring of a flush row stays visible (E28) A list item's ring sat 2 px outside it. On a table row it ran under the sticky head and past the table's sides; on a Plex playlist show header, flush in its clipping card, it was cut away and the focus was invisible. A table row (by its tag), a log line and the Plex show header and episode rows now ring inside themselves, the show header with its card's corners. The sticky header's fade-below was drawn at rest, over the first row's top 16 px, hiding the ring's top edge: it now fades in with the first 1rem of scroll (a scroll-driven animation; no fade where unsupported). --- frontend/src/components/LogLine.tsx | 2 +- .../PlexPlaylistView.menu.dom.test.tsx | 3 ++ frontend/src/components/PlexPlaylistView.tsx | 10 +++-- frontend/src/index.css | 30 +++++++++++++ frontend/src/lib/fadeEdge.contract.test.ts | 21 +++++++++ frontend/src/lib/listNav.ts | 11 ++++- frontend/src/lib/navRing.contract.test.ts | 45 +++++++++++++++++++ 7 files changed, 116 insertions(+), 6 deletions(-) create mode 100644 frontend/src/lib/navRing.contract.test.ts diff --git a/frontend/src/components/LogLine.tsx b/frontend/src/components/LogLine.tsx index 8a150e14..fca3dded 100644 --- a/frontend/src/components/LogLine.tsx +++ b/frontend/src/components/LogLine.tsx @@ -38,7 +38,7 @@ export default function LogLine({ return (
{ expect(document.activeElement).toBe(rowOf("The Show")); pressKey("j"); expect(document.activeElement).toBe(rowOf("Ep e1")); + // Both sit flush in the show's clipping card: an outside ring was cut away (index.css). + expect(rowOf("The Show").hasAttribute("data-nav-inset")).toBe(true); + expect(rowOf("Ep e1").hasAttribute("data-nav-inset")).toBe(true); pressKey("k"); const bulk = vi.spyOn(api, "plexPlaylistRemoveBulk").mockResolvedValue({} as never); pressKey("Delete"); diff --git a/frontend/src/components/PlexPlaylistView.tsx b/frontend/src/components/PlexPlaylistView.tsx index 828a0bfc..0f9a7ccd 100644 --- a/frontend/src/components/PlexPlaylistView.tsx +++ b/frontend/src/components/PlexPlaylistView.tsx @@ -178,7 +178,7 @@ function EpisodeRow({ ep, ctx, dense }: { ep: PlexCard; ctx: Ctx; dense: boolean ref={setNodeRef} style={style} {...ctx.trigger(target)} - {...navItemAttrs(ctx.navIndex(ep.id), ep.title)} + {...navItemAttrs(ctx.navIndex(ep.id), ep.title, { inset: true })} className={`group flex items-center gap-2.5 ${dense ? "rounded-lg px-1.5 py-1" : `${SURFACE.card} p-1.5`}`} > { expect(copies).toEqual([]); }); }); + +// A sticky header's fade shows only once content has slid under it. Drawn at rest it lay over the +// first row's top 16 px, dimming it and hiding the top edge of the row's keyboard ring (E28, seen +// live). jsdom runs no scroll timeline, so what is pinned is the recipe: resting opacity 0, and the +// scroll-driven fade-in over the first 1rem, only where the engine has scroll timelines. +describe("the sticky header's fade", () => { + const css = readFileSync(new URL("../index.css", import.meta.url), "utf-8"); + const at = css.indexOf("@utility fade-below {"); + const body = css.slice(at, css.indexOf("\n}\n", at)); + + it("rests invisible and fades in with the first 1rem of scroll", () => { + expect(at, "the utility").toBeGreaterThan(0); + expect(body).toMatch(/opacity:\s*0;/); + expect(body).toMatch(/@supports \(animation-timeline: scroll\(\)\)/); + expect(body).toMatch(/animation-timeline:\s*scroll\(nearest block\)/); + expect(body).toMatch(/animation-range:\s*0 1rem/); + expect(css).toMatch( + /@keyframes fade-edge-in \{\s*from \{\s*opacity: 0;\s*\}\s*to \{\s*opacity: 1;/, + ); + }); +}); diff --git a/frontend/src/lib/listNav.ts b/frontend/src/lib/listNav.ts index 32f00ca2..b32bfd82 100644 --- a/frontend/src/lib/listNav.ts +++ b/frontend/src/lib/listNav.ts @@ -50,13 +50,20 @@ const REVEAL_FRAMES = 10; /** Spread onto an item's ROOT element. `name` is what a screen reader calls the item while the keyboard * is on it (its title) — pass it where the element Enter opens wraps the whole card (a Plex poster: * badges, year, rating and summary run together), shows a caption that is not the item's name, or - * holds no text at all (deep-review E11 round 3). */ + * holds no text at all (deep-review E11 round 3). + * + * `inset` draws the keyboard ring INSIDE the item. The ring sits 2 px outside by default, which a + * card with a gap round it has room for; an item flush against its neighbours (a log line) or against + * the edge of a clipping card (a Plex playlist show header) does not: the ring ran past the list, or + * was clipped away entirely. A table row needs no flag: index.css insets every `tr` by its tag. */ export const navItemAttrs = ( index: number, name?: string | null, -): { "data-nav-index": number; "data-nav-name"?: string } => ({ + { inset = false }: { inset?: boolean } = {}, +): { "data-nav-index": number; "data-nav-name"?: string; "data-nav-inset"?: "" } => ({ [INDEX]: index, ...(name ? { "data-nav-name": name } : null), + ...(inset ? { "data-nav-inset": "" } : null), }); const indexOf = (item: Element): number => Number(item.getAttribute(INDEX)); diff --git a/frontend/src/lib/navRing.contract.test.ts b/frontend/src/lib/navRing.contract.test.ts new file mode 100644 index 00000000..5f4c4f8f --- /dev/null +++ b/frontend/src/lib/navRing.contract.test.ts @@ -0,0 +1,45 @@ +import { readFileSync } from "node:fs"; + +import { expect, it } from "vitest"; + +import { navItemAttrs } from "./listNav"; + +// The keyboard ring of a list item (`[data-nav-index]`) sits 2 px OUTSIDE the item: a card with a gap +// round it has room. A table row, a log line or a row flush in a clipping card has none — the ring ran +// under a sticky table head and past the table's sides, and in a Plex playlist show's card it was +// clipped away so the focus was invisible (E28, seen live). A `tr` is inset by its tag (a table +// hand-built outside DataTable was the one first reported); the others ask for `inset`. jsdom does no +// layout, so what is pinned is the attribute and the rule that draws it inside, with every selector the +// outside ring uses (a `:has()` one outranks a plain attribute pair). +const css = readFileSync(new URL("../index.css", import.meta.url), "utf8"); + +it("an inset item carries the marker, a plain one does not", () => { + expect(navItemAttrs(3, "x", { inset: true })).toEqual({ + "data-nav-index": 3, + "data-nav-name": "x", + "data-nav-inset": "", + }); + expect(navItemAttrs(3)).toEqual({ "data-nav-index": 3 }); +}); + +it("index.css draws an inset item's ring inside, after the outside rule", () => { + const outside = css.indexOf("[data-nav-index]:has([data-nav-open]:focus-visible)"); + const at = css.indexOf(":is(tr, [data-nav-inset])[data-nav-index],"); + expect(outside, "the outside ring").toBeGreaterThan(0); + expect(at, "the inset rule comes later").toBeGreaterThan(outside); + const rule = css.slice(at, css.indexOf("}", at)); + expect(rule).toContain(":is(tr, [data-nav-inset])[data-nav-index]:focus-visible"); + expect(rule).toContain( + ":is(tr, [data-nav-inset])[data-nav-index]:has([data-nav-open]:focus-visible)", + ); + expect(rule).toMatch(/outline-offset:\s*-2px/); +}); + +// The items that sit flush, by their source: a new one of the kind should be added here. +const src = (p: string) => readFileSync(new URL(p, import.meta.url), "utf8"); +it.each([["../components/LogLine.tsx", /navItemAttrs\(navIndex, [^)]*\{ inset: true \}\)/]])( + "%s asks for the inside ring", + (file, re) => { + expect(src(file)).toMatch(re); + }, +);