a11y(frontend): select outline, CardTitle polymorphic as prop, cheatsheet labels

src/components/KeyboardCheatsheet/KeyboardCheatsheet.test.tsx

@@ -267,4 +267,86 @@ describe("KeyboardCheatsheet", () => {
 
     unmount();
   });
+
+  it("test_dialog_is_aria_labelledby_visible_heading", async () => {
+    // The DialogContent must carry an aria-labelledby pointing at
+    // the visible <h2> so screen readers announce "Keyboard, dialog"
+    // instead of leaving the modal unlabeled (Radix logs a warning
+    // otherwise). The id referenced must also resolve to a real
+    // element with that id inside the cheatsheet.
+    const onClose = vi.fn();
+    const { unmount } = renderIntoContainer(
+      <KeyboardCheatsheet open={true} onClose={onClose} />
+    );
+
+    const body = await settle((b) =>
+      b.querySelector('[data-testid="keyboard-cheatsheet"]') !== null
+    );
+
+    const sheet = body.querySelector(
+      '[data-testid="keyboard-cheatsheet"]'
+    ) as HTMLElement | null;
+    expect(sheet).not.toBeNull();
+
+    // The Radix DialogContent exposes its labelledby on the panel
+    // element itself. We accept either the dialog content id (which
+    // is the data-testid) or its first role-bearing descendant —
+    // depending on which element Radix attaches aria-labelledby to.
+    const labelledBy =
+      sheet?.getAttribute("aria-labelledby") ??
+      sheet
+        ?.querySelector("[aria-labelledby]")
+        ?.getAttribute("aria-labelledby") ??
+      null;
+    expect(labelledBy).not.toBeNull();
+    expect(labelledBy).toBe("keyboard-cheatsheet-title");
+
+    // And the id must point at a real element whose text content is
+    // the visible "Keyboard" heading — closing the loop between the
+    // a11y wiring and the visible UI.
+    const titleEl = body.querySelector("#keyboard-cheatsheet-title");
+    expect(titleEl).not.toBeNull();
+    expect(titleEl?.textContent).toBe("Keyboard");
+
+    unmount();
+  });
+
+  it("test_close_button_has_specific_aria_label_and_describedby", async () => {
+    // The cheatsheet renders an explicit close button (the shared
+    // DialogContent X in the corner has a generic "Close dialog"
+    // label that's out of scope here). The cheatsheet-specific one
+    // must:
+    //  - be findable by its cheatsheet-specific aria-label, and
+    //  - carry aria-describedby pointing back at the visible heading
+    //    so screen-reader users hear "Close keyboard cheatsheet,
+    //    Keyboard, button".
+    const onClose = vi.fn();
+    const { unmount } = renderIntoContainer(
+      <KeyboardCheatsheet open={true} onClose={onClose} />
+    );
+
+    const body = await settle((b) =>
+      b.querySelector('[data-testid="keyboard-cheatsheet"]') !== null
+    );
+
+    const closeBtn = body.querySelector(
+      '[aria-label="Close keyboard cheatsheet"]'
+    ) as HTMLButtonElement | null;
+    expect(closeBtn).not.toBeNull();
+
+    // The describedby wires the close button back to the title so
+    // the screen-reader announcement has context.
+    expect(closeBtn?.getAttribute("aria-describedby")).toBe(
+      "keyboard-cheatsheet-title"
+    );
+
+    // Clicking the close button must invoke the consumer's onClose.
+    expect(onClose).not.toHaveBeenCalled();
+    act(() => {
+      closeBtn?.click();
+    });
+    expect(onClose).toHaveBeenCalledTimes(1);
+
+    unmount();
+  });
 });

src/components/KeyboardCheatsheet/KeyboardCheatsheet.tsx

@@ -1,5 +1,5 @@
 import { useEffect } from "react";
-import { Keyboard } from "lucide-react";
+import { Keyboard, X } from "lucide-react";
 import { Dialog, DialogContent } from "@/components/ui/dialog";
 
 type KeyboardCheatsheetProps = {
@@ -20,6 +20,14 @@ const SHORTCUTS = [
   { keys: ["?"], description: "Toggle this help" },
 ] as const;
 
+/**
+ * Stable id used to wire the dialog's `aria-labelledby` and the close
+ * button's `aria-describedby` to the visible `<h2>` heading. Exposed
+ * as a constant so tests can assert against the same string the
+ * component renders.
+ */
+const CHEATSHEET_TITLE_ID = "keyboard-cheatsheet-title";
+
 /**
  * Keyboard help overlay (SP4).
  *
@@ -78,19 +86,52 @@ export function KeyboardCheatsheet({ open, onClose }: KeyboardCheatsheetProps) {
         // `left-1/2 top-1/2 -translate-x-1/2 -translate-y-1/2`).
         // We narrow it to max-w-md and apply the modern palette so
         // it sits on the same surface as the rest of SP4.
+        //
+        // a11y wiring:
+        //  - `aria-labelledby` points at the visible <h2> below so
+        //    screen readers announce "Keyboard, dialog" instead of an
+        //    unlabeled region (Radix logs a warning otherwise).
+        //  - `aria-describedby={undefined}` suppresses Radix's
+        //    missing-description warning; the cheatsheet's content
+        //    (key list + helper text) is its own description.
         className="max-w-md bg-[color:var(--m-surface)] text-[color:var(--m-ink-primary)] border border-[color:var(--m-border-heavy)]/20"
+        aria-labelledby={CHEATSHEET_TITLE_ID}
         aria-describedby={undefined}
         data-testid="keyboard-cheatsheet"
       >
-        <header className="flex items-center gap-2 mb-3">
+        <header className="flex items-center justify-between gap-2 mb-3 pr-10">
-          <Keyboard
+          <div className="flex items-center gap-2">
-            className="h-4 w-4 text-[color:var(--m-ink-secondary)]"
+            <Keyboard
-            strokeWidth={1.75}
+              className="h-4 w-4 text-[color:var(--m-ink-secondary)]"
-            aria-hidden
+              strokeWidth={1.75}
-          />
+              aria-hidden
-          <h2 className="text-[10.5px] font-semibold uppercase tracking-[0.18em] text-[color:var(--m-ink-tertiary)]">
+            />
-            Keyboard
+            <h2
-          </h2>
+              id={CHEATSHEET_TITLE_ID}
+              className="text-[10.5px] font-semibold uppercase tracking-[0.18em] text-[color:var(--m-ink-tertiary)]"
+            >
+              Keyboard
+            </h2>
+          </div>
+          {/*
+            Explicit close button with a cheatsheet-specific label.
+            The shared DialogContent primitive renders its own X in
+            the top-right corner with a generic "Close dialog" label
+            (out of scope here), so this one carries the specific
+            aria-label and an aria-describedby pointer back to the
+            heading for screen-reader context. `pr-10` on the header
+            keeps the visible title text clear of the primitive's
+            absolute-positioned X in the corner.
+          */}
+          <button
+            type="button"
+            aria-label="Close keyboard cheatsheet"
+            aria-describedby={CHEATSHEET_TITLE_ID}
+            onClick={onClose}
+            className="rounded-md p-1 text-[color:var(--m-ink-secondary)] opacity-70 transition-opacity hover:opacity-100 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-background"
+          >
+            <X className="h-4 w-4" aria-hidden />
+          </button>
         </header>
 
         <ul className="flex flex-col gap-2.5">

src/components/ui/card.test.tsx

@@ -0,0 +1,138 @@
+// @vitest-environment happy-dom
+// Pure presentation component — no React Query, no async work — but
+// we still set IS_REACT_ACT_ENVIRONMENT to match the project convention
+// (ClaimDrawerHeader.test.tsx, KeyboardCheatsheet.test.tsx, etc.).
+(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
+
+import React, { act } from "react";
+import { createRoot, type Root } from "react-dom/client";
+import { describe, expect, it } from "vitest";
+import { Card, CardTitle } from "./card";
+
+function renderIntoContainer(element: React.ReactElement): {
+  container: HTMLDivElement;
+  unmount: () => void;
+} {
+  const container = document.createElement("div");
+  document.body.appendChild(container);
+  const root: Root = createRoot(container);
+  act(() => {
+    root.render(element);
+  });
+  return {
+    container,
+    unmount: () => {
+      act(() => root.unmount());
+      container.remove();
+    },
+  };
+}
+
+describe("CardTitle", () => {
+  it("test_defaults_to_div_when_no_as_prop_provided", () => {
+    // Backwards-compat: existing Dashboard / Upload / Providers
+    // call-sites write `<CardTitle>...</CardTitle>` and the rendered
+    // element must stay a <div>. Touching this contract would
+    // regress every consumer in the codebase.
+    const { container, unmount } = renderIntoContainer(
+      <Card>
+        <CardTitle>Default</CardTitle>
+      </Card>
+    );
+
+    // No `as` prop → <div>.
+    const title = container.querySelector("h2, h3, h4, h5, h6");
+    expect(title).toBeNull();
+    // The title text should still appear (now in a <div>).
+    const text = container.textContent ?? "";
+    expect(text).toContain("Default");
+
+    unmount();
+  });
+
+  it("test_renders_h2_when_as_h2", () => {
+    // Polymorphic `as` prop: consumers can request the semantic level
+    // they need. Here we render <CardTitle as="h2"> and assert the
+    // resulting element is an <h2> with the title text inside.
+    const { container, unmount } = renderIntoContainer(
+      <Card>
+        <CardTitle as="h2">Dashboard section</CardTitle>
+      </Card>
+    );
+
+    const h2 = container.querySelector("h2");
+    expect(h2).not.toBeNull();
+    expect(h2?.textContent).toBe("Dashboard section");
+
+    // And no other heading level should appear inside this card —
+    // confirms the polymorphic forwardRef actually swaps the element
+    // type rather than rendering both.
+    expect(container.querySelector("h1")).toBeNull();
+    expect(container.querySelector("h3")).toBeNull();
+
+    unmount();
+  });
+
+  it("test_renders_h3_when_as_h3", () => {
+    // Verify the generic accepts heading levels other than h2 — the
+    // Dashboard uses h2 for top-level cards but inner cards may want
+    // h3 for proper document outline nesting.
+    const { container, unmount } = renderIntoContainer(
+      <Card>
+        <CardTitle as="h3">Sub-section</CardTitle>
+      </Card>
+    );
+
+    const h3 = container.querySelector("h3");
+    expect(h3).not.toBeNull();
+    expect(h3?.textContent).toBe("Sub-section");
+
+    unmount();
+  });
+
+  it("test_preserves_className_when_using_as", () => {
+    // The base className ("text-base font-semibold ...") must still
+    // apply when `as` is provided — the polymorphic swap shouldn't
+    // drop styling.
+    const { container, unmount } = renderIntoContainer(
+      <Card>
+        <CardTitle as="h2" className="custom-class">
+          Styled
+        </CardTitle>
+      </Card>
+    );
+
+    const h2 = container.querySelector("h2");
+    expect(h2).not.toBeNull();
+    const cls = h2?.className ?? "";
+    expect(cls).toContain("font-semibold");
+    expect(cls).toContain("custom-class");
+
+    unmount();
+  });
+
+  it("test_forwards_ref_to_polymorphic_element", () => {
+    // `React.forwardRef` must hand the ref to whichever element `as`
+    // resolves to — otherwise consumers can't focus / measure the
+    // heading after rendering. We use `createRef` (typed at the
+    // call site as the polymorphic target element) rather than a
+    // callback ref so TypeScript can fully resolve the generic
+    // through the polymorphic cast — a callback ref here would
+    // end up typed as `never` because the generic `E` parameter
+    // can't unify through the call-signature cast on the component.
+    const ref = React.createRef<HTMLHeadingElement>();
+    const { unmount } = renderIntoContainer(
+      <Card>
+        <CardTitle as="h2" ref={ref}>
+          With ref
+        </CardTitle>
+      </Card>
+    );
+
+    expect(ref.current).not.toBeNull();
+    expect(ref.current?.tagName).toBe("H2");
+    expect(ref.current?.textContent).toBe("With ref");
+
+    unmount();
+  });
+});

src/components/ui/card.tsx

@@ -22,16 +22,58 @@ const CardHeader = React.forwardRef<HTMLDivElement, React.HTMLAttributes<HTMLDiv
 );
 CardHeader.displayName = "CardHeader";
 
-const CardTitle = React.forwardRef<HTMLDivElement, React.HTMLAttributes<HTMLDivElement>>(
+// Polymorphic `as` prop so consumers can pick the correct heading level
-  ({ className, ...props }, ref) => (
+// for semantic structure (e.g. `<CardTitle as="h2">` inside a Dashboard
-    <div
+// section). Defaults to `"div"` so all existing call-sites that omit
-      ref={ref}
+// `as` continue to render a <div> exactly as before — no regression.
-      className={cn("text-base font-semibold leading-none tracking-tight", className)}
+//
-      {...props}
+// Implementation note: `React.forwardRef` infers its ref/props types
-    />
+// from the inner render function, but TypeScript can't propagate a
-  )
+// generic parameter through `forwardRef`'s own declaration. The
-);
+// well-known workaround (see Ben Ilegbodu's polymorphic-component
-CardTitle.displayName = "CardTitle";
+// write-up, mirrored by Mantine / tsteele.dev) is to give `forwardRef`
+// concrete types up front and then cast the resulting component to a
+// generic call signature so consumers get element-specific props
+// (e.g. h2 attrs) and the right ref type based on `as`. The inner
+// render uses `as` to swap the actual rendered element at runtime;
+// the type assertion on `ref` is safe because React only cares about
+// the `current` property at runtime regardless of the underlying
+// DOM node type.
+type CardTitleProps<E extends React.ElementType = "div"> = Omit<
+  React.ComponentPropsWithoutRef<E>,
+  "as"
+> & {
+  as?: E;
+};
+
+// The exported component is a polymorphic callable: a single call
+// signature with the generic `E` parameter that consumers control
+// via the `as` prop. We assign forwardRef's result through an
+// `unknown` cast (necessary because the source type is
+// `ForwardRefExoticComponent` while the target is a plain generic
+// function).
+type CardTitleComponent = <E extends React.ElementType = "div">(
+  props: CardTitleProps<E> & { ref?: React.ForwardedRef<React.ElementRef<E>> }
+) => React.ReactElement | null;
+
+const CardTitle = React.forwardRef<HTMLDivElement, CardTitleProps<"div">>(
+  function CardTitleImpl(
+    { as, className, ...props }: CardTitleProps<"div">,
+    ref: React.ForwardedRef<HTMLDivElement>
+  ) {
+    const Component = (as ?? "div") as React.ElementType;
+    return (
+      <Component
+        ref={ref as React.Ref<HTMLDivElement>}
+        className={cn("text-base font-semibold leading-none tracking-tight", className)}
+        {...props}
+      />
+    );
+  }
+) as unknown as CardTitleComponent;
+// Displayname for React DevTools — assignable via cast since the
+// polymorphic call signature doesn't expose it.
+(CardTitle as unknown as { displayName: string }).displayName = "CardTitle";
 
 const CardDescription = React.forwardRef<HTMLDivElement, React.HTMLAttributes<HTMLDivElement>>(
   ({ className, ...props }, ref) => (

src/components/ui/select.test.tsx

@@ -0,0 +1,151 @@
+// @vitest-environment happy-dom
+// Pure presentation primitive — no React Query, no async work — but
+// we still set IS_REACT_ACT_ENVIRONMENT to match the project convention
+// (ClaimDrawerHeader.test.tsx, KeyboardCheatsheet.test.tsx, etc.).
+(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
+
+import { act } from "react";
+import { createRoot, type Root } from "react-dom/client";
+import { beforeEach, describe, expect, it } from "vitest";
+import {
+  Select,
+  SelectContent,
+  SelectItem,
+  SelectTrigger,
+  SelectValue,
+} from "./select";
+
+/**
+ * Render the Radix Select open with two items so the popover content
+ * is in the DOM. Radix's SelectPortal puts the popover in
+ * `document.body`, so tests query there.
+ *
+ * Returns an `unmount` that synchronously tears down the React tree.
+ * Radix Select portals its content into `document.body`; between
+ * tests we wipe `document.body` in `beforeEach` to be safe — happy-dom
+ * + concurrent React can briefly leak portal nodes past unmount in
+ * the test environment, which makes the next test see stale
+ * `[role="option"]` elements.
+ */
+function renderOpenSelect(): {
+  container: HTMLDivElement;
+  unmount: () => void;
+} {
+  const container = document.createElement("div");
+  document.body.appendChild(container);
+  const root: Root = createRoot(container);
+  act(() => {
+    root.render(
+      <Select defaultValue="a" open>
+        <SelectTrigger>
+          <SelectValue />
+        </SelectTrigger>
+        <SelectContent>
+          <SelectItem value="a">Option A</SelectItem>
+          <SelectItem value="b">Option B</SelectItem>
+        </SelectContent>
+      </Select>
+    );
+  });
+  return {
+    container,
+    unmount: () => {
+      act(() => root.unmount());
+      container.remove();
+    },
+  };
+}
+
+/**
+ * Wait for the popover to be portaled into `document.body`. Radix
+ * mounts the SelectContent on a microtask after the trigger's `open`
+ * flips, so a single tick is normally enough; we poll defensively.
+ */
+async function waitForPopover(timeoutMs = 1000): Promise<HTMLElement> {
+  const body = document.body;
+  const start = Date.now();
+  while (!body.querySelector('[role="option"]')) {
+    if (Date.now() - start > timeoutMs) {
+      throw new Error(
+        `waitForPopover: no options rendered within ${timeoutMs}ms (body=${body.innerHTML.slice(0, 200)})`
+      );
+    }
+    await act(async () => {
+      await new Promise((r) => setTimeout(r, 0));
+    });
+  }
+  return body;
+}
+
+describe("SelectItem", () => {
+  // Wipe any leaked portal nodes between tests — happy-dom + Radix's
+  // Portal sometimes leaves a stray SelectContent behind after the
+  // React tree unmounts, which would let the next test see stale
+  // options in document.body and over-count assertions.
+  beforeEach(() => {
+    document.body.innerHTML = "";
+  });
+
+  it("test_selectitem_classname_includes_highlighted_outline_tokens", async () => {
+    // The hairline-outline tokens added for a11y are part of the
+    // static className string on every SelectItem. Tailwind compiles
+    // the `data-[highlighted]:outline-*` variants into CSS that
+    // activates only when the data attribute is present, but the
+    // className tokens themselves are always in the rendered class
+    // string. This test asserts they're present without needing to
+    // trigger keyboard navigation (which is finicky in happy-dom).
+    const { unmount } = renderOpenSelect();
+
+    const body = await waitForPopover();
+
+    const item = body.querySelector(
+      '[role="option"]'
+    ) as HTMLElement | null;
+    expect(item).not.toBeNull();
+    const cls = item?.className ?? "";
+
+    // Three new Tailwind tokens — outline on, 1px wide, accent color.
+    expect(cls).toContain("data-[highlighted]:outline");
+    expect(cls).toContain("data-[highlighted]:outline-1");
+    expect(cls).toContain("data-[highlighted]:outline-accent");
+
+    unmount();
+  });
+
+  it("test_selectitem_classname_preserves_existing_background", async () => {
+    // The new outline classes must layer ON TOP OF the existing
+    // highlighted background, not replace it — `bg-muted` plus a
+    // hairline outline reads cleaner than either alone.
+    const { unmount } = renderOpenSelect();
+
+    const body = await waitForPopover();
+    const item = body.querySelector(
+      '[role="option"]'
+    ) as HTMLElement | null;
+    expect(item).not.toBeNull();
+    const cls = item?.className ?? "";
+
+    // Original highlight background is preserved.
+    expect(cls).toContain("data-[highlighted]:bg-muted");
+    // And the new outline classes coexist with it.
+    expect(cls).toContain("data-[highlighted]:outline");
+
+    unmount();
+  });
+
+  it("test_renders_all_provided_items", async () => {
+    // Sanity check: the popover actually mounted both options. If
+    // this ever fails it's a regression in SelectContent itself, not
+    // in our outline work, but it's worth pinning down because the
+    // other two tests both query the same `[role="option"]` element.
+    const { unmount } = renderOpenSelect();
+
+    const body = await waitForPopover();
+    const options = body.querySelectorAll('[role="option"]');
+    expect(options.length).toBe(2);
+    expect(body.textContent).toContain("Option A");
+    expect(body.textContent).toContain("Option B");
+
+    unmount();
+  });
+});

src/components/ui/select.tsx

@@ -64,7 +64,7 @@ const SelectItem = React.forwardRef<
   <SelectPrimitive.Item
     ref={ref}
     className={cn(
-      "relative flex w-full cursor-default select-none items-center rounded-sm py-1.5 pl-8 pr-2 text-sm outline-none focus:bg-muted focus:text-foreground data-[highlighted]:bg-muted data-[highlighted]:text-foreground data-[state=checked]:font-medium data-[disabled]:pointer-events-none data-[disabled]:opacity-50",
+      "relative flex w-full cursor-default select-none items-center rounded-sm py-1.5 pl-8 pr-2 text-sm outline-none focus:bg-muted focus:text-foreground data-[highlighted]:bg-muted data-[highlighted]:text-foreground data-[highlighted]:outline data-[highlighted]:outline-1 data-[highlighted]:outline-accent data-[state=checked]:font-medium data-[disabled]:pointer-events-none data-[disabled]:opacity-50",
       className
     )}
     {...props}