-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(tooltip): html updates for a11y #3148
feat(tooltip): html updates for a11y #3148
Conversation
Deploy preview for the-carbon-components ready! Built with commit 7da9731 https://deploy-preview-3148--the-carbon-components.netlify.com |
Deploy preview for carbon-components-react ready! Built with commit 7da9731 https://deploy-preview-3148--carbon-components-react.netlify.com |
Deploy preview for carbon-elements ready! Built with commit 7da9731 |
<span class="{{@root.prefix}}--tooltip__caret"></span> | ||
<p>This is some tooltip text. This box shows the maximum amount of text that should appear inside. If more room is | ||
<p id="unique-tooltip-body" >This is some tooltip text. This box shows the maximum amount of text that should appear inside. If more room is |
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.
Could you generate unique ID in .config.js
and refer to that here? Thanks!
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.
Hi @asudoh @elizabethsjudd could these be UUID? node-uuid could be used for this, generating unique keys for each demo (so they don't override each other.) Would require a new dependency, but node-uuid is the most popular/supported tool for 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.
@scottnath Any solutions generating unique ID is fine. e.g. This is our dev-time only I wouldn't foresee a collision with Math.random()
, etc.
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.
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.
Using node-uuid
is OK, but I wouldn't given there is an easier way. Also if you are strong for node-uuid
please use it with devDependencies
.
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.
Working as expected for me. Thanks for 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.
Could you share the AVT1 violations and how to re-create them for the interactive tooltips?
Definitely agreed that interactive tooltips should be replaced by dialog since they have interactive content, however it seems like this PR relies on some updates yet to be made to the JS to make things fully accessible/functional?
@joshblack The AVT 1 violation is that the tooltip didn't have a role so the content wasn't in a landmark. In order to recreate the violation:
You'll still get a color contrast violation with the link but that is design specific so I wasn't focused on it for this PR until @carbon-design-system/design has a chance to look at it. You are correct that it will require some JS updates to make it fully accessible which is what I plan on working next. I do need this question answered by @carbon-design-system/design before I work on it though. This PR resolves the HTML AVT 1 violation but also doesn't make the component less a11y than the current version as the current one is also inaccessible for screen reader users. |
@elizabethsjudd this is what I see when running it with the tooltip shown: And unfortunately none of the violations relate to the tooltip node, is there a step I'm missing here? To help with debugging, here are the rulesets I have enabled: |
@joshblack those are the correct violations you should be receiving when you run it on my updates. If you run DAP on the current version (not my branch) you will bet an additional violation for the tooltip node. |
Oh duh 😂 You're totally right, thanks @elizabethsjudd! Sorry for being dumb here haha |
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.
Excellent! It's all clear here on my end. Thanks for contributing. 🌟
cc: @snidersd any comments before merging? |
@jnm2377 I'm not seeing any change when testing React https://deploy-preview-3148--carbon-components-react.netlify.com. The contrast in the pop-up still fails and there is one DAP issue. |
@snidersd This issue ONLY applies to Carbon Vanilla. WH doesn't use the React versions of Carbon. There is still the color contrast on the link element but we need design input for that violation. My goal was to resolve the other violations/HTML structure issues with the tooltip. There will be additional JS updates needed to fully be accessible. |
@elizabethsjudd I understand that. I was sent a request to review this issue and just wanted to make sure that both Vanilla and React stay in sync since @dakahn is working on the React version. |
🎉 Thanks @elizabethsjudd for your contribution! |
This change adds several ARIA attributes along with `tooltipBodyId` prop, so element relationships are better described. This better aligns to the change made to vanilla earlier (carbon-design-system#3148 and Fixes carbon-design-system#3812.
This change adds several ARIA attributes along with `tooltipBodyId` prop, so element relationships are better described. This better aligns to the change made to vanilla earlier (carbon-design-system#3148 and Fixes carbon-design-system#3812.
Closes #3100
Simple HTML updates to pass AVT 1. The tooltip still has some bugs for keyboard and screen reader users (AVT 2 and AVT 3) that I can take a look at next but it's going to require some updates to the JavaScript as well as getting an answer to my open question
Changelog
New
role="dialog"
to define a tooltip with interactive elements (remove AVT 1 landmark violation)aria-controls
,aria-haspopup
to create the relationship between the trigger and tooltip elementsChanged
role="tooltip"
torole="button"
Testing / Reviewing