-
Notifications
You must be signed in to change notification settings - Fork 115
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
Ensure counter content is exposed to screen reader users #3270
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 49caf4b The changes in this PR will be included in the next version bump. 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 |
This comment was marked as outdated.
This comment was marked as outdated.
@@ -9,8 +9,8 @@ | |||
<% if trailing_visual %> | |||
<span class="Button-visual Button-trailingVisual"> | |||
<% if @trailing_visual_counter %> | |||
<span class="d-flex" aria-hidden="true"><%= trailing_visual %></span> |
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.
Setting flex on this newly added span to ensure content remains centered.
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.
No visual regression - good sign!
- button "Button ( )": Button () | ||
- button "Button ( )": Button () | ||
- button "Button ( )": Button () | ||
- button "Button ( 15 )" |
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.
🎉
This comment was marked as off-topic.
This comment was marked as off-topic.
Just merged in the latest from |
This comment was marked as outdated.
This comment was marked as outdated.
Apply flex so nested span is centered Temp
What are you trying to accomplish?
This PR ensures the counter content is exposed to screen reader users.
Before accessible name -
comment ()
After accessible name -
comment (15)
This removes the
aria-hidden
attribute from the counter component and setssr-only
oraria-hidden: true
directly on respective counters in the view file. We want to render one span visually for sighted users (without parenthesis), and one span for screen reader users (with parenthesis to help the announcement).This unintentional bug was introduced in #2807 😓 .
Screenshots
Integration
List the issues that this change affects.
https://github.com/github/accessibility-audits/issues/10088
Risk Assessment
What approach did you choose and why?
Anything you want to highlight for special attention from reviewers?
Accessibility
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.