fix(a11y,layout): deep-review fixes — menu roles, sort aria-pressed, rem rail threshold

Epic-end deep-review of the branch surfaced six verified findings; fix them. a11y (medium): FilterSelect and SortControl opened their AnchoredPopover in mode:"listbox" but never passed a role, so the surface fell through to role="menu" while the rows rendered role="option"+aria-selected — options orphaned outside a listbox, so AT could not announce the selected value. Switch both to the house menu pattern (as ViewSwitcher already uses): a menu surface with menuitemradio (single) / menuitemcheckbox (multi) rows carrying aria-checked, scoped by a role=group. Trigger stays a button+haspopup=menu. a11y (low): the SortControl asc/desc direction segment signalled the active direction by colour alone. Add aria-pressed so the current direction is programmatically exposed, matching SegmentedToggle. correctness (low): PageShell converted a rem maxWidth to px with a hardcoded 16 to decide the pinned-rail overlay threshold, but index.css scales the root font by --font-scale (default 1.06 ~ 17px, slider 0.9-1.3), so the threshold was wrong at every text size on the rem pages (playlists is the only one with a pinned rail). Use the live root font-size, scale the 36rem slack the same way, and recompute when --font-scale changes (a guarded MutationObserver — the slider fires no resize). tests: cover the two gaps the review flagged — ItemCount (plural, 1M+ overflow, undefined icon-only, tooltip) and the PageShell rail-pinned-w/content-mw threshold math incl. the font-scale scaling. Updated the FilterSelect/ SortControl role queries to the new menuitemradio/checkbox roles. Verified in the running app: menu surfaces carry role=group + menuitemradio/ checkbox with 0 orphaned options, and the sort direction segment exposes aria-pressed. Dropped two review findings (max-width duplication, pill-class reuse) as refuted on adversarial verification. Gate green: tsc, 372 vitest, prettier, eslint (0 err).
2026-08-21 22:41:57 +02:00
parent edbc9dede5
commit 4ee452b854
7 changed files with 214 additions and 97 deletions
@@ -2,8 +2,9 @@
 //
 // Contract of the labeled-pill FilterSelect. Labels are plain props (no i18n). Built on the floating-ui
 // AnchoredPopover platform, so we stub the two observers autoUpdate wires and wrap in a FloatingTree.
-// floating-ui never positions in jsdom → the surface stays visibility:hidden, so option/listbox role
-// queries pass `{ hidden: true }`.
+// floating-ui never positions in jsdom → the surface stays visibility:hidden, so the menuitem role
+// queries pass `{ hidden: true }`. Single-select rows are `menuitemradio`, multi-select `menuitemcheckbox`
+// (a menu surface with checkable items, not a listbox of orphaned options).
 import { describe, it, expect, vi, afterEach, beforeAll } from "vitest";
 import { render, screen, fireEvent, cleanup } from "@testing-library/react";
 import { FloatingTree } from "@floating-ui/react";
@@ -40,20 +41,19 @@ describe("FilterSelect", () => {
    const trigger = screen.getByRole("button", { name: "Status" });
    expect(trigger.textContent).toContain("Status:");
    expect(trigger.textContent).toContain("All");
-    expect(screen.queryByRole("option", { hidden: true })).toBeNull();
+    expect(screen.queryByRole("menuitemradio", { hidden: true })).toBeNull();
  });

  it("single: picking a row replaces the selection and closes", () => {
    const onChange = vi.fn();
    tree(<FilterSelect label="Status" options={STATUS} selected={["all"]} onChange={onChange} />);
    fireEvent.click(screen.getByRole("button", { name: "Status" }));
-    expect(screen.getAllByRole("option", { hidden: true }).map((o) => o.textContent)).toEqual([
-      "All",
-      "New",
-    ]);
-    fireEvent.click(screen.getAllByRole("option", { hidden: true })[1]!); // New
+    const rows = screen.getAllByRole("menuitemradio", { hidden: true });
+    expect(rows.map((o) => o.textContent)).toEqual(["All", "New"]);
+    expect(rows[0]!.getAttribute("aria-checked")).toBe("true"); // "All" is the current selection
+    fireEvent.click(rows[1]!); // New
    expect(onChange).toHaveBeenCalledWith(["new"]);
-    expect(screen.queryByRole("option", { hidden: true })).toBeNull();
+    expect(screen.queryByRole("menuitemradio", { hidden: true })).toBeNull();
  });

  it("multiple: toggling a row adds it and keeps the panel open", () => {
@@ -62,9 +62,9 @@ describe("FilterSelect", () => {
      <FilterSelect label="Type" multiple options={TYPES} selected={["n"]} onChange={onChange} />,
    );
    fireEvent.click(screen.getByRole("button", { name: "Type" }));
-    fireEvent.click(screen.getAllByRole("option", { hidden: true })[1]!); // Shorts
+    fireEvent.click(screen.getAllByRole("menuitemcheckbox", { hidden: true })[1]!); // Shorts
    expect(onChange).toHaveBeenCalledWith(["n", "s"]);
-    expect(screen.queryAllByRole("option", { hidden: true }).length).toBeGreaterThan(0); // still open
+    expect(screen.queryAllByRole("menuitemcheckbox", { hidden: true }).length).toBeGreaterThan(0); // still open
  });

  it("multiple + requireOne: the last remaining selection can't be turned off", () => {
@@ -73,7 +73,7 @@ describe("FilterSelect", () => {
      <FilterSelect label="Type" multiple options={TYPES} selected={["n"]} onChange={onChange} />,
    );
    fireEvent.click(screen.getByRole("button", { name: "Type" }));
-    fireEvent.click(screen.getAllByRole("option", { hidden: true })[0]!); // Normal — the only one selected
+    fireEvent.click(screen.getAllByRole("menuitemcheckbox", { hidden: true })[0]!); // Normal — the only one selected
    expect(onChange).not.toHaveBeenCalled();
  });

@@ -3,7 +3,7 @@ import { ChevronDown, Check } from "lucide-react";
 import { PopoverItem, PopoverSurface, useAnchoredPopover } from "./AnchoredPopover";

 // A compact "labeled pill → dropdown" filter control: the trigger reads `Label: value ▾`, and clicking
-// it opens a listbox of options (facet counts optional). It replaces a wall of individual toolbar chips
+// it opens a menu of options (facet counts optional). It replaces a wall of individual toolbar chips
 // with one pill per filter group, in two flavours:
 //   - single  (default): picking a row replaces the choice and closes — always exactly one active.
 //   - multiple: rows toggle independently and the panel stays open; `requireOne` blocks clearing the last.
@@ -45,7 +45,7 @@ export default function FilterSelect({
  const pop = useAnchoredPopover({
    open,
    onOpenChange: setOpen,
-    mode: "listbox",
+    mode: "menu",
    placement: "bottom-start",
  });

@@ -96,39 +96,44 @@ export default function FilterSelect({
        ariaLabel={label}
        className="glass-menu min-w-52 rounded-xl p-1.5 shadow-lg animate-[popIn_0.16s_ease]"
      >
-        {options.map((o) => {
-          const on = selected.includes(o.value);
-          return (
-            <PopoverItem
-              key={o.value}
-              role="option"
-              selected={on}
-              disabled={o.disabled}
-              onSelect={() => commit(o.value)}
-              label={o.label}
-              data-testid={testId ? `${testId}-${o.value}` : undefined}
-              className={`flex w-full items-center gap-2 rounded-md px-2 py-1.5 text-left text-sm transition ${
-                on ? "text-fg" : "text-muted"
-              }`}
-            >
-              {multiple ? (
-                <span
-                  className={`flex h-3.5 w-3.5 shrink-0 items-center justify-center rounded border ${
-                    on ? "border-accent bg-accent text-accent-fg" : "border-border"
-                  }`}
-                >
-                  {on && <Check className="h-2.5 w-2.5" />}
-                </span>
-              ) : (
-                <Check className={`h-3.5 w-3.5 shrink-0 ${on ? "text-accent" : "opacity-0"}`} />
-              )}
-              <span className="flex-1 truncate">{o.label}</span>
-              {o.count != null && (
-                <span className="shrink-0 text-2xs tabular-nums text-muted">{o.count}</span>
-              )}
-            </PopoverItem>
-          );
-        })}
+        {/* Single = one exclusive choice (radios), multi = independent toggles (checkboxes). Both are
+            menuitem-family roles scoped by a group, so AT announces the checked state — the surface is a
+            menu (see the mode above), never a bare listbox with orphaned options. */}
+        <div role="group" aria-label={label}>
+          {options.map((o) => {
+            const on = selected.includes(o.value);
+            return (
+              <PopoverItem
+                key={o.value}
+                role={multiple ? "menuitemcheckbox" : "menuitemradio"}
+                checked={on}
+                disabled={o.disabled}
+                onSelect={() => commit(o.value)}
+                label={o.label}
+                data-testid={testId ? `${testId}-${o.value}` : undefined}
+                className={`flex w-full items-center gap-2 rounded-md px-2 py-1.5 text-left text-sm transition ${
+                  on ? "text-fg" : "text-muted"
+                }`}
+              >
+                {multiple ? (
+                  <span
+                    className={`flex h-3.5 w-3.5 shrink-0 items-center justify-center rounded border ${
+                      on ? "border-accent bg-accent text-accent-fg" : "border-border"
+                    }`}
+                  >
+                    {on && <Check className="h-2.5 w-2.5" />}
+                  </span>
+                ) : (
+                  <Check className={`h-3.5 w-3.5 shrink-0 ${on ? "text-accent" : "opacity-0"}`} />
+                )}
+                <span className="flex-1 truncate">{o.label}</span>
+                {o.count != null && (
+                  <span className="shrink-0 text-2xs tabular-nums text-muted">{o.count}</span>
+                )}
+              </PopoverItem>
+            );
+          })}
+        </div>
      </PopoverSurface>
    </>
  );
@@ -0,0 +1,45 @@
+// @vitest-environment jsdom
+//
+// Contract of the fixed-width count pill. Uses the REAL i18n instance (not a key-passthrough) so the
+// plural selection (itemCount_one/_other) and the "1M+" overflow interpolation are actually exercised —
+// those are the two branches worth guarding. English is forced so the singular/plural strings are stable.
+import { describe, it, expect, beforeAll, afterEach } from "vitest";
+import { render, screen, cleanup } from "@testing-library/react";
+import "../i18n"; // side-effect: initialise the shared i18next instance
+import i18n from "i18next";
+import ItemCount from "./ItemCount";
+
+beforeAll(async () => {
+  await i18n.changeLanguage("en");
+});
+afterEach(cleanup);
+
+const text = () => screen.getByTestId("item-count").textContent;
+
+describe("ItemCount", () => {
+  it("uses the singular string at 1 and the plural above it", () => {
+    render(<ItemCount count={1} />);
+    expect(text()).toBe("1 item");
+  });
+
+  it("pluralises and thousands-separates a normal count", () => {
+    render(<ItemCount count={192045} />);
+    // Separators come from toLocaleString (runtime-locale dependent) — compare against it, not a fixed
+    // en-US string, so the test asserts ItemCount's formatting path rather than the CI locale.
+    expect(text()).toBe(`${(192045).toLocaleString()} items`);
+  });
+
+  it("collapses a count of a million or more to 1M+, with the exact figure in the tooltip", () => {
+    render(<ItemCount count={1_000_000} />);
+    expect(text()).toBe("1M+ items"); // the "1M+" collapse is locale-independent
+    expect(screen.getByTestId("item-count").getAttribute("title")).toBe(
+      (1_000_000).toLocaleString(),
+    );
+  });
+
+  it("shows the icon-only fixed box (empty label) while the count is undefined", () => {
+    render(<ItemCount />);
+    expect(text()).toBe(""); // the Layers icon carries no text
+    expect(screen.getByTestId("item-count").getAttribute("title")).toBeNull();
+  });
+});
@@ -74,3 +74,54 @@ describe("PageShell --hdr-h (E5 S7)", () => {
    expect(content().style.getPropertyValue("--hdr-h")).toBe("68px"); // re-measured, not the stale 4px
  });
 });
+
+// The pinned-rail overlay decision: a rail pins as OVERLAY (--rail-pinned-w 2.25rem, the toolbar keeps a
+// slim gutter and content stays put) once the window is wide enough that the centred content leaves room
+// for the rail in its left margin (maxWidth + ~36rem); below that it pins in-flow (16.75rem). The rem→px
+// conversion must use the LIVE root font-size, not a hardcoded 16, so it stays correct at every text size.
+describe("PageShell --rail-pinned-w / --content-mw", () => {
+  const root = document.documentElement;
+  const setWidth = (w: number) =>
+    Object.defineProperty(window, "innerWidth", { value: w, configurable: true });
+  afterEach(() => {
+    root.style.removeProperty("font-size");
+    root.style.removeProperty("--content-mw");
+    root.style.removeProperty("--rail-pinned-w");
+  });
+
+  it("overlays the rail once the window clears maxWidth + 36rem (at 16px/rem)", () => {
+    setWidth(1600); // 64rem*16 + 36rem*16 = 1024 + 576 = 1600 → exactly roomy
+    render(<PageShell maxWidth="64rem">content</PageShell>);
+    expect(root.style.getPropertyValue("--content-mw")).toBe("64rem");
+    expect(root.style.getPropertyValue("--rail-pinned-w")).toBe("2.25rem");
+  });
+
+  it("keeps the rail in-flow just below the threshold", () => {
+    setWidth(1599);
+    render(<PageShell maxWidth="64rem">content</PageShell>);
+    expect(root.style.getPropertyValue("--rail-pinned-w")).toBe("16.75rem");
+  });
+
+  it("never overlays on a full-width page (no maxWidth)", () => {
+    setWidth(3000);
+    render(<PageShell>content</PageShell>);
+    expect(root.style.getPropertyValue("--content-mw")).toBe("none");
+    expect(root.style.getPropertyValue("--rail-pinned-w")).toBe("16.75rem");
+  });
+
+  it("recomputes on resize", () => {
+    setWidth(1599);
+    render(<PageShell maxWidth="64rem">content</PageShell>);
+    expect(root.style.getPropertyValue("--rail-pinned-w")).toBe("16.75rem");
+    setWidth(1600);
+    window.dispatchEvent(new Event("resize"));
+    expect(root.style.getPropertyValue("--rail-pinned-w")).toBe("2.25rem");
+  });
+
+  it("scales the rem threshold by the live root font-size, not a hardcoded 16", () => {
+    root.style.fontSize = "17px"; // as the --font-scale 1.06 default renders it
+    setWidth(1650); // roomy at 16px/rem (>1600) but NOT at 17px/rem (needs 100rem*17 = 1700)
+    render(<PageShell maxWidth="64rem">content</PageShell>);
+    expect(root.style.getPropertyValue("--rail-pinned-w")).toBe("16.75rem");
+  });
+});
@@ -75,20 +75,34 @@ export default function PageShell({
    const root = document.documentElement;
    root.style.setProperty("--content-mw", maxWidth ?? "none");
    // Room the pinned rail needs: its own width (~16rem) on each side of the centred content, plus the
-    // nav rail (~4rem). Below that, or on a full-width page (no maxWidth), pin in-flow (w-67).
+    // nav rail (~4rem) = ~36rem. Below that, or on a full-width page (no maxWidth), pin in-flow (w-67).
+    let lastPxPerRem = 0;
    const apply = () => {
+      // Convert the rem maxWidth (and the 36rem slack) against the LIVE root font-size, not a hardcoded
+      // 16: index.css scales the root by --font-scale (default 1.06 ≈ 17px, slider 0.9–1.3), and the page's
+      // own content wrapper is a rem `max-w-*`, so a fixed 16 mis-sizes the threshold at every text size.
+      const pxPerRem = parseFloat(getComputedStyle(root).fontSize) || 16;
+      lastPxPerRem = pxPerRem;
      const mwPx = maxWidth
        ? maxWidth.endsWith("rem")
-          ? parseFloat(maxWidth) * 16
+          ? parseFloat(maxWidth) * pxPerRem
          : parseFloat(maxWidth)
        : null;
-      const roomy = mwPx != null && window.innerWidth >= mwPx + 576;
+      const roomy = mwPx != null && window.innerWidth >= mwPx + 36 * pxPerRem;
      root.style.setProperty("--rail-pinned-w", roomy ? "2.25rem" : "16.75rem");
    };
    apply();
    window.addEventListener("resize", apply);
+    // The text-size slider changes --font-scale (hence the root font-size) WITHOUT firing resize, so the
+    // rem→px threshold would go stale. Re-run when the root font actually changes; the pxPerRem guard keeps
+    // our own --rail-pinned-w write (a rem string, no font change) from re-triggering the observer.
+    const fontObserver = new MutationObserver(() => {
+      if ((parseFloat(getComputedStyle(root).fontSize) || 16) !== lastPxPerRem) apply();
+    });
+    fontObserver.observe(root, { attributes: true, attributeFilter: ["style"] });
    return () => {
      window.removeEventListener("resize", apply);
+      fontObserver.disconnect();
      root.style.removeProperty("--content-mw");
      root.style.removeProperty("--rail-pinned-w");
    };
@@ -2,8 +2,9 @@
 //
 // The rendering integration of SortControl (its pickSort/heal decision logic is covered in
 // columnSort.test.ts). Labels are plain props here — no i18n needed. It's a floating-ui popover now
-// (a labeled pill + a listbox with a footer direction segment), so we stub the two autoUpdate
-// observers and wrap in a FloatingTree; options only exist while open → role queries pass { hidden: true }.
+// (a labeled pill + a menu of `menuitemradio` sort keys with a footer direction segment), so we stub
+// the two autoUpdate observers and wrap in a FloatingTree; rows only exist while open → role queries
+// pass { hidden: true }.
 import { describe, it, expect, vi, afterEach, beforeAll } from "vitest";
 import { render, screen, fireEvent, cleanup } from "@testing-library/react";
 import { FloatingTree } from "@floating-ui/react";
@@ -55,22 +56,17 @@ describe("SortControl", () => {
  it("renders one option per SortOption, no no-sort row without defaultLabel", () => {
    renderControl({ key: "title", dir: "asc" });
    openPanel();
-    expect(screen.getAllByRole("option", { hidden: true }).map((o) => o.textContent)).toEqual([
-      "Date",
-      "Title",
-      "Shuffle",
-    ]);
+    expect(
+      screen.getAllByRole("menuitemradio", { hidden: true }).map((o) => o.textContent),
+    ).toEqual(["Date", "Title", "Shuffle"]);
  });

  it("adds a leading no-sort option when defaultLabel is set (tables)", () => {
    renderControl(null, { defaultLabel: "None" });
    openPanel();
-    expect(screen.getAllByRole("option", { hidden: true }).map((o) => o.textContent)).toEqual([
-      "None",
-      "Date",
-      "Title",
-      "Shuffle",
-    ]);
+    expect(
+      screen.getAllByRole("menuitemradio", { hidden: true }).map((o) => o.textContent),
+    ).toEqual(["None", "Date", "Title", "Shuffle"]);
  });

  it("summarises the trigger with the first option (not blank) for an unavailable key without defaultLabel", () => {
@@ -81,7 +77,7 @@ describe("SortControl", () => {
  it("picking a new option fires onChange at that option's defaultDir", () => {
    const { onChange } = renderControl({ key: "title", dir: "asc" });
    openPanel();
-    fireEvent.click(screen.getAllByRole("option", { hidden: true })[0]!); // Date
+    fireEvent.click(screen.getAllByRole("menuitemradio", { hidden: true })[0]!); // Date
    expect(onChange).toHaveBeenCalledWith({ key: "date", dir: "desc" });
  });

@@ -41,7 +41,7 @@ export default function SortControl({
  const pop = useAnchoredPopover({
    open,
    onOpenChange: setOpen,
-    mode: "listbox",
+    mode: "menu",
    placement: "bottom-end",
  });

@@ -89,43 +89,47 @@ export default function SortControl({
        ariaLabel={label}
        className="glass-menu min-w-52 rounded-xl p-1.5 shadow-lg animate-[popIn_0.16s_ease]"
      >
-        {defaultLabel !== undefined && (
-          <PopoverItem
-            role="option"
-            selected={active === null}
-            onSelect={() => {
-              onChange(null);
-              setOpen(false); // "no sort" is a final choice — nothing left to direct
-            }}
-            label={defaultLabel}
-            className={`flex w-full items-center gap-2 rounded-md px-2 py-1.5 text-left text-sm transition ${
-              active === null ? "text-fg" : "text-muted"
-            }`}
-          >
-            <Check
-              className={`h-3.5 w-3.5 shrink-0 ${active === null ? "text-accent" : "opacity-0"}`}
-            />
-            <span className="flex-1 truncate">{defaultLabel}</span>
-          </PopoverItem>
-        )}
-        {options.map((o) => {
-          const on = active?.key === o.key;
-          return (
+        {/* One exclusive choice of sort key (radios), scoped by a group so AT announces which is
+            checked — the surface is a menu (see the mode above), never a listbox with orphaned options. */}
+        <div role="group" aria-label={label}>
+          {defaultLabel !== undefined && (
            <PopoverItem
-              key={o.key}
-              role="option"
-              selected={on}
-              onSelect={() => onChange(pickSort(options, active, o.key))} // keep open to set direction
-              label={o.label}
+              role="menuitemradio"
+              checked={active === null}
+              onSelect={() => {
+                onChange(null);
+                setOpen(false); // "no sort" is a final choice — nothing left to direct
+              }}
+              label={defaultLabel}
              className={`flex w-full items-center gap-2 rounded-md px-2 py-1.5 text-left text-sm transition ${
-                on ? "text-fg" : "text-muted"
+                active === null ? "text-fg" : "text-muted"
              }`}
            >
-              <Check className={`h-3.5 w-3.5 shrink-0 ${on ? "text-accent" : "opacity-0"}`} />
-              <span className="flex-1 truncate">{o.label}</span>
+              <Check
+                className={`h-3.5 w-3.5 shrink-0 ${active === null ? "text-accent" : "opacity-0"}`}
+              />
+              <span className="flex-1 truncate">{defaultLabel}</span>
            </PopoverItem>
-          );
-        })}
+          )}
+          {options.map((o) => {
+            const on = active?.key === o.key;
+            return (
+              <PopoverItem
+                key={o.key}
+                role="menuitemradio"
+                checked={on}
+                onSelect={() => onChange(pickSort(options, active, o.key))} // keep open to set direction
+                label={o.label}
+                className={`flex w-full items-center gap-2 rounded-md px-2 py-1.5 text-left text-sm transition ${
+                  on ? "text-fg" : "text-muted"
+                }`}
+              >
+                <Check className={`h-3.5 w-3.5 shrink-0 ${on ? "text-accent" : "opacity-0"}`} />
+                <span className="flex-1 truncate">{o.label}</span>
+              </PopoverItem>
+            );
+          })}
+        </div>
        {((showArrow && active) || trailing) && (
          <div
            role="group"
@@ -136,6 +140,7 @@ export default function SortControl({
              <div className="flex flex-1 gap-1">
                <button
                  type="button"
+                  aria-pressed={active.dir === "asc"}
                  onClick={() => onChange({ key: active.key, dir: "asc" })}
                  className={segCls(active.dir === "asc")}
                >
@@ -144,6 +149,7 @@ export default function SortControl({
                </button>
                <button
                  type="button"
+                  aria-pressed={active.dir === "desc"}
                  onClick={() => onChange({ key: active.key, dir: "desc" })}
                  className={segCls(active.dir === "desc")}
                >