diff --git a/src/components/KeyboardCheatsheet/KeyboardCheatsheet.test.tsx b/src/components/KeyboardCheatsheet/KeyboardCheatsheet.test.tsx index 0608960..e8b6bc0 100644 --- a/src/components/KeyboardCheatsheet/KeyboardCheatsheet.test.tsx +++ b/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

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( + + ); + + 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( + + ); + + 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(); + }); }); diff --git a/src/components/KeyboardCheatsheet/KeyboardCheatsheet.tsx b/src/components/KeyboardCheatsheet/KeyboardCheatsheet.tsx index 8288491..39b6d1a 100644 --- a/src/components/KeyboardCheatsheet/KeyboardCheatsheet.tsx +++ b/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 `

` 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

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" > -
- -

- Keyboard -

+
+
+ +

+ Keyboard +

+
+ {/* + 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. + */} +
    diff --git a/src/components/ui/card.test.tsx b/src/components/ui/card.test.tsx new file mode 100644 index 0000000..ec99b21 --- /dev/null +++ b/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 `...` and the rendered + // element must stay a
    . Touching this contract would + // regress every consumer in the codebase. + const { container, unmount } = renderIntoContainer( + + Default + + ); + + // No `as` prop →
    . + const title = container.querySelector("h2, h3, h4, h5, h6"); + expect(title).toBeNull(); + // The title text should still appear (now in a
    ). + 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 and assert the + // resulting element is an

    with the title text inside. + const { container, unmount } = renderIntoContainer( + + Dashboard section + + ); + + 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( + + Sub-section + + ); + + 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( + + + Styled + + + ); + + 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(); + const { unmount } = renderIntoContainer( + + + With ref + + + ); + + expect(ref.current).not.toBeNull(); + expect(ref.current?.tagName).toBe("H2"); + expect(ref.current?.textContent).toBe("With ref"); + + unmount(); + }); +}); \ No newline at end of file diff --git a/src/components/ui/card.tsx b/src/components/ui/card.tsx index 0c9c1d8..234f98b 100644 --- a/src/components/ui/card.tsx +++ b/src/components/ui/card.tsx @@ -22,16 +22,58 @@ const CardHeader = React.forwardRef>( - ({ className, ...props }, ref) => ( -
    - ) -); -CardTitle.displayName = "CardTitle"; +// Polymorphic `as` prop so consumers can pick the correct heading level +// for semantic structure (e.g. `` inside a Dashboard +// section). Defaults to `"div"` so all existing call-sites that omit +// `as` continue to render a
    exactly as before — no regression. +// +// 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 +// 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 = Omit< + React.ComponentPropsWithoutRef, + "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 = ( + props: CardTitleProps & { ref?: React.ForwardedRef> } +) => React.ReactElement | null; + +const CardTitle = React.forwardRef>( + function CardTitleImpl( + { as, className, ...props }: CardTitleProps<"div">, + ref: React.ForwardedRef + ) { + const Component = (as ?? "div") as React.ElementType; + return ( + } + 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>( ({ className, ...props }, ref) => ( diff --git a/src/components/ui/select.test.tsx b/src/components/ui/select.test.tsx new file mode 100644 index 0000000..90a1914 --- /dev/null +++ b/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( + + ); + }); + 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 { + 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(); + }); +}); \ No newline at end of file diff --git a/src/components/ui/select.tsx b/src/components/ui/select.tsx index 074bdba..4670516 100644 --- a/src/components/ui/select.tsx +++ b/src/components/ui/select.tsx @@ -64,7 +64,7 @@ const SelectItem = React.forwardRef<