-
Notifications
You must be signed in to change notification settings - Fork 16
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
section-alert: Improve announcements across screen readers #1893
Conversation
…ss screen readers
🦋 Changeset detectedLatest commit: 671eced 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 |
|
width="24" | ||
height="24" | ||
viewBox="0 0 24 24" | ||
aria-hidden={ariaHidden} |
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.
Just alphabetising and adding id
aria-hidden="false" | ||
aria-label="Success" | ||
aria-label="Error, " |
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.
🙏
'@ag.ds-next/react': minor | ||
--- | ||
|
||
icon: Add `id` as allowed prop. |
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.
Unused now, but it feels like it should still be added
}} | ||
> | ||
{icon} | ||
<span css={visuallyHiddenStyles} id={toneId}> |
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.
Wanted to use aria-label
, but there's no way of using it on a generic element that axe and ARC won't complain at. This means that iOS's virtual cursor is the size of the word and not the icon, but it's better than any alternative
alignItems="center" | ||
aria-labelledby={`${toneId} ${titleId} ${children ? childrenId : ''}`} |
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.
Wanted to use aria-describedby
for childrenId
, but that made things worse in NVDA (the first one I tested), so stopped trying that
Quality Gate passedIssues Measures |
The audt and my testing found that focusing the alert wasn't announcing it in JAWS and iOS 16.7.10 & 18 something, instead it just announced the graphic.
This change makes the alert a region, so not only does it appear in the landmarks menu (which is important since there is no heading to jump to), it improves the announcements across all screen readers. JAWS still doesn't announce it on the default setting, but changing to "high" does.
View preview
Checklist
Preflight
accordion: Updated padding
ordocs: Updated header links
yarn changeset
. Learn more about change management.Testing
yarn test
to ensure tests are passing. If required, runyarn test -u
to update any generated snapshots.