-
Notifications
You must be signed in to change notification settings - Fork 1k
USWDS - Step indicator: Remove aria-label from wrapper #6146
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
@amycole501 and/or @alex-hull, can you confirm that this fix resolves the SiteImprove error described in the related issue? |
There was a problem hiding this comment.
Choose a 8000 reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the label is the cleanest and most straightforward fix. Thank you!
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 to me! Left one comment about a small change in screen reader behavior.
Testing checklist
- If available, confirm that this issue is now resolved in SiteImprove scans
- Confirm that the
aria-label
has been successfully removed - Confirm that removing the
aria-label
is appropriate - Confirm no regressions
@@ -1,4 +1,4 @@ | |||
<div class="usa-step-indicator {{ modifier }}" aria-label="progress"> | |||
<div class="usa-step-indicator {{ modifier }}"> |
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.
Note (non-blocking): I noticed that my screen reader no longer focuses the entire time picker "group".
Before:
- Screen reader focuses entire component group
- Scr 8000 een reader then focuses step indicator bar and reads through steps
- Screen reader then reads out header element
After:
- Screen reader focuses step indicator bar and reads through steps
- Screen reader then focuses header element
This doesn't seem to be an issue but wanted to flag since the behavior changes slightly
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.
Thanks for flagging!
@amycole501 and @alex-hull, would this change in behavior count as an accessibility regression? Wanting to check in with you both before moving forward with this.
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.
I agree with Charlie. I don't think this will be an issue. It appears that the flow is not that much different between the before and after interaction, so that should not be too big of a change. I also think as long as the user hears what current step they are on, this should not be an issue. @amyleadem
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.
Address changes to focus on whole component. Approve on my end.
Summary
Removed the aria-label from the wrapper of the step indicator component. This resolves an automated testing error related to having an invalid attribute on a
div
element.Markup change
To remove automated testing errors, users must update the markup to remove the
aria-label
on theusa-step-indicator
element:Breaking change
This is not a breaking change.
Related issue
Closes #6113
Related pull requests
Changelog PR
Preview link
Step indicator component
Problem statement
The step indicator component fails SiteImprove accessibility tests because it has an invalid
aria-label
attribute on a<div>
element.Solution
This PR removes the
aria-label
from the.usa-step-indicator
div element. This was done for the following reasons:aria-label
attribute is not valid on a<div>
element. In some cases this means the label is not read out, and in others, it causes accessibility scan failures.aria-label
was determined to be an unnecessary addition. The accessibility team reported that user interviews suggested the label did not provide necessary context.Testing and review
aria-label
has been successfully removedaria-label
is appropriate