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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8d7d50
fix(guidelines): guidelines-icons-button-label-audit
andreancardona Feb 17, 2021
3623007
Merge branch 'master' into audit-avt1-guidelines-icons-button-label
andreancardona Feb 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/react/src/components/TooltipDefinition/TooltipDefinition.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

onFocus={composeEventHandlers([onFocus, handleFocus])}>
{children}
</button>
Expand Down
3 changes: 3 additions & 0 deletions packages/react/src/components/TooltipDefinition/__snapshots__/TooltipDefinition-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ exports[`TooltipDefinition should allow the user to specify the direction 1`] =
>
<button
aria-describedby="definition-tooltip-3"
aria-label="tooltip text"
className="bx--tooltip__trigger bx--tooltip--a11y bx--tooltip__trigger--definition bx--tooltip--top bx--tooltip--align-start"
onFocus={[Function]}
type="button"
Expand Down Expand Up @@ -45,6 +46,7 @@ exports[`TooltipDefinition should render 1`] = `
>
<button
aria-describedby="definition-tooltip-1"
aria-label="tooltip text"
className="bx--tooltip__trigger bx--tooltip--a11y bx--tooltip__trigger--definition bx--tooltip--bottom bx--tooltip--align-start"
onFocus={[Function]}
type="button"
Expand Down Expand Up @@ -77,6 +79,7 @@ exports[`TooltipDefinition should support a custom trigger element class 1`] = `
>
<button
aria-describedby="definition-tooltip-2"
aria-label="tooltip text"
className="bx--tooltip__trigger bx--tooltip--a11y bx--tooltip__trigger--definition custom-trigger-class bx--tooltip--bottom bx--tooltip--align-start"
onFocus={[Function]}
type="button"
Expand Down