A11y: Add aria-live announcements via @react-aria/live-announcer#33970
Conversation
|
@copilot fix the PR description with a "#### Manual testing" section for reviewers |
|
View your CI Pipeline Execution ↗ for commit 21f483c
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit 33b2422 ☁️ Nx Cloud last updated this comment at |
Added a "#### Manual testing" section to the PR description with step-by-step instructions covering: starting Storybook UI, verifying each story (Default, Visible, DynamicUpdates, VisibleFormError, PolitenessToggle), screen reader testing, and running unit tests. |
Sidnioulz
left a comment
There was a problem hiding this comment.
As it is, overriding announce from react-aria seems useless. Our hook provides no QoL advantage.
The component also seems useless. It does not even use the hook.
Remove both the hook and component from the codebase.
Then, modify the TestProvider / vitest addon to use react-aria's announce function. And use the toHaveLiveRegion checker to verify that live announcements are created. I want you to announce when a test run starts and finishes (announcing if there's an error, and then how many components errored/have warnings/passed)
Done in dbb64bd. Removed both the Integrated
Added 3 stories with play functions ( |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds accessibility announcements to the vitest addon's test provider component for lifecycle events (start, completion, crash) using live regions, alongside a new custom Vitest matcher for testing live-region content and comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test/Story
participant Component as TestProviderRender
participant Announcer as Live Announcer
participant Region as Live Region
participant SR as Screen Reader
Test->>Component: Update testProviderState
Component->>Announcer: announce(message, level)
Announcer->>Region: Inject text into aria-live region
Region->>SR: Notify (polite/assertive)
SR->>SR: Queue/Read Announcement
Test->>Test: waitFor() + toHaveLiveRegion()
Test->>Region: Query aria-live element for text match
Region-->>Test: pass/fail assertion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
code/core/src/manager/utils/toHaveLiveRegion.ts (1)
18-19: Aligntextcontract with implementation.The docs say string matching is exact, but the implementation currently uses substring matching (
includes). Please update either the docs or logic so behavior is explicit.Also applies to: 59-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/manager/utils/toHaveLiveRegion.ts` around lines 18 - 19, The comment for the toHaveLiveRegion matcher is inconsistent with its implementation: if the intent is exact string matching, update the implementation in the toHaveLiveRegion matcher (function toHaveLiveRegion) to use strict equality (===) when text is a string instead of substring matching (includes); otherwise change the doc comment on the text property to state that string matching is a substring match. Locate the matching logic in toHaveLiveRegion and replace the string branch to use === for exact match (or adjust the comment on the text field to say "substring match") and ensure the same change is applied to the other occurrence referenced at the second location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/addons/vitest/src/components/TestProviderRender.stories.tsx`:
- Around line 337-345: The announcement assertions run immediately after mount
but announce() from `@react-aria/live-announcer` updates the DOM asynchronously,
so wrap the live-region expectation in an async wait helper (e.g., await
waitFor(() => expect(document.body).toHaveLiveRegion({ text: 'Test run
started.', level: 'polite' })) or use a find-style helper) to retry until the
live region appears; update each play block that mounts TestProviderRender
(references: the play function, mount, TestProviderRender, and announce) to
await the waitFor-based assertion instead of asserting immediately after mount.
In `@code/addons/vitest/src/components/TestProviderRender.tsx`:
- Around line 180-201: The announce urgency only checks errorCount so runs that
fail solely due to accessibility issues are announced as polite; update the
logic in TestProviderRender (around hasA11yAddon / a11yErrorCount) to treat
accessibility errors as assertive too by using a combined error check (e.g.,
compute a totalErrorCount = errorCount + (hasA11yAddon ? a11yErrorCount : 0) or
otherwise include a11yErrorCount in the condition) and then call
announce(message, totalErrorCount > 0 ? 'assertive' : 'polite'); reference the
variables/function names a11yErrorCount, hasA11yAddon, errorCount, announce, and
message when making the change.
In `@code/core/package.json`:
- Line 266: Add the missing devDependency "@react-aria/live-announcer" to the
vitest addon package by updating code/addons/vitest/package.json: add
"@react-aria/live-announcer": "^3.4.4" (or match the version used in core) under
devDependencies so imports in TestProviderRender.tsx and
TestProviderRender.stories.tsx resolve correctly during strict workspace
installs; then run install to update lockfiles.
In `@code/core/src/manager/utils/toHaveLiveRegion.ts`:
- Around line 57-60: The RegExp used in the text matching may have global/sticky
flags and .test() mutates lastIndex, causing false negatives across multiple
elements; in the mesh that computes textMatches (where variable text is used and
textMatches is assigned), detect if text is a RegExp (instanceof RegExp) and
reset text.lastIndex = 0 before calling .test(content) so each element test
starts from the beginning.
---
Nitpick comments:
In `@code/core/src/manager/utils/toHaveLiveRegion.ts`:
- Around line 18-19: The comment for the toHaveLiveRegion matcher is
inconsistent with its implementation: if the intent is exact string matching,
update the implementation in the toHaveLiveRegion matcher (function
toHaveLiveRegion) to use strict equality (===) when text is a string instead of
substring matching (includes); otherwise change the doc comment on the text
property to state that string matching is a substring match. Locate the matching
logic in toHaveLiveRegion and replace the string branch to use === for exact
match (or adjust the comment on the text field to say "substring match") and
ensure the same change is applied to the other occurrence referenced at the
second location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0654d110-bcd1-486a-a7a0-1ea05fe9a72c
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
code/addons/vitest/src/components/TestProviderRender.stories.tsxcode/addons/vitest/src/components/TestProviderRender.tsxcode/core/package.jsoncode/core/src/manager/utils/toHaveLiveRegion.test.tscode/core/src/manager/utils/toHaveLiveRegion.ts
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/addons/vitest/src/components/TestProviderRender.stories.tsx (1)
377-379: Prefer event-driven transitions over fixed timers in stories.Using
setTimeout(..., 100)in Line 378 and Line 411 adds avoidable timing sensitivity. Consider triggering these transitions from explicit mocked actions (similar toAnnouncesTestRunStart) so the play assertions are fully deterministic.Also applies to: 410-412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/components/TestProviderRender.stories.tsx` around lines 377 - 379, The story currently uses setTimeout to flip state (e.g., setState('test-provider-state:succeeded')) which causes flaky timing; replace the timer-based transition with an explicit, testable mock action: add a named trigger function (e.g., triggerTestSucceeded) that calls setState('test-provider-state:succeeded') and expose it to the story's play via the same mocked-actions pattern used by AnnouncesTestRunStart, then remove the setTimeout blocks (including the other occurrence around lines 410-412) and ensure the play function invokes triggerTestSucceeded deterministically; also return a cleanup that clears any listeners/subscriptions established by the mock action.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/addons/vitest/src/components/TestProviderRender.stories.tsx`:
- Around line 377-379: The story currently uses setTimeout to flip state (e.g.,
setState('test-provider-state:succeeded')) which causes flaky timing; replace
the timer-based transition with an explicit, testable mock action: add a named
trigger function (e.g., triggerTestSucceeded) that calls
setState('test-provider-state:succeeded') and expose it to the story's play via
the same mocked-actions pattern used by AnnouncesTestRunStart, then remove the
setTimeout blocks (including the other occurrence around lines 410-412) and
ensure the play function invokes triggerTestSucceeded deterministically; also
return a cleanup that clears any listeners/subscriptions established by the mock
action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0ab73ff-f205-47ec-8a1e-9504f9d1fdb2
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
code/.storybook/storybook.setup.tscode/addons/vitest/src/components/TestProviderRender.stories.tsxcode/addons/vitest/src/components/TestProviderRender.tsxcode/addons/vitest/src/test-matchers.d.tscode/core/package.json
✅ Files skipped from review due to trivial changes (3)
- code/.storybook/storybook.setup.ts
- code/addons/vitest/src/test-matchers.d.ts
- code/core/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- code/addons/vitest/src/components/TestProviderRender.tsx
eddf60f to
1ceed73
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
code/core/src/shared/utils/toHaveLiveRegion.test.ts (1)
6-91: Add a regression case for<output>withlevel: 'polite'.Given this suite validates inferred politeness, add one test for
<output>so implicit level handling is covered and doesn’t regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/shared/utils/toHaveLiveRegion.test.ts` around lines 6 - 91, Add a regression test to toHaveLiveRegion.test.ts that asserts an <output> element is treated as polite: create a container with '<output>Output update</output>' and add an it block that calls expect(container).toHaveLiveRegion({ text: 'Output update', level: 'polite' }) and also include the inverse expect(() => expect(container).toHaveLiveRegion({ text: 'Output update', level: 'assertive' })).toThrow() to ensure implicit level inference for role-like elements is covered for the toHaveLiveRegion matcher.code/core/src/shared/utils/toHaveLiveRegion.ts (1)
17-18: Align matcher docs with implemented string matching semantics.Line 17 says string matching is exact, but Line 66 uses substring matching (
includes). Please either update docs or switch implementation to exact equality.Also applies to: 65-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/shared/utils/toHaveLiveRegion.ts` around lines 17 - 18, The docs say the matcher option text is an exact string but the matcher implementation uses substring matching (includes); update the implementation in the toHaveLiveRegion matcher so string comparisons use exact equality and RegExp comparisons require a full-string match: inside the toHaveLiveRegion matcher (the function that reads the text option), replace any use of includes for string matching with a strict === comparison against the element's full text, and when text is a RegExp, test it against the entire string by anchoring the pattern (e.g. construct a new RegExp like new RegExp(`^(?:${pattern.source})$`, pattern.flags) and use .test(fullText)) so the runtime behavior matches the documented semantics for the text property.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/shared/utils/toHaveLiveRegion.test.ts`:
- Line 4: The test file imports a local helper without the required TypeScript
extension; update the relative import in toHaveLiveRegion.test.ts from the
extensionless form to include ".ts" (i.e., change the import statement that
references './toHaveLiveRegion' to './toHaveLiveRegion.ts'), then run the
project's lint/tests to verify the import rules are satisfied; target the import
statement in toHaveLiveRegion.test.ts to make this change.
In `@code/core/src/shared/utils/toHaveLiveRegion.ts`:
- Around line 47-55: The level-inference block for computing effectiveLevel
doesn't treat <output> as implicitly polite, so toHaveLiveRegion can falsely
fail for output elements; update the logic that sets effectiveLevel (the branch
using role === 'status' || role === 'log' and role === 'alert') to include role
=== 'output' in the polite case (i.e., treat 'status', 'log', and 'output' as
polite) while keeping 'alert' -> 'assertive' behavior, referencing the
effectiveLevel, ariaLive and role identifiers to locate the change.
---
Nitpick comments:
In `@code/core/src/shared/utils/toHaveLiveRegion.test.ts`:
- Around line 6-91: Add a regression test to toHaveLiveRegion.test.ts that
asserts an <output> element is treated as polite: create a container with
'<output>Output update</output>' and add an it block that calls
expect(container).toHaveLiveRegion({ text: 'Output update', level: 'polite' })
and also include the inverse expect(() => expect(container).toHaveLiveRegion({
text: 'Output update', level: 'assertive' })).toThrow() to ensure implicit level
inference for role-like elements is covered for the toHaveLiveRegion matcher.
In `@code/core/src/shared/utils/toHaveLiveRegion.ts`:
- Around line 17-18: The docs say the matcher option text is an exact string but
the matcher implementation uses substring matching (includes); update the
implementation in the toHaveLiveRegion matcher so string comparisons use exact
equality and RegExp comparisons require a full-string match: inside the
toHaveLiveRegion matcher (the function that reads the text option), replace any
use of includes for string matching with a strict === comparison against the
element's full text, and when text is a RegExp, test it against the entire
string by anchoring the pattern (e.g. construct a new RegExp like new
RegExp(`^(?:${pattern.source})$`, pattern.flags) and use .test(fullText)) so the
runtime behavior matches the documented semantics for the text property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90d7e8fd-3e5f-4c26-96e8-1fe68fd552e1
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
code/.storybook/storybook.setup.tscode/addons/vitest/src/components/TestProviderRender.stories.tsxcode/core/package.jsoncode/core/src/shared/utils/toHaveLiveRegion.test.tscode/core/src/shared/utils/toHaveLiveRegion.tscode/core/vitest.d.tscode/vitest-setup.ts
✅ Files skipped from review due to trivial changes (2)
- code/.storybook/storybook.setup.ts
- code/core/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- code/addons/vitest/src/components/TestProviderRender.stories.tsx
Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com>
… into TestProviderRender Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com>
Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com>
…tils, clean up stories Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com>
…d a11y assertive, refactor stories Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com>
Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com>
914085b to
9dd51cd
Compare
Storybook UI lacks a centralized approach for managing
aria-liveannouncements, making screen reader users unable to perceive status changes (test results, loading states, errors, etc.).Changes
extend-to-be-announcedintegration — Addedextend-to-be-announced(by @AriPerkkio) as a devDependency in core for robust ARIA live region assertion viaaria-live-capture. Registered incode/.storybook/storybook.setup.tsviaimport 'extend-to-be-announced/vitest'.announce()from@react-aria/live-announcerdirectly into the vitest addon'sTestProviderRendercomponent to announce test run lifecycle events:polite) when tests beginassertiveif component test errors or accessibility errors are present,politeotherwise; includes a11y error/warning counts when the a11y addon is present)assertive) on fatal errorsplayfunctions onTestProviderRenderusingrenderwithuseStateand mock-driven state transitions (via mockedstore.sendorsetTimeout) to verify announcements viawaitFor+toBeAnnounced(AnnouncesTestRunStart,AnnouncesTestRunFinished,AnnouncesTestRunCrashed). Tagged withvitestfor vitest-only test execution.@react-aria/live-announcerandextend-to-be-announcedare added as devDependencies incode/core/package.json(required by the pre-bundling system).Usage
Manual testing
cd code && yarn storybook:ui(requires compilation first viayarn nx run-many -t compile -c production)AnnouncesTestRunStart,AnnouncesTestRunFinished,AnnouncesTestRunCrashedstories pass their play function assertionsOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
Summary by CodeRabbit
Release Notes
New Features
Tests