[test][button] Add axe test coverage and WCAG conformance report - #48916
[test][button] Add axe test coverage and WCAG conformance report#48916michelengelen wants to merge 17 commits into
Conversation
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
9739034 to
3887296
Compare
212555a to
9b9147b
Compare
d60b37a to
5c23780
Compare
PR reviewNo findings. This PR sets the template for the stack and it is internally consistent. Checked: the demoMeta enrolment matches the 15 keys in Residual risk: the 371-line conformance report was spot-checked against the code, not verified claim by claim, and the new demos are regression-harness-only (not referenced from VerdictApprove - the enrolment, fixtures, tests, and report counts are consistent end to end. 🤖 Review generated with Claude Code |
JCQuintas
left a comment
There was a problem hiding this comment.
Reviewed the last commit only (5c23780), against master with #48915 and #48936 already in.
Looks good overall: the counts are self-consistent (23 + 4 + 0 + 28 = 55, which is exactly WCAG 2.2 A/AA once 4.1.1 is dropped, and the 8/27 flag count matches the body), the harness wiring is right (ButtonA11y* opted out of screenshots with ButtonA11yTextSpacing re-enabled by last-match-wins), the non-native tabIndex=-1 claim checks out against useButtonBase.ts, and all doc links resolve. Comments below are mostly wording accuracy on the report, since this text ends up published in #48926.
The one I would not merge without: the 1.4.3 known gap names only contained, but the same two palette colors fail as text and outlined labels too.
Two housekeeping notes:
- The branch is
CONFLICTINGwith master.Button.test.jsmoved toimport { describe, expect, it, vi } from 'vitest'upstream, so the addedimport { expect } from 'chai'hunk collides on rebase.sinon'sspyis still fine, 57 files inpackages/mui-material/srcstill use it. - Argos shows 1 added screenshot, which is
ButtonA11yTextSpacing. Expected, that is the demo the rule deliberately keeps enabled.
Non-blocking observation: ButtonA11yTextSpacing overlaps what #48936 already automates, since registerCssLayoutSuites injects TEXT_SPACING_CSS on /docs-components-buttons/BasicButtons. The inline comment already says it adds no unique axe coverage, so the value here is purely the permanent Argos snapshot. Fine if that is intended, just worth being explicit that it is a screenshot we now maintain forever.
5c23780 to
796e748
Compare
Each component PR appends its own row when its report lands. Empty rows read as gaps instead of work in progress. See mui#48916 (comment) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HgD6GEW3UxNRvC2abbTy9
Applies mui#48916 (comment): the shared reports table starts empty, and each component PR appends its own row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HgD6GEW3UxNRvC2abbTy9
Applies mui#48916 (comment): the shared reports table starts empty, and each component PR appends its own row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HgD6GEW3UxNRvC2abbTy9
Applies mui#48916 (comment): the shared reports table starts empty, and each component PR appends its own row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HgD6GEW3UxNRvC2abbTy9
Applies mui#48916 (comment): the shared reports table starts empty, and each component PR appends its own row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HgD6GEW3UxNRvC2abbTy9
Applies mui#48916 (comment): the shared reports table starts empty, and each component PR appends its own row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HgD6GEW3UxNRvC2abbTy9
Applies mui#48916 (comment): the shared reports table starts empty, and each component PR appends its own row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HgD6GEW3UxNRvC2abbTy9
Applies mui#48916 (comment): the shared reports table starts empty, and each component PR appends its own row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HgD6GEW3UxNRvC2abbTy9
Applies mui#48916 (comment): the shared reports table starts empty, and each component PR appends its own row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HgD6GEW3UxNRvC2abbTy9
…he contrast ratio is off
| describe('parseRoute', () => { | ||
| it('returns null for non-component routes (regression fixtures)', () => { | ||
| expect(parseRoute('/regression-Rating/FocusVisibleRating')).to.equal(null); | ||
| it('parses a regression fixture route into its fixtures path, suite as slug', () => { |
There was a problem hiding this comment.
is this related to the PRs scope?
There was a problem hiding this comment.
Yes, simply because the ButtonA11y* demos now live under test/regressions/fixtures/buttons/ (it was under the docs demos previously). axe only runs on routes parseRoute can map to. That's why this is necessarry here. Nice catch though! :D
JCQuintas
left a comment
There was a problem hiding this comment.
Re-reviewed at c423943. All 22 checks green, branch is mergeable again, and every point from the last round is addressed: the 1.4.3 table matches the WCAG math independently (3.11:1 for warning, 3.86:1 for info, closest pass primary at 4.6:1), the version reads 9.x, the 3.2.2 test now actually flips state via setProps, the reports index is trimmed to the row that exists, and the fixtures move resolves the "do not add docs demos" thread.
Four follow-ups, one of which I would fix before merge: the new contrast tests do not pin what they claim to pin.
| } | ||
| const fixtureMatch = route.match(FIXTURE_ROUTE_REGEX); | ||
| if (fixtureMatch) { | ||
| const [, suite, demo] = fixtureMatch; |
There was a problem hiding this comment.
Using the suite directory as the slug means a fixture route can write outside the slug it belongs to. A11yReporter does mkdirSync(path.join(COMPONENTS_DIR, slug), { recursive: true }), so enrolling any existing PascalCase suite for a11y (say fixtures/Rating/) writes docs/data/material/components/Rating/Rating.a11y.json.
That fails differently per platform, which is the unpleasant part: on Linux CI it is a brand new directory sitting next to the real rating/, on case-insensitive macOS it lands inside rating/ under a wrong-case filename. The docstring above asks for a lowercase docs slug, but nothing enforces it.
Cheap guard in the reporter: only write when the slug directory already exists, and throw with the route name otherwise, instead of recursive: true quietly creating it.
There was a problem hiding this comment.
Agreed, fixed in 7e7d4ec with one change to the suggestion. existsSync folds case on macOS. There, a check for Rating matches rating/, and the guard fires only on Linux CI. The reporter now compares each slug against the exact names from readdirSync. A slug without a docs directory throws, with the demo names in the message.
| @@ -0,0 +1,32 @@ | |||
| import Box from '@mui/material/Box'; | |||
There was a problem hiding this comment.
Naming confusion worth heading off: test/regressions/fixtures/Button/ already exists (screenshot fixtures, PascalCase component name), and this adds test/regressions/fixtures/buttons/ (a11y fixtures, lowercase docs slug). Same component, two sibling directories that differ only by case and a plural, holding fixtures for different tools.
There was a problem hiding this comment.
Agreed, and the problem is larger than naming. A docs slug can differ from a screenshot suite only by case, for example rating and Rating. Case-insensitive file systems fold these into one directory. I moved the a11y suites to test/regressions/a11y/fixtures/{slug}/ in 8bc6648. These routes use their own /a11y-{slug}/{Demo} prefix now. Screenshots are off by default for this tree. The reporter also rejects a suite name that is not a docs slug (7e7d4ec).
The contrast guard moves to theme-level contract tests in a separate PR. Parsing the report table from a unit test was brittle, and the palette facts are not Button-specific. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HgD6GEW3UxNRvC2abbTy9
A11y fixture suites use lowercase docs slugs as directory names. A
lowercase suite next to a PascalCase screenshot suite can differ only
by case, for example rating/ next to Rating/. Case-insensitive file
systems fold these into one directory. The new tree
test/regressions/a11y/fixtures/{slug}/ removes this risk. These routes
now use the /a11y-{slug}/{Demo} prefix. Screenshots are off by default
for this tree. Plain /regression-* routes are screenshot-only again.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013HgD6GEW3UxNRvC2abbTy9
The reporter created missing slug directories with mkdirSync. A badly named fixture suite could then write outside the docs tree. The reporter now compares each slug against the exact names from readdirSync and throws on a miss. existsSync is not sufficient: it folds case on macOS, so Rating would match rating/ and fail only on Linux CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HgD6GEW3UxNRvC2abbTy9
Expands axe coverage for
Buttonand adds its WCAG 2.2 Level A/AA conformance report, plus the shared reports legend atpackages/mui-material/src/accessibility.md.Known gaps: contrast on
info/warningcontained buttons (1.4.3), focus-indicator contrast and its removal viadisableRipple(1.4.11, 2.4.7), and no live region for theloadingstate (4.1.3).Report:
Button/accessibility.mdImportant
How to review this PR
Layer 3 of 14 in a stacked series (#48915 → #48926). Its branch is built on
a11y/stack/css-layout(#48936). GitHub cannot chain PR bases across a fork, so every PR in the series targetsmasterinstead.Review only the last commit —
5c23780. Everything above it belongs to the layers below, and disappears as those merge.Original work by @mj12albert. Supersedes #48708, rebased onto current master.
Part of the WCAG conformance effort (#14187).