-
Notifications
You must be signed in to change notification settings - Fork 646
Replace aria-live and LiveRegion usage
#7230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 7566471 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR modernizes the accessibility announcement system by replacing direct aria-live attributes and the internal LiveRegion component with the newer AriaStatus helper from the live-region module. This provides a more maintainable and React-friendly approach to screen reader announcements across the design system.
Key Changes
- Removed the internal
LiveRegioncomponent entirely and all its dependencies - Migrated all
aria-liveattribute usage to theAriaStatuscomponent from the live-region module - Updated tests to use the
getLiveRegion()test helper for verifying announcements
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/internal/components/LiveRegion.tsx | Removed the entire internal LiveRegion component implementation |
| packages/react/src/experimental/SelectPanel2/SelectPanel.tsx | Replaced aria-live attributes with AriaStatus component for status messages |
| packages/react/src/TreeView/TreeView.tsx | Replaced VisuallyHidden with aria-live attributes with AriaStatus component |
| packages/react/src/TreeView/TreeView.test.tsx | Updated tests to use getLiveRegion() helper and improved test robustness by using getByRole |
| packages/react/src/Skeleton/Skeleton.examples.stories.tsx | Wrapped loading announcements in AriaStatus component |
| packages/react/src/DataTable/Pagination.tsx | Removed LiveRegion/Message usage and replaced with AriaStatus for pagination announcements |
| .changeset/easy-suits-mate.md | Added changeset documenting the changes as a patch release |
| <VisuallyHidden role="status" aria-live="polite" aria-atomic="true"> | ||
| {ariaLiveMessage} | ||
| <VisuallyHidden> | ||
| {/* Message fail without passing a `key` */} |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment has a grammar issue. It should be "Messages fail" (plural) or "Message fails" (singular with correct verb form) instead of "Message fail".
| {/* Message fail without passing a `key` */} | |
| {/* Message fails without passing a `key` */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, we're getting rid of the key. Thanks though.
| // Set up live region for this test | ||
| const liveRegionEl = document.createElement('live-region') | ||
| document.body.appendChild(liveRegionEl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The live region utilities should be doing this automatically, let me know if not 👀
I think test-wise we just have to have a clean up for them, e.g.
afterEach(() => {
const liveRegion = document.querySelector('live-region');
document.body.removeChild(liveRegion);
});There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I thought too, but they weren't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mperrotti if I remove these lines from the test it still seems to pass, are you seeing that too or no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, that's odd. The failures may have been caused by something else I changed. Will remove.
| <VisuallyHidden role="status" aria-live="polite" aria-atomic="true"> | ||
| {ariaLiveMessage} | ||
| <VisuallyHidden> | ||
| {/* Message fail without passing a `key` */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooo what issue were you running into without key? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just wasn't picking up the content. Here's the test failure when I don't pass a key:
FAIL @primer/react (chromium) src/TreeView/TreeView.test.tsx > Asynchronous loading > updates aria live region when loading is done
AssertionError: expected '' to be 'Parent content loading' // Object.is equality
- Expected
+ Received
- Parent content loading
❯ toBe src/TreeView/TreeView.test.tsx:1442:44
1440| fireEvent.click(doneButton)
1441|
1442| expect(liveRegion.getMessage('polite')).toBe('Parent content loading')
| ^
1443|
1444| // Click done button to mimic the completion of async loading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mperrotti it seems like the test is complaining about updates not being wrapped in act() when debugging locally, are you seeing that as well? When updating to the following it seemed to work without key:
// Click load button to mimic async loading
await act(async () => {
await user.click(doneButton)
})There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh ok I see! Will update. Thank you!
Co-authored-by: Josh Black <joshblack@github.com>
joshblack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/8147 |
|
🔴 ci completed with status |
Closes https://github.com/github/primer/issues/4892
Changelog
New
Changed
Removed
LiveRegioncomponentRollout strategy
Testing & Reviewing
Merge checklist