-
Notifications
You must be signed in to change notification settings - Fork 88
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
fix(a11y): move aria-label from svg icon to button #2640
fix(a11y): move aria-label from svg icon to button #2640
Conversation
@brandonlenz Hey stranger 😄 No issues from my side. I think my original solution was based on the USWDS example code which uses an |
203a7bd
to
58548ec
Compare
Some additional context - these changes will bring the NavCloseButton default behavior closer to parity with ModalCloseButton:
The aria-label being present on the svg within the button is probably not hugely different from applying it directly to the button, except that our a11y tests can't seem to drill down into the svg as content to satisfy the button requirements. See this error message when using the ExtendedNav default:
EDIT: would it be acceptable to add an |
@seevee I think that sounds fine. @shkeating this one is pretty accessibility focused. Do you have any thoughts about the changes or suggestion? |
{...buttonProps} | ||
type="button"> | ||
<Icon.Close size={3} aria-label="Close" /> |
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 is an acceptable implementation to me, my ANDI output (which shows the text data that is received by screen readers visually) shows the close button labelled as 'Close' as intended.
However, I would encourage you to get more specific in the accessible name. Let's say we had another component in a page layout that also had a close icon. If both are named "close" they would be indistinguishable for a screen reader user navigating by links on a page.
the aria label is correct but I would change it to "close menu" or "close navigation" to avoid duplicate accessible names. If there was two components using close buttons under this pattern in the same page layout, we would see this ANDI error: ⚠ Non-unique button: same name/description as another button.
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.
What about enabling the prop to be passed in?
Whatever default value we provide here, whether "close" or "close menu" might be nice to have the option to override it. That would require an iconProps
param passed in where we could then get iconProps['aria-label']
, and if present, use that instead of the default.
I'm fine with that being an enhancement later on too though.
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.
@shkeating thanks for the review and the advice! I've updated the aria-label
to "Close Navigation Menu"
to make it more unique and descriptive.
@brandonlenz Allowing the label to be passed in as a prop sounds like the ideal path to me! I think this would be especially useful if we want to show localized aria labels.
I think there's a couple of other components that could use that enhancement as well, so I'll keep the diff on this PR as lean as possible for now and open a new PR targeted at using those props as overrides. Let me know if that works!
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 works for me.
58548ec
to
cf2344f
Compare
This reverts commit 889b985.
@brandonlenz apologies for the churn on this, looks like we can't add an |
@seevee no worries! Thanks for seeing it through |
I think we'll need @shkeating to give it another approval anyways since she requested changes |
all set! great work all! |
Summary
Related Issues or PRs
Closes #2639
How To Test
I tested with
yarn link
to an application using playwright, AXE, and HTML_CodeSniffer. I am yet not familiar enough with these packages to set up an example repo, but I can confirm that these changes address our E2E testing issues.