Skip to content

feat(storybook): add aria surface regression check - #5217

Open
sauldom102 wants to merge 1 commit into
mainfrom
claude/accessibility-regression-cypress-d4371a
Open

feat(storybook): add aria surface regression check#5217
sauldom102 wants to merge 1 commit into
mainfrom
claude/accessibility-regression-cypress-d4371a

Conversation

@sauldom102

Copy link
Copy Markdown
Collaborator

Description

Roles and accessible names are a public API — downstream suites find elements with getByRole("button", { name: "Clear" }) and cy.findByRole(...) — but nothing in CI watched them for change. This adds an aria surface check that captures every story's role + accessible-name pairs in the existing Storybook test-runner pass and diffs them against a baseline published on main, reporting the result as a PR comment.

Type of change

  • Other: CI check / test infrastructure

Screenshots (if applicable)

No visual changes — this is CI tooling. What it posts on a PR that renames an accessible name:

🔎 Accessible name & role changes

⚠️ 1 change that could break an existing getByRole / findByRole query, across 1 story.

Could break a query

Story Change Before After
Components/Primitives/F0InputField / Snapshot 🔁 renamed button "Clear" button "Clear input"

Implementation details

Three existing gates all miss this class of regression, by construction:

  • check-api-surface.ts runs ts.createProgram over the 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. There are dozens of these — F0InputField's "Clear", Arrows' "Increase"/"Decrease", pagination's "Go to next page", breadcrumb's landmark name.
  • axe is an absolute check: it asks whether an element has a name, never whether it is the same name as before. Renaming "Clear""Clear input" keeps every rule green.
  • Chromatic diffs pixels. A DOM change that renders identically passes.

Verified end to end: renaming aria-label="Clear" in F0InputField.tsx left all 23 of its stories passing axe, and produced exactly the comment above. Then reverted.

  • feat: capture each story's role + accessible-name pairs in the test-runner's postVisit, written to aria-snapshots.jsonl alongside the existing a11y artifact

    Why scope to `body` rather than `#storybook-root`?

    In the test-runner page is the preview iframe, so body picks up 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 divs and map to generic, which Playwright omits, so none of the harness leaks in.

  • feat: add check-aria-surface.ts, which diffs base vs head per story, pairs same-role removals/additions into renames, and renders the PR comment

  • feat: publish the aria surface of main as a 90-day aria-baseline artifact, and diff PRs against it

    Why this needs no second Storybook build

    The workflow already runs on push: main and visits all ~2.3k stories there, so the baseline costs one artifact merge. The PR side builds the merge commit (PR + current main), which lines up with a baseline taken from main. The alternative — building both sides per PR, as api-surface.yaml does — would double an 8-shard, ~30-minute job.

  • test: 33 unit tests, including a fixture pinned to verbatim Playwright 1.57 ariaSnapshot() output

    Why a real fixture?

    The parser is only as good as its grip on the snapshot format, and three things in the real output were not obvious: link targets are emitted as their own - /url: "#" entries and must not parse as a role; a trailing : x is the element's text content, not part of the name; and names are always double-quoted with backslash escapes. The fixture was captured from an actual chromium run, not hand-written.

  • chore: document the check in packages/react/AGENTS.md, including its coverage limit

Noise control

Counts rather than sets, so "three buttons named Remove became one" — a getAllByRole break — is caught. Named nodes are always tracked; unnamed ones only for roles tests query bare, with list/listitem/paragraph deliberately excluded as the noisiest structural roles. Ambiguous multi-renames stay split across removed/added rather than guessed at. A story with nondeterministic content can opt out with parameters: { ariaSnapshot: { skip: true } }.

Two degraded modes are handled explicitly rather than reported as PR breakage: a failed shard passes --partial so unrun stories aren't blamed on the PR, and an empty head side reports an infrastructure problem instead of "you deleted every story".

Known limits

  • Advisory — posts a comment, never fails. Deliberate: worth seeing the real churn before anyone's merge depends on it. breakingTotal is already returned for when it's flipped to blocking.
  • Coverage is bounded by what stories render. F0InputField's clear button only mounts once the field has a value, so that rename surfaced in the one story with a filled input. A conditional element with no story covering its state is invisible here.
  • The first PR after this merges gets "no baseline" until main runs once and publishes one. The comment says so explicitly.

Roles and accessible names are a public API — consumers query them with
getByRole/findByRole — but nothing in CI watched them for *change*:

  - the public API check diffs rolled-up .d.ts files, and an aria-label
    written inline in a component body is a value, not a type;
  - axe asks whether a name exists, never whether it is the same name;
  - Chromatic diffs pixels, not semantics.

Capture each story's role + accessible-name pairs in the existing
Storybook test-runner pass and diff them against the baseline published
on main. No second Storybook build: main already visits every story, so
the baseline costs one artifact merge.

Advisory for now — it posts a PR comment and never fails a build.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@sauldom102
sauldom102 requested a review from a team as a code owner August 24, 2026 12:56
@github-actions github-actions Bot added feat react Changes affect packages/react labels Aug 24, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Review policy: Feature

The PR title starts with feat, so this is a feature: it needs one approval from f0-devs AND one from f0-designers (rule 3).

Required approvals

Team Why Status
@factorialco/f0-devs Features need a dev approval ⏳ pending
@factorialco/f0-designers Features need a design approval ⏳ pending
How this was decided
  • PRs touching only sds/ modules require their owners and nothing else.
  • Otherwise, docs-only changes (*.md, *.mdx, *.stories.tsx, anything in __stories__/) → one f0-general approval.
  • Otherwise, feat: titles → one f0-devs and one f0-designers approval. Not a feature? Fix the title prefix.
  • Anything else → one f0-devs approval.
  • Add the needs-design-review label to also request a design approval on any PR.
  • Creating a new sds/ module (new package.yml) additionally requires an f0-general approval.

Policy source: ownership/review-policy.ts · Team members: ownership/teams.yml

@github-actions

Copy link
Copy Markdown
Contributor

✅ No New Circular Dependencies

No new circular dependencies detected. Current count: 0

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Visual review for your branch is published 🔍

Here are the links to:

@github-actions

Copy link
Copy Markdown
Contributor

📦 Alpha Package Version Published

Use pnpm i github:factorialco/f0#npm/alpha-pr-5217 to install the package

Use pnpm i github:factorialco/f0#83045fa37f1eaf9b001e462c910e87a3faa60bbf to install this specific commit

@github-actions

Copy link
Copy Markdown
Contributor

✅ No breaking public API changes

No public exports were removed, renamed, or had existing props/types changed in a breaking way compared to main.

Comparing f0, experimental and ai against main. Adding components, types, or optional props is safe. This check is non-blocking.

⚠️ Could not analyze component-status (no-base) — a build may have failed; results may be incomplete.

@github-actions

Copy link
Copy Markdown
Contributor

🔎 Accessible name & role changes

ℹ️ 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.

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.

@github-actions

Copy link
Copy Markdown
Contributor

♿ Accessibility (axe) — components changed in this PR

✅ No a11y issues in the stories this PR changed.

Scope: only stories in the files/component folders this PR changed. It can't yet flag downstream ripple from shared-code/token changes, or diff against main (planned: base-vs-head delta).

@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report for packages/react

Status Category Percentage Covered / Total
🔵 Lines 68.08% 28415 / 41737
🔵 Statements 67.08% 30063 / 44816
🔵 Functions 60.83% 6756 / 11106
🔵 Branches 60.79% 21158 / 34804
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/react/src/lib/storybook-utils/ariaSurface.ts 100% 100% 100% 100%
Generated in workflow #17392 for commit 4409bcf by the Vitest Coverage Report Action

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat react Changes affect packages/react

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant