fix(#2101): TooltipTrigger: Tooltip should remain hidden in the DOM to provide an IDREF for aria-describedby on trigger element #2102

majornista · 2021-07-13T20:26:39Z

🐛 Bug Report

TooltipTrigger: Tooltip should remain hidden in the DOM to provide an IDREF for aria-describedby on trigger element.

🤔 Expected Behavior

In order for the Tooltip information to be available to assistive technology when navigating elements using JAWS's virtual cursor or NVDA's browse mode, the Tooltip that displays as an overlay in a portal when the trigger is either hovered or focused should remain hidden in the DOM to provide an IDREF for aria-describedby on trigger element. The triggerProps returned by useTooltipTrigger should always include aria-describedby referencing the tooltip by id, regardless of whether the tooltip is visible in the portal or hidden in the DOM.

😯 Current Behavior

Currently, a tooltip is only rendered to the DOM when the TooltipTrigger state.isOpen === true. The tooltip and the aria-describedby attribute on its trigger get removed when state.isOpen === false. This means that the text of the tooltip is only announced when the user focuses or hovers the trigger, and is not available to screen reader users navigating using virtual cursor/browse mode.

💁 Possible Solution

Keep the Tooltip in the DOM with a hidden attribute when the tooltip is not shown.

🔦 Context

This is important for improving screen reader accessibility.

💻 Code Sample

See https://codepen.io/Moiety/pen/LaPvWy for a positive example in development for WAI-ARIA 1.2 Authoring Practices: https://w3c.github.io/aria-practices/#tooltip

🌍 Your Environment

Software	Version(s)
react-spectrum	@react-spectrum/tooltip@3.1.3, @react-aria/tooltip@3.1.2
Browser	All
Operating System	All

Closes #2101

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue #2101.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Open https://reactspectrum.blob.core.windows.net/reactspectrum/674b18a8de35c388d5f289a7be8a1b4b77d3561f/storybook/index.html?path=/story/tooltiptrigger--default
Verify TooltipTrigger examples still work as expected on hover and focus.
Using a Windows screen reader, like JAWS or NVDA, navigate to the TooltipTrigger button in browse/virtual cursor mode. Neither JAWS nor NVDA will announce the Tooltip text by default, however:
- with JAWS, INSERT+TAB, or CAPS LOCK+TAB on laptops, announces the current object in virtual cursor mode, including the Tooltip,
- and similarly with NVDA, INSERT+SHIFT+O, or CAPS LOCK+SHIFT+O on laptops, announces the current object in virtual cursor mode, including the Tooltip.
Open https://reactspectrum.blob.core.windows.net/reactspectrum/674b18a8de35c388d5f289a7be8a1b4b77d3561f/storybook/index.html?path=/story/actiongroup--with-tooltips and verify that keyboard navigation of items in the ActionGroup still works.

🧢 Your Project:

Adobe/Accessibility

…o provide an IDREF for aria-describedby on trigger element TooltipTrigger: Tooltip should remain hidden in the DOM to provide an IDREF for aria-describedby on trigger element. In order for the Tooltip to announce consistently when navigating elements using JAWS's virtual cursor or NVDA's browse mode, the Tooltip that displays as an overlay in a portal when the trigger is either hovered or focused should remain hidden in the DOM to provide an IDREF for `aria-describedby` on trigger element. The `triggerProps` returned by `useTooltipTrigger` should always include `aria-describedby` referencing the tooltip by `id`, regardless of whether the tooltip is visible in the portal or hidden in the DOM. Currently, a tooltip is only rendered to the DOM when the TooltipTrigger `state.isOpen === true`. The tooltip and the `aria-describedby` attribute on its trigger get removed when `state.isOpen === false`. This means that the text of the tooltip is only announced when the user focuses or hovers the trigger, and is not available to screen reader users navigating using virtual cursor/browse mode. Keep the Tooltip in the DOM with a `hidden` attribute when the tooltip is not shown. This is important for improving screen reader accessibility. See https://codepen.io/Moiety/pen/LaPvWy for a positive example in development for WAI-ARIA 1.2 Authoring Practices: https://w3c.github.io/aria-practices/#tooltip | Software | Version(s) | | ---------------- | ---------- | | react-spectrum | @react-spectrum/tooltip@3.1.3, @react-aria/tooltip@3.1.2 | Browser | All | Operating System | All Adobe/Accessibility

adobe-bot · 2021-07-13T20:44:41Z

Build successful! 🎉

snowystinger

in general, I like the way this is done, and I still can't think of the reason why we didn't want it rendered when it was hidden

snowystinger · 2021-07-13T21:58:30Z

packages/@react-aria/tooltip/docs/useTooltipTrigger.mdx

-          And the tooltip tells you more information.
-        </Tooltip>
-      )}
+      <Tooltip hidden={!state.isOpen} state={state} {...tooltipProps}>


It looks non-breaking, but that there is work for anyone who copied our example :(
I don't know how we want to call this out in our release

I think the difference for anyone who copied from the prior example would be that the aria-describedby prop on the trigger referencing the Tooltip element will persist even when the Tooltip is not in the DOM. This minor failure would be imperceivable to assistive technology users, but might trigger warnings with automated accessibility testing.

btw, hidden isn't actually needed, it's in state if it's needed inside the component

In my tests with NVDA and JAWS, the Tooltip is not immediately announced when using the virtual cursor, so perhaps we decided that persisting the Tooltip with aria-describedby was a moot point.

yep! that was it!

With JAWS, INSERT+TAB or CAPS LOCK+TAB (on laptop) announces the button and tooltip for the button focused with the virtual cursor.

With NVDA INSERT+numpad5 or CAPS LOCK+SHIFT+O (on laptop) announces the button and tooltip for the button focused in browse mode. [hidden] attribute makes no difference.

So, in both cases, there is a way to access the tooltip info for an object that would not be available if the Tooltip and aria-describedby attribute were not persistent in the DOM.

btw, hidden isn't actually needed, it's in state if it's needed inside the component

[hidden] is necessary, because the tooltipProps returned by useTooltip do not return anything related to the isOpen state:

react-spectrum/packages/@react-aria/tooltip/src/useTooltip.ts

Lines 37 to 41 in 622e33d

return {

tooltipProps: mergeProps(domProps, hoverProps, {

role: 'tooltip'

})

};

A Tooltip is visible by default, and needs to be so that we can do animations on hide and show.

I could put the isOpen logic in the Tooltip component for this example, but I think this may be easier to understand and is more consistent with how Tooltips work when using an Overlay.

What I mean is that it should be in the aria hook and should be returned from useTooltip, or am I missing some reason why it can't be?

An alternative example adding a CSS animation to set opacity and visibility based on isOpen state:

import {useTooltipTriggerState} from '@react-stately/tooltip'; import {mergeProps} from '@react-aria/utils'; function Tooltip({state, ...props}) { let {tooltipProps} = useTooltip(props, state); let {isOpen} = state; return ( <span style={{ position: 'absolute', left: '5px', top: '100%', marginTop: '10px', backgroundColor: 'white', color: 'black', padding: '5px', /* fade in or fade out tooltip based on isOpen state */ opacity: (isOpen ? 1 : 0), visibility: (isOpen ? 'visible' : 'hidden'), transition: ` opacity 130ms ease-in-out 0ms, visibility 0ms linear ${isOpen ? 0 : 130}ms ` }} {...mergeProps(props, tooltipProps)} > {props.children} </span> ); } function Example(props) { let state = useTooltipTriggerState(props); let ref = React.useRef(); // Get props for the trigger and its tooltip let {triggerProps, tooltipProps} = useTooltipTrigger(props, state, ref); return ( <span style={{position: 'relative'}}> <button ref={ref} {...triggerProps}>I have a tooltip</button> <Tooltip state={state} {...tooltipProps}> And the tooltip tells you more information. </Tooltip> </span> ); } <Example /> <Example />

Ok, I see, we technically have two tooltips... https://github.com/adobe/react-spectrum/pull/2102/files#diff-225c01ce8d5eeafee3ecca35c4f8cded18543869468adbfb9b956914bde5280bR74 and they are in different places
whereas this mdx file only has one

could we wrap the new hidden tooltip into a new component (HiddenTooltip) that calls useTooltip which would return hidden when state is closed? I don't think it'll matter if the hidden element has role=tooltip? otherwise we can remove the role when hidden. This way we don't have to move the overlay. It should also fix this example so the user only has to remove the {isOpen &&

the aria hooks should take care of everything for the user

Ok I added a note to the description before the example about why hidden is used here.

majornista · 2021-07-14T16:28:46Z

in general, I like the way this is done, and I still can't think of the reason why we didn't want it rendered when it was hidden

In my tests with NVDA and JAWS, the Tooltip is not immediately announced when using the virtual cursor, so perhaps we decided that persisting the Tooltip with aria-describedby was a moot point.

snowystinger · 2021-07-14T16:43:27Z

In my tests with NVDA and JAWS, the Tooltip is not immediately announced when using the virtual cursor, so perhaps we decided that persisting the Tooltip with aria-describedby was a moot point.

yep! that was it!

dannify · 2021-07-21T23:09:52Z

I am a little confused by this. The ticket says that rendering tooltips in the dom will help improve assistive technology but this last comment

In my tests with NVDA and JAWS, the Tooltip is not immediately announced when using the virtual cursor, so perhaps we decided that persisting the Tooltip with aria-describedby was a moot point.

makes it sound like it is not really necessary?

If we can avoid having to add all tooltips to the DOM, I would prefer that. It has the potential of adding a lot of bloat if there are many tooltips on a page (maybe in a table) and would also require react-aria users to have to follow the same process themselves.

adobe-bot · 2021-08-09T17:07:46Z

Build successful! 🎉

majornista · 2021-08-09T21:34:30Z

I am a little confused by this. The ticket says that rendering tooltips in the dom will help improve assistive technology but this last comment

In my tests with NVDA and JAWS, the Tooltip is not immediately announced when using the virtual cursor, so perhaps we decided that persisting the Tooltip with aria-describedby was a moot point.

makes it sound like it is not really necessary?

If we can avoid having to add all tooltips to the DOM, I would prefer that. It has the potential of adding a lot of bloat if there are many tooltips on a page (maybe in a table) and would also require react-aria users to have to follow the same process themselves.

I think it's best to have the Tooltips in the DOM.

With JAWS, INSERT+TAB, or CAPS LOCK+TAB on laptops, announces the current object in virtual cursor mode, including the Tooltip,

and similarly with NVDA, INSERT+SHIFT+O, or CAPS LOCK+SHIFT+O on laptops, announces the current object in virtual cursor mode, including the Tooltip.

When the Tooltip is omitted from the DOM, the screen reader has no way to access the Tooltip information in virtual cursor/browse mode.

…o Issue-2101-tooltip

adobe-bot · 2021-09-08T20:15:25Z

Build successful! 🎉

…he DOM to provide an IDREF for aria-describedby on trigger element (#2102)" This reverts commit a3d0e8c.

…he DOM to provide an IDREF for aria-describedby on trigger element (#2102)" (#2312) This reverts commit a3d0e8c.

majornista requested review from dannify, devongovett and snowystinger as code owners July 13, 2021 20:26

majornista force-pushed the Issue-2101-tooltip branch from b29d487 to 674b18a Compare July 13, 2021 20:35

majornista requested a review from jnurthen July 13, 2021 20:55

snowystinger reviewed Jul 13, 2021

View reviewed changes

Merge branch 'main' into Issue-2101-tooltip

023d0ab

devongovett added 2 commits September 8, 2021 12:55

Merge branch 'main' of github.com:adobe-private/react-spectrum-v3 int…

c64b5fd

…o Issue-2101-tooltip

Add note to the docs about use of hidden prop

cf04880

devongovett approved these changes Sep 8, 2021

View reviewed changes

devongovett merged commit a3d0e8c into main Sep 8, 2021

devongovett deleted the Issue-2101-tooltip branch September 8, 2021 20:55

snowystinger added a commit that referenced this pull request Sep 9, 2021

Revert "fix(#2101): TooltipTrigger: Tooltip should remain hidden in t…

086d740

…he DOM to provide an IDREF for aria-describedby on trigger element (#2102)" This reverts commit a3d0e8c.

snowystinger mentioned this pull request Sep 9, 2021

Revert "fix(#2101): TooltipTrigger: Tooltip should remain hidden in the DOM to provide an IDREF for aria-describedby on trigger element" #2312

Merged

snowystinger added a commit that referenced this pull request Sep 10, 2021

Revert "fix(#2101): TooltipTrigger: Tooltip should remain hidden in t…

e867743

…he DOM to provide an IDREF for aria-describedby on trigger element (#2102)" (#2312) This reverts commit a3d0e8c.

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#2101): TooltipTrigger: Tooltip should remain hidden in the DOM to provide an IDREF for aria-describedby on trigger element #2102

fix(#2101): TooltipTrigger: Tooltip should remain hidden in the DOM to provide an IDREF for aria-describedby on trigger element #2102

majornista commented Jul 13, 2021 •

edited

Loading

adobe-bot commented Jul 13, 2021

snowystinger left a comment

snowystinger Jul 13, 2021

majornista Jul 14, 2021

snowystinger Jul 14, 2021

majornista Jul 14, 2021 •

edited

Loading

majornista Jul 14, 2021 •

edited

Loading

snowystinger Jul 14, 2021 •

edited

Loading

majornista Jul 14, 2021

snowystinger Jul 15, 2021

devongovett Sep 8, 2021

majornista commented Jul 14, 2021

snowystinger commented Jul 14, 2021

dannify commented Jul 21, 2021

adobe-bot commented Aug 9, 2021

majornista commented Aug 9, 2021

adobe-bot commented Sep 8, 2021

	return {
	tooltipProps: mergeProps(domProps, hoverProps, {
	role: 'tooltip'
	})
	};

fix(#2101): TooltipTrigger: Tooltip should remain hidden in the DOM to provide an IDREF for aria-describedby on trigger element #2102

fix(#2101): TooltipTrigger: Tooltip should remain hidden in the DOM to provide an IDREF for aria-describedby on trigger element #2102

Conversation

majornista commented Jul 13, 2021 • edited Loading

🐛 Bug Report

🤔 Expected Behavior

😯 Current Behavior

💁 Possible Solution

🔦 Context

💻 Code Sample

🌍 Your Environment

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

adobe-bot commented Jul 13, 2021

snowystinger left a comment

Choose a reason for hiding this comment

snowystinger Jul 13, 2021

Choose a reason for hiding this comment

majornista Jul 14, 2021

Choose a reason for hiding this comment

snowystinger Jul 14, 2021

Choose a reason for hiding this comment

majornista Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

majornista Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

snowystinger Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

majornista Jul 14, 2021

Choose a reason for hiding this comment

snowystinger Jul 15, 2021

Choose a reason for hiding this comment

devongovett Sep 8, 2021

Choose a reason for hiding this comment

majornista commented Jul 14, 2021

snowystinger commented Jul 14, 2021

dannify commented Jul 21, 2021

adobe-bot commented Aug 9, 2021

majornista commented Aug 9, 2021

adobe-bot commented Sep 8, 2021

majornista commented Jul 13, 2021 •

edited

Loading

majornista Jul 14, 2021 •

edited

Loading

majornista Jul 14, 2021 •

edited

Loading

snowystinger Jul 14, 2021 •

edited

Loading