Uh oh!
There was an error while loading. Please reload this page.
fix(html-reporter): use a button element for the chip header - #42149
Conversation
The chip header is a <div role="button">, so it cannot take focus and the collapsible sections of a test - Errors, Test Steps, Screenshots, Traces, Attachments - cannot be expanded or collapsed with a keyboard at all. Make it a button and add a :focus-visible outline, the same way tab elements were converted.
There was a problem hiding this comment.
🟡 Changes recommended
Chip now renders a focusable <button> even when setExpanded is undefined, creating a misleading non-actionable control for keyboard/AT users.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Improves keyboard accessibility in the html-reporter by converting Chip section headers from a non-focusable div role="button" to a native <button> and adding a keyboard interaction test.
Changes:
- Replace chip header container with a
<button>so it can receive focus and handle Enter/Space natively. - Add
:focus-visibleoutline styling for the chip header. - Add a spec that verifies keyboard expand/collapse behavior.
File summaries
| File | Description |
|---|---|
| packages/html-reporter/src/chip.tsx | Converts the chip header wrapper to a native button element. |
| packages/html-reporter/src/chip.css | Styles the new button-based header and adds a :focus-visible outline. |
| packages/html-reporter/src/chip.spec.ts | Adds a keyboard-focused test for expand/collapse. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| <button | ||
| aria-expanded={!!expanded} | ||
| aria-controls={id} | ||
| className={clsx('chip-header', setExpanded && ' expanded-' + expanded)} | ||
| onClick={() => setExpanded?.(!expanded)} | ||
| title={typeof header === 'string' ? header : undefined}> | ||
| {setExpanded ? (expanded ? <icons.downArrow /> : <icons.rightArrow />) : <icons.spacer />} | ||
| {header} | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
I agree with Copilot! SlowestTests uses this component in a static way, where it's not actually expandable. Denis Skvortsov (@skvortsov-dev) can you maybe update this so we switch between button and div depending on whether the element is expandable?
There was a problem hiding this comment.
Done — the header is a <button type="button"> only when the chip is expandable, a plain <div> otherwise.
The Speedboard aria snapshots needed - text: /Slowest Tests/ rather than an exact match: without a role the header text coalesces with the neighbouring text node, which carries the run timestamp. chip.spec.ts also gets a case asserting a non-expandable chip exposes no button.
Component tests 17 passed, reporter-html.spec.ts + reporter-blob.spec.ts 238 passed, npm run lint clean.
SlowestTests renders Chip without setExpanded, so that header is static. Render a button only when the chip is expandable and a plain div otherwise, and mark the interactive case with type='button'. The aria snapshots in reporter-html.spec.ts are updated accordingly: the Speedboard header is no longer exposed as a button.
There was a problem hiding this comment.
🟢 Ready to approve
The change correctly improves keyboard accessibility with native buttons, updates styling, and includes targeted tests and snapshot updates to validate the new behavior.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"1 flaky51238 passed, 1190 skipped Merge workflow run. |
Test results for "MCP"1 failed 8049 passed, 1284 skipped Merge workflow run. |
Rationale
Came across this while reading through the html-reporter components. The chip header — every collapsible section of a test (
Errors,Test Steps,Screenshots,Traces,Attachments) and every file row in the list — is a<div role="button">. A div can't take focus, so those sections cannot be expanded or collapsed with a keyboard at all:$$('.chip-header').map(e => e.tabIndex)returns-1for each and Tab walks past them, even though the header already carriesaria-expanded/aria-controlsandchip.spec.tsasserts it as a button in an aria snapshot.This converts the header to a
<button>and adds a:focus-visibleoutline, the same way tab elements were converted in #41440 and #41434. No key handlers — Enter and Space come from the platform.Before — Tab walks past every section header, so nothing can be expanded from the keyboard:
chip-keyboard-check-before.mov
After — the headers take focus, Enter and Space toggle them:
chip-keyboard-check-after.mov
Test
chip.spec.tsgets a keyboard case: the header takes focus and toggles on Enter and Space. On the current<div>it fails withtoBeFocusedreceivinginactive.