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

Tooltip: refactor using ariakit #48222

Closed
25 tasks done
brookewp opened this issue Feb 18, 2023 · 4 comments · Fixed by #48440
Closed
25 tasks done

Tooltip: refactor using ariakit #48222

brookewp opened this issue Feb 18, 2023 · 4 comments · Fixed by #48440
Assignees
Labels
[Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.

Comments

@brookewp
Copy link
Contributor

brookewp commented Feb 18, 2023

What problem does this address?

The decision was made to refactor the 'legacy' Tooltip component due to the complexity of the existing component, the time it would take to refactor and migrate to typescript, and to utilize an accessible library that is actively maintained.

This issue branches off of what @t-hamano started in #42753, and aims to track the completion of the second step outlined by @ciampo:

Work on the new implementation using ariakit under the hood:

  • MVP is to create a component with the same APIs as the "legacy" one, which in theory doesn't introduce any regressions
  • Component to be written following the latest contributing guidelines (typescript, emotion, ...)
  • This would be a good time to assess the component's design
  • Optionally, as we work on the new component, understand if we want to introduce any new feature / expand the APIs

What is your proposed solution?

The following is a WIP, but my general tl;dr plan so far is:

First phase (MVP):

Research

  • review the 'legacy' tooltip component to see which features it supports
  • look at current bugs for the legacy tooltip
  • research how new features could be implemented and how current bugs could be solved using ariakit

Create

  • create a rough tooltip component to start exploring API
  • add legacy features to the new component
    • children
    • delay (web only)
    • position (to be deprecated)
    • placement
    • text
    • shortcut (web only)

Testing

  • look at legacy tests and add any missing tests as needed
  • add those tests to the new tooltip to ensure they meet the requirements of the legacy component
  • add additional tests for:

Documentation

  • add README.md
  • add and refine JS Docs and types
  • add storybook examples

Implementation

  • remove existing unused/unfinished experimental tooltip
  • replace existing legacy tooltip in GB usages

Second phase (TBD):

  • review the 'experimental' tooltip component to see which features it supports
  • research how these existing features could be implemented using ariakit
  • see if there are any new features we want to expose
  • add any experimental features and new features worth having
  • look at implementing a facade for the legacy version
  • request design input and implement styles
@afercia
Copy link
Contributor

afercia commented Mar 24, 2023

Looked also at the pending PR, seems to me an important point has been missed and there's a bit of confusion on what an accessible name and an accessible description are.

In both ARIA 1.1 and ARIA 1.2, the role=tooltip is meant to provide an accessible description. It must not be used to provide the accessible name.

A contextual popup that displays a description for an element.
Authors SHOULD ensure that elements with the role tooltip are referenced through the use of aria-describedby ...

Focus on the part 'referenced through the use of aria-describedby'.

From what I see, Ariakit uses aria-labelledby instead, thus providing the accessible name by referencing the element with role=tooltip. That's not valid. It's incorrect and can lead to potential accessibility issues when the visible text of a control and its actual accessible name mismatch.

Worth noting there's no official example of the ARIA tooltip design pattern yet. Work to develop a tooltip example is tracked by issue 127. It's still in the works. The ARIA Authoring Practices Task Force are still discussing what a role=tooltip even is and what it should do.

I'd strongly suggest to have a read at the relevant issues on their GitHub repo before continuing this implementation with Ariakit.

Clarify the use of role=tooltip
w3c/aria#979

Tooltip role should allow referencing by aria-labelledby
w3c/aria#987

To my understanding, the current state of the tooltip pattern example is here, where it's clear it must not be used to provide the accessible name.

Tooltip code example for ARIA Authoring Practices Guide (APG)
https://github.com/ZoeBijl/apg-tooltip
Demo:
https://zoebijl.github.io/apg-tooltip/

It's perfectly fine revealing the accessible name on hover/focus by the means of a 'visual tooltip' as long as it doesn't use a role=tooltip. That's what the current Tooltip component does (in most of the cases).

@ciampo
Copy link
Contributor

ciampo commented Mar 24, 2023

Hey @afercia , thank you for the early feedback!

We definitely plan to have a conservative approach with introducing the new Tooltip and replacing the legacy one, especially because we want to make sure that we don't cause any disruption to downstream developers and end users (and getting the accessibility part right is fundamental).

Regarding the aria-describedby issue that you raise, Brooke and I discussed about it just yesterday — we plan on using the described prop to change the behaviour of the component and use aria-describedby instead of aria-labelledby.

You can test the new behaviour by adding the described={true} prop to TooltipAnchor in the interactive example.

Would that change be enough, in your opinion?

@brookewp
Copy link
Contributor Author

Hi @afercia, thank you for taking a look! I initially added aria-describedby after reviewing ARIA's guidelines for tooltip but then swapped it for thedescribed prop which is in my most recent commit. After seeing your comment, I added a note to the PR to give context on this prop but we could also add a more explicit inline comment so it is more clear.

After my discussion with @ciampo, I opened a related issue in the ariakit repo to ask the authors on their decision to not have aria-describedby as the default option: ariakit/ariakit#2228

@diegohaz
Copy link
Member

The points mentioned by @afercia were fixed in the latest version of Ariakit, and a section about using Tooltip as a label was added to https://ariakit.org/components/tooltip#tooltips-are-descriptions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
Status: Done 🎉
Development

Successfully merging a pull request may close this issue.

5 participants