a11y(Tooltip): "tooltip" role #6865
Conversation
Generate changelog in
|
There was a problem hiding this comment.
I think this is getting closer but we need to be very careful here of breaking existing behavior. We'll only be able to merge this if this can be done in a way that is a strict improvement, and doesn't negatively impact users who may have already done the work to work around this.
| // want Popover to warn if empty, so don't wrap in element if empty. | ||
| Utils.isReactNodeEmpty(content) | ||
| ? content | ||
| : Utils.ensureElement(content, undefined, { role: "tooltip", id: tooltipId }) |
There was a problem hiding this comment.
I don't think we can do this because a consumer may have already defined an id on the content which this would override
There was a problem hiding this comment.
Highly doubt that would ever be the case, can't imagine what other scenarios they'd be doing this for. There are many cases, such as in Popover, where we apply a bunch of props without caring what the consumer may have already applied.
There was a problem hiding this comment.
This should account for that case: be12eb3
There was a problem hiding this comment.
@evansjohnson the other alternative solution here would be we add a prop contentProps to Popover and apply it to the div that's already wrapping the content. Then we could apply these props via that. Up to you.
There was a problem hiding this comment.
what other scenarios they'd be doing this for
basically to fix this issue in consumer land is what I'm thinking about, or possibly adding an id used to target a tooltip in tests?
There was a problem hiding this comment.
Refactored, please re review, I think this concern has been solved
There was a problem hiding this comment.
Thank you for the iteration so far on this one! In interest of reducing risk of any behavior regressions and to make sure we don't regress in the future on the behavior added here, could we add tests for:
protect existing behavior:
- an
idprovided to tooltipcontentshould not be overridden - a provided
aria-describedbyon the tooltip target should not be overridden
protect against future regressions:
- for both
childrentarget andrenderTarget:- an
idwill be added to tooltipcontent, used inaria-describedbyadded to the tooltip target - an existing
idwill be used, with relevantaria-describedbystill added
- an
| // want Popover to warn if empty, so don't wrap in element if empty. | ||
| Utils.isReactNodeEmpty(content) | ||
| ? content | ||
| : Utils.ensureElement(content, undefined, { role: "tooltip", id: tooltipId }) |
There was a problem hiding this comment.
what other scenarios they'd be doing this for
basically to fix this issue in consumer land is what I'm thinking about, or possibly adding an id used to target a tooltip in tests?
| const childTarget = Utils.ensureElement(React.Children.toArray(children)[0]); | ||
| const clonedTarget = childTarget | ||
| ? React.cloneElement(childTarget, { | ||
| "aria-describedby": [tooltipId, childTarget.props["aria-describedby"]].filter(Boolean).join(" "), |
There was a problem hiding this comment.
tooltipId could possibly appear here twice in cases where consumers of Tooltip have already added these id/aria-describedby attributes
I'm not sure of the implications of that but it seems easy enough to guard against
There was a problem hiding this comment.
I don't think that would make a difference in a11y but here: 0336796
|
|
@evansjohnson refactored this one a little bit, which I think fixed some of your concerns. Ready for re-review! |
| const popoverClasses = classNames(Classes.TOOLTIP, Classes.intentClass(intent), popoverClassName, { | ||
| [Classes.COMPACT]: compact, | ||
| }); | ||
|
|
||
| const contentElement = Utils.ensureElement(content); | ||
| const tooltipId = contentElement?.props?.id ?? React.useMemo(() => Utils.uniqueId("tooltip"), []); |
There was a problem hiding this comment.
The use of React.useMemo here violates the rules of hooks since it is called conditionally within the ternary.
Can we extract this default memoized value out to the top level of this component?
| @@ -134,6 +177,14 @@ export class Tooltip< | |||
| }, | |||
| }} | |||
| {...restProps} | |||
| renderTarget={ | |||
| renderTarget && | |||
| React.useCallback(props => renderTarget({ ...props, tooltipId }), [renderTarget, tooltipId]) | |||
There was a problem hiding this comment.
Similar to above, can we pull this React.useCallback into a variable at the top of the component such that it is not called conditionally?
| contentElement && | ||
| React.cloneElement(contentElement, { | ||
| role: tooltipRole, | ||
| id: tooltipId, |
There was a problem hiding this comment.
nit: The linter is complaining that these keys, are not sorted alphabetically. We can fix by sorting them:
React.cloneElement(contentElement, {
id: tooltipId,
role: tooltipRole,
});
Fixes #0000
Checklist
Changes proposed in this pull request:
role="tooltip"to the Tooltip'scontent- https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/tooltip_roleidto the tooltip content, andaria-describedby={id}to the target