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(tooltip): add aria-labelledby #3011

Closed
wants to merge 13 commits into from
Closed
35 changes: 24 additions & 11 deletions packages/react/src/components/Tooltip/Tooltip.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,11 @@ class Tooltip extends Component {
iconName: PropTypes.string,

...isRequiredOneOf({
/**
* A general purpose aria-label to be read by screen readers
*/
ariaLabel: PropTypes.string,

/**
* The content to put into the trigger UI, except the (default) tooltip icon.
*/
Expand Down Expand Up @@ -306,6 +311,18 @@ class Tooltip extends Component {
}
};

getAccessibleLabel = (ariaLabel, triggerText, triggerId, iconDescription) => {
// if the user provides and aria-label go with that
if (ariaLabel) {
return { 'aria-label': ariaLabel };
// if the user provides triggerText -- set aria-labelledby to that element's ID
} else if (triggerText) {
return { 'aria-labelledby': triggerId };
}
// if the user provides neither of those set aria-label to icon-description
return { 'aria-label': iconDescription };
Copy link
Member

Choose a reason for hiding this comment

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

how are you getting iconDescription here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, getting back to this now. :D That's just a prop. Are you thinking it should be required if we don't have either ariaLabel or triggerText?

Copy link
Member

Choose a reason for hiding this comment

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

what I mean is it looks like it's undefined. you would need to get the value from this.props.iconDescription first

};

render() {
const {
triggerId = (this.triggerId =
Expand All @@ -321,6 +338,7 @@ class Tooltip extends Component {
children,
className,
triggerClassName,
ariaLabel,
direction,
triggerText,
showIcon,
Expand Down Expand Up @@ -363,17 +381,12 @@ class Tooltip extends Component {
onBlur: this.handleMouse,
'aria-haspopup': 'true',
'aria-expanded': open,
// if the user provides property `triggerText`,
// then the button should use aria-describedby to point to its id,
// if the user doesn't provide property `triggerText`,
// then an aria-label will be provided via the `iconDescription` property.
...(triggerText
? {
'aria-describedby': triggerId,
}
: {
'aria-label': iconDescription,
}),
...this.getAccessibleLabel(
ariaLabel,
triggerText,
triggerId,
iconDescription
),
};

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,8 @@ import cx from 'classnames';
import React from 'react';
import PropTypes from 'prop-types';
import { settings } from 'carbon-components';
import setupGetInstanceId from '../../tools/setupGetInstanceId';

const { prefix } = settings;
const getInstanceId = setupGetInstanceId();
const TooltipDefinition = ({
id,
className,
Expand All @@ -22,7 +20,6 @@ const TooltipDefinition = ({
tooltipText,
...rest
}) => {
const tooltipId = id || `definition-tooltip-${getInstanceId()}`;
const tooltipClassName = cx(`${prefix}--tooltip--definition`, className);
const tooltipTriggerClasses = cx(
`${prefix}--tooltip__trigger`,
Expand All @@ -36,7 +33,7 @@ const TooltipDefinition = ({
<div {...rest} className={tooltipClassName}>
<button
className={tooltipTriggerClasses}
aria-describedby={tooltipId}
aria-describedby={id}
aria-label={tooltipText}>
{children}
</button>
Expand All @@ -63,8 +60,7 @@ TooltipDefinition.propTypes = {
align: PropTypes.oneOf(['start', 'center', 'end']),

/**
* Optionally specify a custom id for the tooltip. If one is not provided, we
* generate a unique id for you.
* Optionally specify a custom id for the tooltip.
*/
id: PropTypes.string,

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ exports[`TooltipDefinition should allow the user to specify the direction 1`] =
className="bx--tooltip--definition custom-class"
>
<button
aria-describedby="definition-tooltip-2"
aria-label="tooltip text"
className="bx--tooltip__trigger bx--tooltip__trigger--definition bx--tooltip--top bx--tooltip--align-start"
>
Expand All @@ -32,7 +31,6 @@ exports[`TooltipDefinition should render 1`] = `
className="bx--tooltip--definition custom-class"
>
<button
aria-describedby="definition-tooltip-1"
aria-label="tooltip text"
className="bx--tooltip__trigger bx--tooltip__trigger--definition bx--tooltip--bottom bx--tooltip--align-start"
>
Expand Down