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

Update toggletip element roles #13475

Conversation

gcamacho079
Copy link
Contributor

Description

Updated roles so that toggletip uses button role instead of li. The button label has also been updated to use a disclosure message rather than the contents of the toggletip. Upon testing, live region messages have been nixed as they don’t communicate a status update and may cause the interface to be too chatty.

A couple of outstanding questions I’m hoping to get your opinion on @brandonkelly

  • The Tooltip.js uses a HUD, which isn’t ideal for the message container. In addition to having role="status", the message container will be empty and present in the accessibility tree on page load (not display: none when closed), and the message container will be emptied and then re-injected when the button is toggled open. In that case, should the message container be something other than a HUD?
  • Upon testing, the hover-to-open could have implications for screen magnifier users. Because a screen magnifier user will need to move their cursor to center the message on screen, the message will continue to close unless they click to keep it open; however, this requires two clicks, as the first click would close the toggletip. Should we have the first click toggle it open if the tip is initially opened by hover?

Related issues

@gcamacho079 gcamacho079 requested review from benjamindavid and a team as code owners July 24, 2023 15:52
@gcamacho079 gcamacho079 requested review from brandonkelly and removed request for benjamindavid July 24, 2023 15:52
…ement' into a11y/live-region-and-roles

# Conflicts:
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
#	src/web/assets/cp/dist/css/cp.css
#	src/web/assets/cp/dist/css/cp.css.map
…ement' into a11y/live-region-and-roles

# Conflicts:
#	src/web/assets/pluginstore/dist/css/app.css.map
#	src/web/assets/pluginstore/dist/js/app.js
#	src/web/assets/pluginstore/dist/js/app.js.map
@brandonkelly
Copy link
Member

The Tooltip.js uses a HUD, which isn’t ideal for the message container. In addition to having role="status", the message container will be empty and present in the accessibility tree on page load (not display: none when closed), and the message container will be emptied and then re-injected when the button is toggled open. In that case, should the message container be something other than a HUD?

I’m not seeing any of those behaviors on my end. It’s not getting a role=status, it is getting display:none when closed, and the contents are not getting refreshed when closed/reopened.

Should we have the first click toggle it open if the tip is initially opened by hover?

Agreed, done.

@gcamacho079
Copy link
Contributor Author

I’m not seeing any of those behaviors on my end. It’s not getting a role=status, it is getting display:none when closed, and the contents are not getting refreshed when closed/reopened.

Apologies for the confusion - I meant that the HUD might not be the ideal choice for the message container because it doesn't match that markup. I think the ideal implementation of this component uses the inclusive toggletip pattern.

If you scroll down to the "Toggletips with live regions" section, it outlines the markup/functionality in order to have the info announce after the button is toggled open.

@brandonkelly
Copy link
Member

Gotcha. Most of the HUD code is just for displaying/positioning it properly, so I think it makes sense to use that. We can tweak other behaviors to match the toggletip recommendations though. So to make sure we are on the same page, you are saying:

  • The container should have role="status"
  • The inner contents should be cleared out when the tooltip is closed
  • The tooltip shouldn’t get display: none when closed

Is that right? And if so, what should we do instead of display: none?

@gcamacho079
Copy link
Contributor Author

The container should have role="status"

Yes, if by container you mean the HUD/tooltip container. 👍🏼 All looks good with those bulletpoints.

And if so, what should we do instead of display: none?

The visually-hidden class could be used as an alternative. Or if those styles are problematic, something like setting opacity to zero, or positioning it outside the viewport could work as well.

…ement' into a11y/live-region-and-roles

# Conflicts:
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
#	src/web/assets/cp/dist/css/cp.css
#	src/web/assets/cp/dist/css/cp.css.map
@brandonkelly
Copy link
Member

Alright, made those changes. Let me know how it looks. Also there’s this question, ICYMI: #13475 (comment)

@gcamacho079
Copy link
Contributor Author

gcamacho079 commented Aug 3, 2023

Alright, made those changes. Let me know how it looks. Also there’s this question, ICYMI: #13475 (comment)

Looks and sounds great! I made a quick update to add a region role to the container.

I only noticed a couple things:

  • The tooltip should only automatically open on hover, not on focus.
  • If the toggle is focused and the activity center is removed from the DOM, the tooltip stays visible. Ideally it would also be destroyed/deleted.

@brandonkelly
Copy link
Member

Alright, those are addressed!

@brandonkelly brandonkelly merged commit 635d4f2 into feature/dev-1154-show-other-authors-editing-the-same-element Aug 4, 2023
@brandonkelly brandonkelly deleted the a11y/live-region-and-roles branch August 4, 2023 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants