fix(a11y): move aria-label from svg icon to button

[seevee]

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.

[brandonlenz]

Thanks for the PR!

This is conflicting with the last change to this component in #2500

@sawyerh do you have thoughts on this change since you authored the last iteration?

[sawyerh]

@brandonlenz Hey stranger 😄 No issues from my side. I think my original solution was based on the USWDS example code which uses an alt on their img, but this solution seems like it should be fine too.

[seevee]

Some additional context - these changes will bring the NavCloseButton default behavior closer to parity with ModalCloseButton:

react-uswds/src/components/Modal/ModalCloseButton/ModalCloseButton.tsx

Lines 14 to 24 in 59dffd1

	return (
	<Button
	aria-label="Close this window"
	{...buttonProps}
	className="usa-modal__close"
	onClick={handleClose}
	data-close-modal
	type="button">
	<Icon.Close aria-hidden="true" />
	</Button>
	)

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:

WCAG2AA.Principle4.Guideline4_1.4_1_2.H91.Button.Name

This button element does not have a name available to an accessibility API. Valid names are: title undefined, element content, aria-label undefined, aria-labelledby undefined.

EDIT: would it be acceptable to add an alt="Close" to the icon properties alongside the button's aria-label="Close"? That seems like it should let us satisfy both the svg-img-alt rule from https://accessibilityinsights.com as well as the Button.Name rule from our AXE/Playwright setup.

[brandonlenz]

EDIT: would it be acceptable to add an alt="Close" to the icon properties alongside the button's aria-label="Close"? That seems like it should let us satisfy both the svg-img-alt rule from https://accessibilityinsights.com as well as the Button.Name rule from our AXE/Playwright setup.

@seevee I think that sounds fine. @shkeating this one is pretty accessibility focused. Do you have any thoughts about the changes or suggestion?

[shkeating]

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.

[brandonlenz]

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.

[seevee]

@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!

[brandonlenz]

That works for me.

[seevee]

@brandonlenz apologies for the churn on this, looks like we can't add an alt attribute to an svg.

[brandonlenz]

@seevee no worries! Thanks for seeing it through

[brandonlenz]

I think we'll need @shkeating to give it another approval anyways since she requested changes

[shkeating]

all set! great work all!