diff --git a/.github/workflows/storybook-tests.yaml b/.github/workflows/storybook-tests.yaml index 702c70f4da..3df2c365cd 100644 --- a/.github/workflows/storybook-tests.yaml +++ b/.github/workflows/storybook-tests.yaml @@ -146,6 +146,18 @@ jobs: if-no-files-found: ignore retention-days: 1 + # The aria surface (role + accessible-name pairs per story), captured in + # the same postVisit pass. Sharded exactly like the a11y artifact above; + # the aria-baseline / aria-diff jobs below stitch the shards together. + - name: Upload aria snapshots + if: always() && steps.run_tests.outcome != 'skipped' + uses: actions/upload-artifact@v4 + with: + name: aria-snapshots-${{ matrix.shard }} + path: packages/react/aria-snapshots.jsonl + if-no-files-found: ignore + retention-days: 1 + # Single job to gate the branch on, so protection rules do not have to list # every shard, and does not need editing when SHARD_TOTAL changes. Named to # match the "✅ …" gate every other PR workflow now exposes. @@ -248,3 +260,187 @@ jobs: comment-type: a11y_axe github-token: ${{ secrets.GITHUB_TOKEN }} comment-body: ${{ steps.a11y_comment.outputs.body }} + + # Publish the aria surface of `main` — the role + accessible-name pairs every + # story renders — so PRs have something to diff against. + # + # This is the whole reason the check needs no second Storybook build: the + # workflow already visits all ~2.3k stories on every push to main, so the + # baseline costs one artifact merge. The PR side compares against it directly. + # + # Deliberately gated on `needs.test.result == 'success'`: a run where a shard + # failed produced a *partial* snapshot set, and publishing that as the + # baseline would make the next PR report every unrun story as deleted. A + # slightly stale but complete baseline beats a fresh broken one. + aria-baseline: + needs: [detect-changes, test] + name: "[⚛️ REACT] aria baseline (main)" + if: > + needs.test.result == 'success' && + github.event_name == 'push' && + github.ref == 'refs/heads/main' + runs-on: ubuntu-latest + permissions: + contents: read + steps: + # No `merge-multiple` — every shard names its file `aria-snapshots.jsonl`, + # so merging would have them overwrite each other. Same pattern as the + # a11y artifacts above. + - name: Download aria snapshots from all shards + uses: actions/download-artifact@v4 + with: + pattern: aria-snapshots-* + path: /tmp/aria-shards + - name: Merge shard artifacts + run: | + shopt -s nullglob + files=(/tmp/aria-shards/*/aria-snapshots.jsonl) + if [ ${#files[@]} -eq 0 ]; then + echo "::warning::No aria snapshots produced — baseline left unchanged." + exit 0 + fi + mkdir -p /tmp/aria-baseline + cat "${files[@]}" > /tmp/aria-baseline/aria-snapshots.jsonl + echo "Merged ${#files[@]} shard(s), $(wc -l < /tmp/aria-baseline/aria-snapshots.jsonl) stories." + # 90 days, unlike the 1-day PR artifacts: this one has to still be there + # for whatever PR opens next. The lookup below walks back through recent + # main runs, so an occasional gap is survivable. + - name: Upload baseline + uses: actions/upload-artifact@v4 + with: + name: aria-baseline + path: /tmp/aria-baseline/aria-snapshots.jsonl + if-no-files-found: ignore + retention-days: 90 + + # Diff this PR's aria surface against the main baseline and comment. + # + # Advisory by design — it posts a comment and never fails. The point of + # starting non-blocking is to see how much real churn these role/name diffs + # carry before anyone's merge depends on them. + # + # Not gated on by "✅ Storybook Tests" for the same reason the a11y comment + # isn't: a best-effort PR comment is not worth blocking a merge over. + aria-diff: + needs: [detect-changes, test] + name: "[⚛️ REACT] aria surface diff" + if: > + always() && needs.test.result != 'skipped' && + github.event_name == 'pull_request' && + github.event.pull_request.head.repo.full_name == github.repository + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write + # Required to list and download an artifact belonging to a *different* + # workflow run (the main baseline). + actions: read + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: "22.x" + + # Walk recent successful main runs newest-first and take the first that + # actually carries a non-expired baseline. A single lookup at the latest + # run would come up empty whenever that run predates this feature, failed, + # or had its artifact expire. + - name: Find the latest main baseline + id: baseline_run + uses: actions/github-script@v7 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const { owner, repo } = context.repo; + const runs = await github.rest.actions.listWorkflowRuns({ + owner, repo, + workflow_id: 'storybook-tests.yaml', + branch: 'main', + event: 'push', + status: 'success', + per_page: 20, + }); + for (const run of runs.data.workflow_runs) { + const arts = await github.rest.actions.listWorkflowRunArtifacts({ + owner, repo, run_id: run.id, per_page: 100, + }); + const hit = arts.data.artifacts.find( + (a) => a.name === 'aria-baseline' && !a.expired + ); + if (hit) { + core.info(`Baseline from run ${run.id} (${run.head_sha.slice(0, 7)})`); + core.setOutput('run_id', String(run.id)); + return; + } + } + core.warning('No aria-baseline artifact found on main yet — the first main run after this lands will create one.'); + core.setOutput('run_id', ''); + + - name: Download main baseline + if: steps.baseline_run.outputs.run_id != '' + continue-on-error: true + uses: actions/download-artifact@v4 + with: + name: aria-baseline + path: /tmp/aria-base + run-id: ${{ steps.baseline_run.outputs.run_id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Download this PR's aria snapshots + uses: actions/download-artifact@v4 + continue-on-error: true + with: + pattern: aria-snapshots-* + path: /tmp/aria-shards + + - name: Merge shard artifacts + run: | + shopt -s nullglob + mkdir -p /tmp/aria-head + files=(/tmp/aria-shards/*/aria-snapshots.jsonl) + if [ ${#files[@]} -gt 0 ]; then + cat "${files[@]}" > /tmp/aria-head/aria-snapshots.jsonl + echo "Merged ${#files[@]} shard(s), $(wc -l < /tmp/aria-head/aria-snapshots.jsonl) stories." + else + echo "No aria snapshots from this run." + fi + + - name: Diff the aria surface + id: aria_diff + continue-on-error: true + env: + # A failed shard means whole story files are missing from the head + # side; the script then suppresses its "deleted story" findings rather + # than blame the PR for stories that simply never ran. + PARTIAL: ${{ needs.test.result != 'success' && '--partial' || '' }} + run: | + npx --yes tsx@4 packages/react/.scripts/check-aria-surface.ts \ + --base /tmp/aria-base \ + --head /tmp/aria-head \ + $PARTIAL > /tmp/aria-output.txt 2>&1 || true + LAST_JSON_LINE=$(grep -n '^{' /tmp/aria-output.txt | tail -n 1 | cut -d: -f1) + if [ -z "$LAST_JSON_LINE" ]; then + echo "No JSON output from the aria surface check:" + cat /tmp/aria-output.txt + exit 0 + fi + sed -n "${LAST_JSON_LINE},\$p" /tmp/aria-output.txt > /tmp/aria-data.json + jq -r '.commentMarkdown // ""' /tmp/aria-data.json > /tmp/aria-comment.md || true + BREAKING=$(jq -r '.breakingTotal // 0' /tmp/aria-data.json) + if [ "$BREAKING" != "0" ]; then + echo "::warning::$BREAKING accessible name/role change(s) vs main could break an existing getByRole query — see the PR comment." + fi + { + echo "body<> "$GITHUB_OUTPUT" + + - name: Post aria surface PR comment + if: always() && steps.aria_diff.outputs.body != '' + continue-on-error: true + uses: ./.github/actions/add-or-update-pr-comment + with: + comment-type: aria_surface + github-token: ${{ secrets.GITHUB_TOKEN }} + comment-body: ${{ steps.aria_diff.outputs.body }} diff --git a/packages/react/.gitignore b/packages/react/.gitignore index 38f289c163..ade4a17f7b 100644 --- a/packages/react/.gitignore +++ b/packages/react/.gitignore @@ -4,3 +4,7 @@ # Written by the a11y test-runner; consumed by the a11y PR-comment step a11y-violations.jsonl + +# Written by the aria-snapshot test-runner hook; consumed by the aria-surface +# PR-comment step +aria-snapshots.jsonl diff --git a/packages/react/.scripts/__tests__/check-aria-surface.test.ts b/packages/react/.scripts/__tests__/check-aria-surface.test.ts new file mode 100644 index 0000000000..c2fe714f57 --- /dev/null +++ b/packages/react/.scripts/__tests__/check-aria-surface.test.ts @@ -0,0 +1,426 @@ +import { describe, expect, it } from "vitest" + +import { + extractAriaSurface, + nodeKey, + parseNodeHeader, +} from "../../src/lib/storybook-utils/ariaSurface" +import { + buildMarkdown, + countBreaking, + diffStory, + diffSurfaces, + pairRenames, + readSnapshots, + type StorySnapshot, +} from "../check-aria-surface" + +const story = ( + id: string, + nodes: Record, + overrides: Partial = {} +): StorySnapshot => ({ + id, + title: "Components/InputField", + name: "Default", + file: "src/components/F0InputField/__stories__/F0InputField.stories.tsx", + nodes, + ...overrides, +}) + +describe("parseNodeHeader", () => { + it("parses a bare role", () => { + expect(parseNodeHeader("button")).toEqual({ + role: "button", + name: null, + level: null, + }) + }) + + it("parses role + accessible name", () => { + expect(parseNodeHeader('button "Clear"')).toEqual({ + role: "button", + name: "Clear", + level: null, + }) + }) + + it("keeps the heading level, which getByRole can filter on", () => { + expect(parseNodeHeader('heading "Settings" [level=2]')).toEqual({ + role: "heading", + name: "Settings", + level: 2, + }) + }) + + it("ignores state attributes that are not part of the query identity", () => { + expect(parseNodeHeader('checkbox "Remember me" [checked]')).toEqual({ + role: "checkbox", + name: "Remember me", + level: null, + }) + }) + + it("handles a trailing colon introducing children", () => { + expect(parseNodeHeader('navigation "breadcrumb":')).toEqual({ + role: "navigation", + name: "breadcrumb", + level: null, + }) + }) + + it("decodes a YAML single-quoted header with an escaped apostrophe", () => { + expect(parseNodeHeader(`'button "It''s here"'`)).toEqual({ + role: "button", + name: "It's here", + level: null, + }) + }) + + it("unescapes backslash escapes inside the name", () => { + expect(parseNodeHeader('button "Say \\"hi\\""')).toEqual({ + role: "button", + name: 'Say "hi"', + level: null, + }) + }) + + it("drops text entries — content, not a queryable role", () => { + expect(parseNodeHeader("text: Some copy")).toBeNull() + }) + + it("returns null rather than throwing on junk", () => { + expect(parseNodeHeader("")).toBeNull() + expect(parseNodeHeader(" ")).toBeNull() + expect(parseNodeHeader("|")).toBeNull() + expect(parseNodeHeader("'unterminated")).toBeNull() + }) +}) + +describe("nodeKey", () => { + it("renders the key the way the node would be queried", () => { + expect(nodeKey({ role: "button", name: "Clear", level: null })).toBe( + 'button "Clear"' + ) + expect(nodeKey({ role: "textbox", name: null, level: null })).toBe( + "textbox" + ) + expect(nodeKey({ role: "heading", name: "Title", level: 1 })).toBe( + 'heading "Title" [level=1]' + ) + }) +}) + +describe("extractAriaSurface", () => { + /** + * Verbatim output of `page.locator("body").ariaSnapshot()` on Playwright + * 1.57 — captured from a real chromium run, not hand-written, because the + * parser is only as good as its grip on this format. Three things here were + * not obvious and are the reason this fixture is pinned: + * + * - `- /url: "#"` — link targets are emitted as their own entries with a + * leading slash, so they must not parse as a role. + * - `- button "Clear": x` — the trailing `: x` is the element's text + * content, not part of the name. + * - names are always double-quoted with backslash escapes + * (`"Say \\"hi\\""`); an apostrophe needs no special handling. + */ + const snapshot = `- banner: + - heading "Dashboard" [level=1] +- main: + - text: Email + - textbox "Email" + - button "Clear": x + - button "Clear": x + - button "It's here": "y" + - button "Say \\"hi\\"": z + - link "Docs": + - /url: "#" + - heading "Sub" [level=3] + - paragraph: Some prose + - checkbox "Remember me" [checked] + - text: Remember me + - navigation "breadcrumb": + - list: + - listitem: + - link "Home": + - /url: "#" + - button "Increase": + + - list: + - listitem: plain item + - textbox "Notes" + - combobox "Country": + - option "ES" [selected] +` + + it("counts occurrences rather than de-duplicating", () => { + // Two identically named buttons is exactly what a getAllByRole length + // assertion depends on, so the count has to survive. + expect(extractAriaSurface(snapshot)['button "Clear"']).toBe(2) + }) + + it("extracts the whole surface of a real snapshot", () => { + expect(extractAriaSurface(snapshot)).toEqual({ + banner: 1, + 'heading "Dashboard" [level=1]': 1, + main: 1, + 'textbox "Email"': 1, + 'button "Clear"': 2, + 'button "It\'s here"': 1, + 'button "Say "hi""': 1, + 'link "Docs"': 1, + 'heading "Sub" [level=3]': 1, + 'checkbox "Remember me"': 1, + 'navigation "breadcrumb"': 1, + 'link "Home"': 1, + 'button "Increase"': 1, + 'textbox "Notes"': 1, + 'combobox "Country"': 1, + 'option "ES"': 1, + }) + }) + + it("omits unnamed structural noise and non-role entries", () => { + const surface = extractAriaSurface(snapshot) + expect(surface.paragraph).toBeUndefined() + expect(surface.list).toBeUndefined() + expect(surface.listitem).toBeUndefined() + expect(surface.text).toBeUndefined() + // `- /url: "#"` must not be mistaken for a role. + expect(Object.keys(surface).some((k) => k.includes("url"))).toBe(false) + }) + + it("does not fold an element's text content into its name", () => { + // `- button "Increase": +` — the `+` is content, not part of the name. + expect(extractAriaSurface(snapshot)['button "Increase"']).toBe(1) + }) + + it("returns an empty surface for an empty snapshot", () => { + expect(extractAriaSurface("")).toEqual({}) + }) +}) + +describe("diffStory", () => { + it("returns null when the surface is unchanged", () => { + const s = { 'button "Clear"': 1 } + expect(diffStory(story("a", s), story("a", s))).toBeNull() + }) + + it("flags a disappeared role+name pair as removed", () => { + const d = diffStory( + story("a", { 'button "Clear"': 1, textbox: 1 }), + story("a", { textbox: 1 }) + ) + expect(d?.removed).toEqual([{ key: 'button "Clear"', before: 1, after: 0 }]) + expect(d?.added).toEqual([]) + }) + + it("flags a reduced count — the getAllByRole break", () => { + const d = diffStory( + story("a", { 'button "Remove"': 3 }), + story("a", { 'button "Remove"': 1 }) + ) + expect(d?.removed).toEqual([ + { key: 'button "Remove"', before: 3, after: 1 }, + ]) + }) + + it("treats a purely additive change as non-breaking", () => { + const d = diffStory(story("a", {}), story("a", { 'button "New"': 1 })) + expect(d?.removed).toEqual([]) + expect(d?.added).toEqual([{ key: 'button "New"', before: 0, after: 1 }]) + expect( + countBreaking({ + changed: [d!], + deletedStories: [], + newStories: [], + baseStories: 1, + headStories: 1, + }) + ).toBe(0) + }) + + it("pairs a same-role removal + addition into a rename", () => { + const d = diffStory( + story("a", { 'button "Clear"': 1 }), + story("a", { 'button "Clear input"': 1 }) + ) + expect(d?.renamed).toEqual([ + { + role: "button", + before: 'button "Clear"', + after: 'button "Clear input"', + }, + ]) + expect(d?.removed).toEqual([]) + expect(d?.added).toEqual([]) + }) + + it("catches a role change with the name held constant", () => { + const d = diffStory( + story("a", { 'button "Save"': 1 }), + story("a", { 'link "Save"': 1 }) + ) + expect(d?.renamed).toEqual([]) + expect(d?.removed).toEqual([{ key: 'button "Save"', before: 1, after: 0 }]) + expect(d?.added).toEqual([{ key: 'link "Save"', before: 0, after: 1 }]) + }) + + it("catches a heading level change", () => { + const d = diffStory( + story("a", { 'heading "Title" [level=3]': 1 }), + story("a", { 'heading "Title" [level=2]': 1 }) + ) + expect(d?.renamed).toHaveLength(1) + }) +}) + +describe("pairRenames", () => { + it("leaves ambiguous multi-rename cases split rather than guessing", () => { + const removed = [ + { key: 'button "A"', before: 1, after: 0 }, + { key: 'button "B"', before: 1, after: 0 }, + ] + const added = [ + { key: 'button "C"', before: 0, after: 1 }, + { key: 'button "D"', before: 0, after: 1 }, + ] + const out = pairRenames(removed, added) + expect(out.renamed).toEqual([]) + expect(out.removed).toHaveLength(2) + expect(out.added).toHaveLength(2) + }) + + it("does not pair when the counts don't line up", () => { + const out = pairRenames( + [{ key: 'button "A"', before: 3, after: 0 }], + [{ key: 'button "B"', before: 0, after: 1 }] + ) + expect(out.renamed).toEqual([]) + }) +}) + +describe("diffSurfaces", () => { + it("separates new stories from changed ones", () => { + const result = diffSurfaces( + [story("a", { 'button "X"': 1 })], + [story("a", { 'button "X"': 1 }), story("b", { 'button "Y"': 1 })] + ) + expect(result.changed).toEqual([]) + expect(result.newStories.map((s) => s.id)).toEqual(["b"]) + expect(countBreaking(result)).toBe(0) + }) + + it("counts a deleted story as breaking", () => { + const result = diffSurfaces([story("a", { button: 1 })], []) + expect(result.deletedStories.map((s) => s.id)).toEqual(["a"]) + expect(countBreaking(result)).toBe(1) + }) +}) + +describe("readSnapshots", () => { + it("returns an empty list for a missing path", () => { + expect(readSnapshots(null)).toEqual([]) + expect(readSnapshots("/nonexistent/aria-snapshots.jsonl")).toEqual([]) + }) +}) + +describe("buildMarkdown", () => { + const empty = { + changed: [], + deletedStories: [], + newStories: [], + baseStories: 10, + headStories: 10, + } + + it("explains itself when there is no baseline", () => { + const md = buildMarkdown(empty, { hasBaseline: false }) + expect(md).toContain("No baseline found") + expect(md).not.toContain("No accessible name or role changed") + }) + + it("reports a clean run", () => { + const md = buildMarkdown(empty, { hasBaseline: true }) + expect(md).toContain("✅ No accessible name or role changed across 10") + }) + + it("blames the run, not the PR, when head captured nothing", () => { + const md = buildMarkdown( + { ...empty, headStories: 0 }, + { hasBaseline: true, hasHead: false } + ) + expect(md).toContain("produced no aria snapshots") + expect(md).not.toContain("no longer exist") + }) + + it("warns and withholds deletions on a partial run", () => { + const md = buildMarkdown(empty, { hasBaseline: true, partial: true }) + expect(md).toContain("did not finish") + expect(md).toContain("not** reported") + }) + + it("puts breaking changes in the visible table, additive behind a details", () => { + const md = buildMarkdown( + { + ...empty, + changed: [ + { + id: "a", + title: "Components/InputField", + name: "Default", + file: "f.tsx", + removed: [{ key: 'button "Clear"', before: 1, after: 0 }], + added: [], + renamed: [], + }, + { + id: "b", + title: "Components/Card", + name: "Default", + file: "g.tsx", + removed: [], + added: [{ key: 'button "More"', before: 0, after: 1 }], + renamed: [], + }, + ], + }, + { hasBaseline: true } + ) + expect(md).toContain("Could break a query") + expect(md).toContain('`button "Clear"`') + expect(md).toContain("
") + expect(md).toContain('`button "More"`') + expect(md).toContain("1 change that could break") + // The additive-only story must not inflate the breaking-scope count. + expect(md).toContain("across **1 story**") + }) + + it("renders a rename as a single before/after row", () => { + const md = buildMarkdown( + { + ...empty, + changed: [ + { + id: "a", + title: "Components/InputField", + name: "Default", + file: "f.tsx", + removed: [], + added: [], + renamed: [ + { + role: "button", + before: 'button "Clear"', + after: 'button "Clear input"', + }, + ], + }, + ], + }, + { hasBaseline: true } + ) + expect(md).toContain("🔁 renamed") + expect(md).toContain('`button "Clear"` | `button "Clear input"`') + }) +}) diff --git a/packages/react/.scripts/check-aria-surface.ts b/packages/react/.scripts/check-aria-surface.ts new file mode 100644 index 0000000000..cd7a8367c7 --- /dev/null +++ b/packages/react/.scripts/check-aria-surface.ts @@ -0,0 +1,460 @@ +#!/usr/bin/env tsx +/** + * check-aria-surface.ts + * + * Diffs the **aria surface** — every story's `role` + accessible-name pairs — + * between the baseline captured on `main` and the current PR, and renders the + * result as a PR comment. + * + * Why this exists: nothing else in CI watches the accessibility tree for + * *change*. + * + * - `check-api-surface.ts` compares rolled-up `.d.ts` files. Declarations + * hold types, not values, so an `aria-label` written inline in a component + * body changes with a byte-identical API diff. + * - axe (`.storybook/test-runner.ts`) asks "does this element have a name?", + * never "is it the same name as before". Renaming "Clear" to "Clear input" + * keeps every rule green. + * - Chromatic diffs pixels, not semantics. + * + * Meanwhile downstream suites query by exactly this: `getByRole("button", { + * name: "Clear" })`, `cy.findByRole(...)`. A rename here is a silent break + * there. + * + * Both sides come from `aria-snapshots.jsonl`, written per story by the + * test-runner. The baseline is the artifact from the most recent `main` run of + * the Storybook Tests workflow — no second Storybook build, and the head side + * is the PR *merge commit* (PR + current main), so the two line up. + * + * Usage (CI): + * tsx .scripts/check-aria-surface.ts --base --head [--json] + * + * Emits a single JSON object as its last stdout line: { commentMarkdown, ... }. + * Exit code is always 0 while this check is advisory — it reports, it does not + * gate. Flip `--fail-on-breaking` on once the signal has been observed. + */ +import { existsSync, readFileSync, statSync } from "node:fs" +import { join } from "node:path" + +import type { AriaSurface } from "../src/lib/storybook-utils/ariaSurface" + +const ARTIFACT_NAME = "aria-snapshots.jsonl" + +const COMMENT_NOTE = + "Compares the role + accessible-name pairs every story renders against the " + + "baseline from the latest `main` run. These are what `getByRole(role, { name })` " + + "and `cy.findByRole(...)` match on downstream — the typed API check can't see " + + "them (they're values, not types) and axe can't either (it checks a name " + + "*exists*, not that it's unchanged). Advisory: this comment never blocks a merge." + +/** One story's captured surface, as written by the test-runner. */ +export interface StorySnapshot { + id: string + title: string + name: string + file: string + nodes: AriaSurface +} + +/** A single role+name pair that gained or lost occurrences. */ +export interface NodeDelta { + key: string + before: number + after: number +} + +/** A removed/added pair that looks like the same element renamed. */ +export interface RenameDelta { + role: string + before: string + after: string +} + +export interface StoryDiff { + id: string + title: string + name: string + file: string + /** Present in base, gone (or fewer) in head — the breaking direction. */ + removed: NodeDelta[] + /** New in head, or more occurrences than base. */ + added: NodeDelta[] + /** Removed+added pairs on the same role, matched up as a likely rename. */ + renamed: RenameDelta[] +} + +export interface AriaDiffResult { + /** Stories whose surface changed and that exist on both sides. */ + changed: StoryDiff[] + /** Story ids in the baseline with no counterpart in head. */ + deletedStories: StorySnapshot[] + /** Story ids in head that the baseline never had. */ + newStories: StorySnapshot[] + baseStories: number + headStories: number +} + +/** + * Resolve a `--base`/`--head` argument that may name either the JSONL file + * itself or the directory an artifact was downloaded into. + */ +export function resolveArtifactPath(p: string): string | null { + if (!existsSync(p)) return null + if (statSync(p).isDirectory()) { + const nested = join(p, ARTIFACT_NAME) + return existsSync(nested) ? nested : null + } + return p +} + +/** Parse a JSONL artifact, skipping any line that doesn't parse. */ +export function readSnapshots(path: string | null): StorySnapshot[] { + if (!path || !existsSync(path)) return [] + return readFileSync(path, "utf-8") + .split("\n") + .map((l) => l.trim()) + .filter(Boolean) + .flatMap((l) => { + try { + const parsed = JSON.parse(l) as StorySnapshot + return parsed && typeof parsed.id === "string" && parsed.nodes + ? [parsed] + : [] + } catch { + return [] + } + }) +} + +/** The `role` prefix of a node key, used to pair renames. */ +function roleOf(key: string): string { + const match = /^([a-zA-Z][a-zA-Z-]*)/.exec(key) + return match ? match[1] : key +} + +/** + * Pair each removed key with an added key of the same role, so a rename reads + * as one "Clear → Clear input" line instead of two unrelated ones. + * + * Only pairs when the role has exactly one removal and one addition. Anything + * ambiguous (two buttons renamed at once) stays split across removed/added, + * where it is still reported — just not guessed at. + */ +export function pairRenames( + removed: NodeDelta[], + added: NodeDelta[] +): { + renamed: RenameDelta[] + removed: NodeDelta[] + added: NodeDelta[] +} { + const renamed: RenameDelta[] = [] + const usedRemoved = new Set() + const usedAdded = new Set() + + const byRole = (list: NodeDelta[]) => { + const m = new Map() + for (const d of list) { + const r = roleOf(d.key) + const bucket = m.get(r) + if (bucket) bucket.push(d) + else m.set(r, [d]) + } + return m + } + + const removedByRole = byRole(removed) + const addedByRole = byRole(added) + + for (const [role, gone] of Array.from(removedByRole)) { + const appeared = addedByRole.get(role) + if (!appeared || gone.length !== 1 || appeared.length !== 1) continue + // Same count on each side, or it's a count change rather than a rename. + if ( + gone[0].before - gone[0].after !== + appeared[0].after - appeared[0].before + ) + continue + renamed.push({ role, before: gone[0].key, after: appeared[0].key }) + usedRemoved.add(gone[0].key) + usedAdded.add(appeared[0].key) + } + + return { + renamed, + removed: removed.filter((d) => !usedRemoved.has(d.key)), + added: added.filter((d) => !usedAdded.has(d.key)), + } +} + +/** Diff one story's surface. Returns null when nothing changed. */ +export function diffStory( + base: StorySnapshot, + head: StorySnapshot +): StoryDiff | null { + const keys = new Set([...Object.keys(base.nodes), ...Object.keys(head.nodes)]) + const removed: NodeDelta[] = [] + const added: NodeDelta[] = [] + + for (const key of Array.from(keys)) { + const before = base.nodes[key] ?? 0 + const after = head.nodes[key] ?? 0 + if (before === after) continue + if (after < before) removed.push({ key, before, after }) + else added.push({ key, before, after }) + } + + if (!removed.length && !added.length) return null + + const paired = pairRenames(removed, added) + return { + id: head.id, + title: head.title, + name: head.name, + file: head.file, + removed: paired.removed.sort((a, b) => a.key.localeCompare(b.key)), + added: paired.added.sort((a, b) => a.key.localeCompare(b.key)), + renamed: paired.renamed.sort((a, b) => a.before.localeCompare(b.before)), + } +} + +export function diffSurfaces( + baseList: StorySnapshot[], + headList: StorySnapshot[] +): AriaDiffResult { + const base = new Map(baseList.map((s) => [s.id, s])) + const head = new Map(headList.map((s) => [s.id, s])) + + const changed: StoryDiff[] = [] + const newStories: StorySnapshot[] = [] + const deletedStories: StorySnapshot[] = [] + + for (const [id, headStory] of Array.from(head)) { + const baseStory = base.get(id) + if (!baseStory) { + newStories.push(headStory) + continue + } + const d = diffStory(baseStory, headStory) + if (d) changed.push(d) + } + for (const [id, baseStory] of Array.from(base)) { + if (!head.has(id)) deletedStories.push(baseStory) + } + + const byStory = (a: { title: string; name: string }, b: typeof a) => + `${a.title}/${a.name}`.localeCompare(`${b.title}/${b.name}`) + + return { + changed: changed.sort(byStory), + deletedStories: deletedStories.sort(byStory), + newStories: newStories.sort(byStory), + baseStories: base.size, + headStories: head.size, + } +} + +/** + * A change is "breaking" when a query that used to match may now find nothing: + * a role+name pair lost occurrences, a pair was renamed, or a whole story + * disappeared. Purely additive changes are not. + */ +export function countBreaking(result: AriaDiffResult): number { + return ( + result.changed.reduce( + (n, s) => n + s.removed.length + s.renamed.length, + 0 + ) + result.deletedStories.length + ) +} + +function describeCount(delta: NodeDelta): string { + if (delta.before === 0) return "added" + if (delta.after === 0) return "removed" + return `${delta.before} → ${delta.after}` +} + +export function buildMarkdown( + result: AriaDiffResult, + { + hasBaseline, + hasHead = true, + partial = false, + }: { hasBaseline: boolean; hasHead?: boolean; partial?: boolean } +): string { + const header = "### 🔎 Accessible name & role changes\n" + const partialNote = partial + ? "\n> ⚠️ At least one Storybook shard did not finish, so the head side is " + + "incomplete. Stories that only appear in the baseline are **not** reported " + + "as deleted here — they may simply not have run.\n" + : "" + const note = `\n${COMMENT_NOTE}\n` + + if (!hasBaseline) { + return ( + `${header}\nℹ️ No baseline found — the \`aria-baseline\` artifact from the ` + + `latest \`main\` run of this workflow wasn't available (it's produced on ` + + `push to \`main\`, and expires after 90 days). Nothing to compare against ` + + `this time.\n${note}` + ) + } + + // No head snapshots at all means the run captured nothing — an infrastructure + // problem, not a PR that deleted every story. Reporting the baseline as + // wholesale deletions would be both alarming and wrong. + if (!hasHead) { + return ( + `${header}\nℹ️ This run produced no aria snapshots, so there is nothing to ` + + `compare against the ${result.baseStories}-story baseline. Usually that ` + + `means the Storybook shards did not get far enough to capture any.\n${note}` + ) + } + + const breaking = countBreaking(result) + const additiveOnly = result.changed.filter( + (s) => !s.removed.length && !s.renamed.length + ) + const withBreaks = result.changed.filter( + (s) => s.removed.length || s.renamed.length + ) + // Scope the headline to the stories that actually carry a breaking change — + // additive-only stories are reported separately and must not inflate it. + const breakingStories = withBreaks.length + result.deletedStories.length + + if (!result.changed.length && !result.deletedStories.length) { + const suffix = result.newStories.length + ? ` ${result.newStories.length} new stor${result.newStories.length === 1 ? "y" : "ies"} added.` + : "" + return ( + `${header}${partialNote}\n✅ No accessible name or role changed across ` + + `${result.headStories} stories.${suffix}\n${note}` + ) + } + + const summary = breaking + ? `\n⚠️ **${breaking} change${breaking === 1 ? "" : "s"} that could break an existing ` + + `\`getByRole\` / \`findByRole\` query**, across ` + + `**${breakingStories} stor${breakingStories === 1 ? "y" : "ies"}**.\n` + : `\n✅ No query-breaking changes — ${additiveOnly.length} stor${ + additiveOnly.length === 1 ? "y" : "ies" + } gained roles or names.\n` + + const sections: string[] = [] + + if (withBreaks.length) { + const rows = withBreaks + .flatMap((s) => [ + ...s.renamed.map( + (r) => + `| ${s.title} / ${s.name} | 🔁 renamed | \`${r.before}\` | \`${r.after}\` |` + ), + ...s.removed.map( + (d) => + `| ${s.title} / ${s.name} | ❌ ${describeCount(d)} | \`${d.key}\` | — |` + ), + ]) + .join("\n") + sections.push( + "\n#### Could break a query\n\n" + + "| Story | Change | Before | After |\n| --- | --- | --- | --- |\n" + + rows + + "\n" + ) + } + + if (result.deletedStories.length) { + sections.push( + "\n#### Stories that no longer exist\n\n" + + result.deletedStories + .map((s) => `- ${s.title} / ${s.name} (\`${s.file}\`)`) + .join("\n") + + "\n" + ) + } + + if (additiveOnly.length) { + const rows = additiveOnly + .flatMap((s) => + s.added.map( + (d) => + `| ${s.title} / ${s.name} | \`${d.key}\` | ${describeCount(d)} |` + ) + ) + .join("\n") + sections.push( + `\n
\nAdditive only (${additiveOnly.length} stor${ + additiveOnly.length === 1 ? "y" : "ies" + })\n\n` + + "| Story | Node | Change |\n| --- | --- | --- |\n" + + rows + + "\n\n
\n" + ) + } + + return `${header}${partialNote}${summary}${sections.join("")}${note}` +} + +function parseArgs(): { base?: string; head?: string; partial: boolean } { + const args = process.argv.slice(2) + const at = (flag: string) => { + const i = args.indexOf(flag) + return i !== -1 && args[i + 1] ? args[i + 1] : undefined + } + return { + base: at("--base"), + head: at("--head"), + // Set by the workflow when a Storybook shard failed: the head side is then + // missing whole story files, and "absent from head" no longer means + // "deleted". + partial: args.includes("--partial"), + } +} + +function main(): void { + const { base, head, partial } = parseArgs() + + const basePath = base ? resolveArtifactPath(base) : null + const headPath = head ? resolveArtifactPath(head) : null + + const baseSnapshots = readSnapshots(basePath) + const headSnapshots = readSnapshots(headPath) + + // A baseline of zero stories means the artifact was missing or empty, not + // that main renders nothing — treat it as "no baseline" so the comment says + // so instead of reporting every story as new. + const hasBaseline = baseSnapshots.length > 0 + const hasHead = headSnapshots.length > 0 + const result = diffSurfaces(baseSnapshots, headSnapshots) + + // On a partial head, a story missing from head is indistinguishable from a + // story whose shard never ran. Drop the claim rather than cry wolf. + if (partial || !hasHead) result.deletedStories = [] + + const commentMarkdown = buildMarkdown(result, { + hasBaseline, + hasHead, + partial, + }) + + process.stdout.write( + JSON.stringify( + { + hasBaseline, + hasHead, + baseStories: result.baseStories, + headStories: result.headStories, + changedStories: result.changed.length, + newStories: result.newStories.length, + deletedStories: result.deletedStories.length, + breakingTotal: hasBaseline ? countBreaking(result) : 0, + commentMarkdown, + }, + null, + 2 + ) + "\n" + ) +} + +if (process.argv[1] && /check-aria-surface\.(ts|js)$/.test(process.argv[1])) { + main() +} diff --git a/packages/react/.storybook/test-runner.ts b/packages/react/.storybook/test-runner.ts index 78cb291a64..f17a5fd639 100644 --- a/packages/react/.storybook/test-runner.ts +++ b/packages/react/.storybook/test-runner.ts @@ -17,6 +17,10 @@ import { A11Y_CI_CONTEXT, A11Y_RUN_ONLY, } from "../src/lib/storybook-utils/a11yAxeConfig.ts" +import { + extractAriaSurface, + type AriaSurface, +} from "../src/lib/storybook-utils/ariaSurface.ts" // Story files grandfathered to skip axe while their violations are burned // down (Path to AA). Maps file → number of allowed skip call-sites; counts @@ -43,6 +47,12 @@ const A11Y_TEST_MODES = ["error", "todo", "warning"] as const // test-runner relocates import.meta.url when it transforms this module. const A11Y_ARTIFACT = join(process.cwd(), "a11y-violations.jsonl") +// Machine-readable aria-surface record consumed by the aria-surface diff step +// (.scripts/check-aria-surface.ts). One JSONL line per story holding the +// role + accessible-name pairs it renders, so a PR can be diffed against the +// baseline captured on main. Same location/rationale as A11Y_ARTIFACT above. +const ARIA_ARTIFACT = join(process.cwd(), "aria-snapshots.jsonl") + /** * Map an axe rule's tags to its WCAG success criterion, level and version. * Inlined (not shared with A11yRow's copy) because the test-runner's loader @@ -102,6 +112,47 @@ function recordA11yViolations( } } +/** + * Capture the story's aria surface — the role + accessible-name pairs it + * renders — and append one JSONL line for the diff step. + * + * Scoped to `body`, deliberately *wider* than the `#storybook-root` context + * axe uses (see A11Y_CI_CONTEXT). In the test-runner `page` is the preview + * iframe, so `body` adds exactly the portaled content axe currently cannot see + * — dropdowns, dialogs, tooltips, the Select listbox — which is the part + * consumers' Cypress suites struggle with most. Storybook's own wrappers are + * plain `div`s and map to `generic`, which Playwright omits from the snapshot, + * so nothing of the harness leaks in. + * + * Best-effort throughout: a story that cannot be snapshotted is skipped, never + * failed. This hook is observational and must not change which tests pass. + */ +async function recordAriaSurface( + page: Parameters>[0], + story: { id: string; title: string; name: string; file: string } +): Promise { + try { + // Same two-frame settle the axe path uses further down, for the same + // reason: entry animations (AnimatePresence mounts the F0InputField clear + // button, for one) must reach their committed state, or the snapshot + // records a tree that never existed and reports a phantom diff. + await page.evaluate( + () => + new Promise((resolve) => { + requestAnimationFrame(() => requestAnimationFrame(() => resolve())) + }) + ) + const snapshot = await page.locator("body").ariaSnapshot() + const surface: AriaSurface = extractAriaSurface(snapshot) + appendFileSync( + ARIA_ARTIFACT, + JSON.stringify({ ...story, nodes: surface }) + "\n" + ) + } catch { + // ignore — a missing snapshot degrades the PR comment, nothing more + } +} + /** * Custom reporter that only logs violations, suppressing success messages */ @@ -243,6 +294,22 @@ const config: TestRunnerConfig = { "" ) + // Capture the aria surface first, before any of the axe branches below + // can return or throw. Every story contributes — including the ones + // grandfathered out of axe via `skipCi`, whose accessible names are still + // a contract consumers query. Stories with nondeterministic content + // (random ids, `new Date()`) can opt out with + // `parameters: { ariaSnapshot: { skip: true } }` rather than emit a diff + // on every run. + if (storyContext.parameters?.ariaSnapshot?.skip !== true) { + await recordAriaSurface(page, { + id: context.id, + title: context.title, + name: context.name, + file: storyFile, + }) + } + if (a11yParams.skipCi) { // Grandfathered files keep skipping while their violations are burned // down (Path to AA). The allowlist may only shrink. diff --git a/packages/react/AGENTS.md b/packages/react/AGENTS.md index 8cfd31bf15..f14a4e641e 100644 --- a/packages/react/AGENTS.md +++ b/packages/react/AGENTS.md @@ -239,6 +239,36 @@ See `f0-component-patterns` skill for `TranslationsType`, `defaultTranslations`, - Delegate complex widgets (dialogs, selects, toggles) to Radix via `@/ui/` - Load the `a11y` skill for detailed WCAG patterns and decision trees +### Accessible names are a public API + +Roles and accessible names are what consumers query — `getByRole("button", { +name: "Clear" })` in unit tests, `cy.findByRole(...)` in Cypress. Changing one +breaks them, and neither of the other checks notices: the public API check +diffs `.d.ts` files (names are values, not types) and axe only asks whether a +name *exists*, not whether it changed. + +The **aria surface** check covers this. Every story's role + accessible-name +pairs are captured in the Storybook test-runner and diffed against the baseline +from the latest `main` run, then reported as a PR comment. It is **advisory** — +it never blocks a merge. + +- Treat a rename in that comment as a breaking change: it needs the same + deliberation as removing a prop. +- Hardcoded `aria-label`s in component bodies are the usual source. i18n-ing one + changes the name in every non-English locale. +- A story with nondeterministic content can opt out with + `parameters: { ariaSnapshot: { skip: true } }` rather than emit a diff on + every run. +- **It only sees what a story actually renders.** F0InputField's clear button + is a live example: it mounts only once the field has a value, so renaming its + `aria-label` shows up in the one story with a filled input and nowhere else. + A conditional element with no story covering that state is invisible to this + check — which is one more reason to give new states their own story. + +```bash +pnpm check:aria-surface --base --head +``` + ## Code Quality - Comments answer "Why?", not "What?" — keep them rare diff --git a/packages/react/package.json b/packages/react/package.json index fc7256dc87..2764782134 100644 --- a/packages/react/package.json +++ b/packages/react/package.json @@ -48,6 +48,7 @@ "format:check": "sh -c 'oxfmt --check \"${@:-src/}\"' --", "tsc": "tsc --noEmit", "check:api-surface": "tsx .scripts/check-api-surface.ts", + "check:aria-surface": "tsx .scripts/check-aria-surface.ts", "check:bugfix-red-green": "tsx .scripts/check-bugfix-red-green.ts", "check:new-component-dod": "tsx .scripts/check-new-component-dod.ts", "check:stable-dod": "tsx .scripts/check-stable-dod.ts", diff --git a/packages/react/src/lib/storybook-utils/ariaSurface.ts b/packages/react/src/lib/storybook-utils/ariaSurface.ts new file mode 100644 index 0000000000..17ca136465 --- /dev/null +++ b/packages/react/src/lib/storybook-utils/ariaSurface.ts @@ -0,0 +1,213 @@ +/** + * The **aria surface**: the set of `role` + accessible-name pairs a story + * renders. + * + * This is the contract consumers actually query — `getByRole("button", { name: + * "Clear" })` in a unit test, `cy.findByRole(...)` in a Cypress suite — and + * nothing else in CI watches it: + * + * - the public API surface check (`.scripts/check-api-surface.ts`) runs + * `ts.createProgram` over the rolled-up `.d.ts` files. Declarations hold + * types, not values, so an `aria-label` hardcoded in a component body (there + * are dozens — `F0InputField`'s "Clear", `Arrows`' "Increase"/"Decrease", + * `pagination`'s "Go to next page") can change with a byte-identical diff. + * - axe (`.storybook/test-runner.ts`) is an *absolute* check: it asks "does + * this element have a name?", never "is it the same name as last week?". A + * rename from "Clear" to "Clear input" keeps every rule green. + * - Chromatic diffs pixels. A DOM change that renders identically passes. + * + * So this module extracts the queryable identity of every node in a story and + * the diff script (`.scripts/check-aria-surface.ts`) compares it against the + * baseline captured on `main`. + * + * NOTE: kept dependency-free and side-effect-free on purpose — the Storybook + * test-runner imports it through its own loader (with an explicit `.ts` + * extension, see the import in `.storybook/test-runner.ts`). + */ + +/** One node of a Playwright aria snapshot, reduced to what a query can target. */ +export interface AriaNode { + role: string + /** The accessible name, or `null` when the node has none. */ + name: string | null + /** Heading level, when the node carries `[level=N]`. */ + level: number | null +} + +/** + * Roles worth tracking even when they carry no accessible name. + * + * An unnamed node can't break a name-based query, so by default a node only + * enters the surface once it has a name — that keeps prose-heavy stories from + * emitting a wall of anonymous `paragraph` / `listitem` entries that churn on + * every copy edit. + * + * These roles are the exception: tests routinely query them bare + * (`getByRole("button")`, `within(getByRole("dialog"))`), so their *count* + * matters even nameless. Deliberately excludes `list`, `listitem` and + * `paragraph`, which are the noisiest structural roles in this library. + */ +export const NAMELESS_ROLES_TRACKED: ReadonlySet = new Set([ + "alert", + "alertdialog", + "banner", + "button", + "cell", + "checkbox", + "columnheader", + "combobox", + "complementary", + "contentinfo", + "dialog", + "form", + "grid", + "gridcell", + "heading", + "link", + "listbox", + "main", + "menu", + "menuitem", + "menuitemcheckbox", + "menuitemradio", + "navigation", + "option", + "progressbar", + "radio", + "region", + "row", + "rowheader", + "search", + "searchbox", + "slider", + "spinbutton", + "status", + "switch", + "tab", + "table", + "tablist", + "tabpanel", + "textbox", + "toolbar", + "tooltip", + "tree", + "treeitem", +]) + +/** + * Snapshot entries that are content rather than a queryable role. Playwright + * emits raw strings as `- text: …`; `text` is not an ARIA role, and the copy it + * carries changes on every wording tweak. + */ +const NON_ROLE_ENTRIES: ReadonlySet = new Set(["text"]) + +/** Matches a YAML list item, capturing everything after the `- `. */ +const NODE_LINE = /^\s*-\s+(.*)$/ +/** A role token: the leading identifier of a node header. */ +const ROLE_TOKEN = /^([a-zA-Z][a-zA-Z-]*)/ +/** A double-quoted accessible name, allowing backslash escapes. */ +const QUOTED_NAME = /^\s+"((?:[^"\\]|\\.)*)"/ +const LEVEL_ATTR = /\[level=(\d+)\]/ + +/** + * Read a YAML single-quoted scalar starting at index 0, returning its decoded + * contents (`''` is an escaped `'`). Playwright wraps a whole node header in + * single quotes when the accessible name contains characters YAML would + * otherwise read as syntax — e.g. `- 'button "It''s here"'`. + * + * Returns `null` when the scalar is never closed (a truncated line). + */ +function readSingleQuoted(s: string): string | null { + let out = "" + let i = 1 + while (i < s.length) { + if (s[i] === "'") { + if (s[i + 1] === "'") { + out += "'" + i += 2 + continue + } + return out + } + out += s[i] + i++ + } + return null +} + +/** + * Parse one node header (the text after `- `) into its queryable identity. + * Returns `null` for content entries and anything unparseable — a malformed + * line must never take the run down. + */ +export function parseNodeHeader(raw: string): AriaNode | null { + let s = raw.trim() + if (!s) return null + + if (s.startsWith("'")) { + const decoded = readSingleQuoted(s) + if (decoded === null) return null + s = decoded + } + + const roleMatch = ROLE_TOKEN.exec(s) + if (!roleMatch) return null + const role = roleMatch[1] + if (NON_ROLE_ENTRIES.has(role)) return null + + let rest = s.slice(role.length) + + let name: string | null = null + const nameMatch = QUOTED_NAME.exec(rest) + if (nameMatch) { + // Undo the backslash escaping Playwright applies inside the quotes. + name = nameMatch[1].replace(/\\(.)/g, "$1") + rest = rest.slice(nameMatch[0].length) + } + + const levelMatch = LEVEL_ATTR.exec(rest) + const level = levelMatch ? Number(levelMatch[1]) : null + + return { role, name, level } +} + +/** + * The stable key a node is counted under — and the string shown in the PR + * comment. Mirrors how the node would be queried: + * + * button "Clear" + * heading "Settings" [level=2] + * textbox + */ +export function nodeKey(node: AriaNode): string { + const name = node.name ? ` "${node.name}"` : "" + const level = node.level !== null ? ` [level=${node.level}]` : "" + return `${node.role}${name}${level}` +} + +/** A story's aria surface: node key → how many times it appears. */ +export type AriaSurface = Record + +/** + * Reduce a Playwright `ariaSnapshot()` string to the story's aria surface. + * + * Counts, not a set: "there used to be three buttons named Remove and now + * there is one" is exactly the kind of break a `getAllByRole` assertion trips + * over, and a set would hide it. + */ +export function extractAriaSurface(snapshot: string): AriaSurface { + const surface: AriaSurface = {} + for (const line of snapshot.split("\n")) { + const lineMatch = NODE_LINE.exec(line) + if (!lineMatch) continue + + const node = parseNodeHeader(lineMatch[1]) + if (!node) continue + // Unnamed nodes only count for the roles tests query bare. + if (!node.name && !NAMELESS_ROLES_TRACKED.has(node.role)) continue + + const key = nodeKey(node) + surface[key] = (surface[key] ?? 0) + 1 + } + return surface +}