Skip to content
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(guidelines): guidelines-icons-button-label-audit #7821

Closed
wants to merge 2 commits into from
Closed

fix(guidelines): guidelines-icons-button-label-audit #7821

wants to merge 2 commits into from

Conversation

andreancardona
Copy link
Contributor

Closes (Carbon Website): carbon-design-system/carbon-website#1498

  • Adds an aria-label in the Tooltip to fix a11y violation

  • Adds: ariaLabel: {tooltipText} to TooltipDefinition component

@netlify
Copy link

netlify bot commented Feb 17, 2021

Deploy preview for carbon-elements ready!

Built with commit 3623007

https://deploy-preview-7821--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Feb 17, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 3623007

https://deploy-preview-7821--carbon-components-react.netlify.app

@@ -77,6 +77,7 @@ const TooltipDefinition = ({
type="button"
className={tooltipTriggerClasses}
aria-describedby={tooltipId}
aria-label={tooltipText}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this end up causes the same text (tooltipText) to be announced twice from both aria-label and aria-describedby

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshblack good point! aria-describedby announces the tooltipId which I think should be different from the text

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andreancardona I believe the way aria-describedby works is that it uses the id to find the DOM node from which it computes the description. In this case, using aria-label with the tooltipText would give the node the accessible name of tooltipText. It would also give the node the description of tooltipText from aria-describedby as it points to a node that contains that as its accessible label.

In the preview, we can see this in the accessibility tab:

image

Another issue that this can cause is that now the button no longer uses the underlying text as the label, so the user would only hear the tooltip text but would not know what that text it is defining.

@tw15egan
Copy link
Collaborator

tw15egan commented Feb 17, 2021

I just did some further research into this issue, and it seems like the root of the issue is that we are trying to use TooltipDefinition as an icon-only button. But changing these to Button and not TooltipDefinition in ActionBar.js in the website repo, I was able to get rid of the errors

<Button
        kind="ghost"
        hasIconOnly
        onFocus={() => setIsActionBarVisible(true)}
        onClick={handleDownload}
        tooltipAlignment="center"
        tooltipPosition="top"
        iconDescription="Download SVG"
        className={styles.tooltip}
        triggerClassName={styles.trigger}
        renderIcon={Download16}
      />

Screen Shot 2021-02-17 at 12 29 31 PM

@andreancardona I'll throw up a PR so you can take a look and verify if this will close out the linked issue 👍🏻

PR: carbon-design-system/carbon-website#2144

@andreancardona
Copy link
Contributor Author

Closing this issue with @tw15egan fix :) carbon-design-system/carbon-website#2144

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants