-
Notifications
You must be signed in to change notification settings - Fork 647
Fix: Ensure dynamically updated label is announced #4537
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
|
size-limit report 📦
|
|
@khiga8 just wanted to share example usage of Something interesting is that there are two instances of this where both |
|
EEEP @joshblack thank you for the primer query link!!! I'll have to dive into it next week! |
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.
Looks great, thanks for doing this! 👍 Just need snapshots updated and should be good to go
6610598 to
41cbd79
Compare
| const [count, setCount] = useState(0) | ||
| const onClick = () => { | ||
| setCount(count + 1) | ||
| announce(`Watch ${count + 1}`) |
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.
Hey @joshblack! I decided to update the approaches in these examples to use a live region, based on the conclusion I came to that the aria-live hack shouldn't be used, unless it's already being considered for other reasons.
I am thinking the next steps for me after this PR are:
- a dedicated storybook page that dives into different approaches for communicating button state.
- adding a section to the interface guidelines.
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.
Ah okay, sounds good @khiga8 👍
lindseywild
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.
Looks good from an accessibility perspective!
c910d22 to
b7086c1
Compare
* Fix: Ensure dynamically updated label is announced * Use Link component * Fix circular dependency * Update approach * Update packages/react/src/Box/Box.features.stories.tsx * test(vrt): update snapshots --------- Co-authored-by: khiga8 <[email protected]>
Closes https://github.com/github/accessibility-audits/issues/5629
Changelog
I'm addressing an audit ticket that points out how the dynamically updated counter button in Storybook label is not announced when incremented. It turns out there isn't really a standardized approach to ensure announcements happen when a button is dynamically updated.
I ended up testing various approaches across different screen reader/browser combinations which you can check out in https://github.com/github/accessibility/pull/6222. (I welcome your feedback on that PR).
Ultimately through my testing, I found that dynamically updating the
aria-labelperforms the best in terms of ensuring the button state is communicated. However, this is a brittle approach so I recommend using a live region announcement (unless anaria-labelis already being set on the button).This PR updates the storybook stories accordingly with a note.
Rollout strategy
Testing & Reviewing
See my changes 🔍
Videos from testing across multiple screen reader/browsers
Merge checklist