fix(Tooltop): a11y improvements #2245
Conversation
…he div if the user provides property `triggerText`, then the button should use aria-labelledby to point to its id, if the user doesn't provide property `triggerText`, then they need to provide an aria-label via the `iconDescription` property.
|
Deploy preview for carbon-components-react ready! Built with commit de2fc39 https://deploy-preview-2245--carbon-components-react.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit f359e1f https://deploy-preview-2245--carbon-components-react.netlify.com |
paschalidi
force-pushed
the
cp/tooltip-a11y
branch
from
6ca00da to
bd28340
Compare
|
@dakahn i can reproduce it. will take care of it as soon as possible. Thanks for taking care of this |
|
@dakahn hey thanks for reviewing! In my FF the bug is now fixed. Problem was that I created that Well hope that makes little bit of sense. |
src/components/Tooltip/Tooltip.js
Outdated
| * The title of the default tooltip icon, to be put in its SVG `<title>` element. | ||
| */ | ||
| iconTitle: PropTypes.string, | ||
| iconDescription: ({ triggerText, iconDescription }) => { |
There was a problem hiding this comment.
Seems close to the use case of: https://github.com/IBM/carbon-components-react/blob/v7.1.0/src/prop-types/isRequiredOneOf.js
src/components/Tooltip/Tooltip.js
Outdated
| {finalIcon} | ||
| <div className={`${prefix}--tooltip__trigger`} {...properties}> | ||
| {IconCustomElement ? ( | ||
| <IconCustomElement {...iconProperties} /> |
There was a problem hiding this comment.
Need to set triggerEl for <IconCustomElement> in addition to <Icon>.
src/components/Tooltip/Tooltip.js
Outdated
| onMouseOut: this.handleMouse, | ||
| onFocus: this.handleMouse, | ||
| onBlur: this.handleMouse, | ||
| ref: mergeRefs(ref, node => { |
There was a problem hiding this comment.
This ref should be set to <div className={triggerClasses}> only when its content is text-only. This is for locating tooltip relative to icon when it's present, and to the containing <div> otherwise.
There was a problem hiding this comment.
thanks for explaining, was not very clear to me .. will fix that
|
@asudoh maybe have another look |
|
@paschalidi Have you had a chance to address my comments...? Thanks! |
|
I did but for some reason never pushed those changes. very sorry for notifying for no good reason @asudoh. |
🌳 👋 hello to you all 🌻 Hope you enjoying the great day!
Continues on https://github.com/IBM/carbon-components-react/issues/2193
Tackles the following
IconTitle- its not needed for a11y.aria-owns- this attribute its not needed for a11y in thetooltipaccording to https://github.com/IBM/carbon-components-react/issues/2193aria-labelledbythis attribute its not needed for a11y in thetooltipaccording to https://github.com/IBM/carbon-components-react/issues/2193Iconis present then the div wrapping theIconhas all the attributes are are relevant to the action of showing theTooltip. When theIconcomponent is not present those attributes are now being parsed to the div that wraps the actual text. I call these attributespropertiesits not a very distinct name please feel free to suggest something more meaningful!triggerTextis not being parsed user gets error message that the propertyiconDescriptionis required in order to have better user experienceChangelog
Tooltip a11y changes.
##Future 👨🚀
The next PR would be regarding
from https://github.com/IBM/carbon-components-react/issues/2193