ToolTip: refactor using ariakit #48440
Conversation
brookewp
added
[Type] Enhancement
brookewp
force-pushed
the
add/ariakit-tooltip
branch
from
6d2dd09
to
a1eb09a
Compare
brookewp
force-pushed
the
add/ariakit-tooltip
branch
from
c133d33
to
4c403e5
Compare
|
Size Change: +7.55 kB (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
| const button = screen.getByRole( 'button', { name: /Button/i } ); | ||
|
|
||
| expect( button ).toBeVisible(); | ||
| //expect( button ).toBeDisabled(); |
There was a problem hiding this comment.
In the legacy tooltip, the tooltip will show if an element is disabled. From what I've seen, it shouldn't show unless aria-disabled is added instead. In order to be backwards compatible, we may need to add a temporary solution to be deprecated 🤔
|
|
||
| return ( | ||
| <> | ||
| <TooltipAnchor described state={ tooltipState }> |
There was a problem hiding this comment.
After seeing this comment, I just wanted to add a note to clarify what the described prop is. In ariakit, aria-labelledby is the default but aria-describedby can be added by adding the described prop:
https://github.com/ariakit/ariakit/blob/5d8a1f047fcadcf117073c70359663a3946b73bf/packages/ariakit/src/tooltip/tooltip-anchor.ts#L122-L136
According to ARIA guidelines, aria-describedby is required for tooltip so I'm not sure why it isn't the default option. @ciampo pointed out it might be worth asking the author about this choice.
| shortcut, | ||
| text, | ||
| } = props; | ||
|
|
||
|
|
||
| const DEFAULT_PLACEMENT = 'bottom'; |
There was a problem hiding this comment.
Interestingly, the legacy tooltip has a default position of bottom middle in code, while the README states that the default is top center.
Personally, I think that it makes more sense for a tooltip's default position to be on top of its anchor, but it makes sense to keep bottom as the default placement, since it corresponds to the current legacy tooltip behaviour
|
|
||
| return ( | ||
| <> | ||
| <TooltipAnchor as={ Slot } described state={ tooltipState }> |
There was a problem hiding this comment.
We need to use a solution like Slot from Radix to merge props to the immediate child.
This solution was one of the options proposed by ariakit author: ariakit/ariakit#1245 (comment)
There was a problem hiding this comment.
That's not needed anymore. See https://ariakit.org/examples/dialog-radix for an example of how to implement an API similar to Radix UI.
This should be as easy as:
<TooltipAnchor render={children} />
|
Flaky tests detected in ef7d837. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5898187692
|
brookewp
force-pushed
the
add/ariakit-tooltip
branch
from
2531dfc
to
26442dd
Compare
brookewp
force-pushed
the
add/ariakit-tooltip
branch
from
b8e719e
to
9182f1e
Compare
|
Hey @tyxla! I requested a review from you because of some strange changes in the unit tests since I returned to this PR. The significant change was updating the branch to the updated I found that only one test failed on its own and I've added an inline comment here on what I found so far. For the remaining three failing tests, they all pass on their own. They also pass when the previous test is skipped. So it seems to be a weird side effect. I thought maybe it was an issue with async/await, mutation, or timers/mocks not being reset, but I didn't find any solutions from going down those paths. The only solution I found was if I moved the tests to before the first failing test. This is the test they needed to be moved before: And three tests were moved, beginning here: So technically, all unit tests pass now but it's a bit confusing. If you have any insights, I'd appreciate it! |
|
From what I've seen in my initial look, it appears that the first failing test expects that after tabbing, the tooltip will be hidden. However, in my testing with the Storybook instance, it appears that it doesn't. So the test is actually accurately failing, because we don't seem to properly be processing the blur event. So I'd start from investigating what changed between |
|
For the protocol, when testing the ariakit |
|
The problem seems a side-effect caused by #52620:
To confirm my assumption, I've added temporarily a A better fix would be to somehow differentiate the spinner from the tooltip wrapper without using test-id (@diegohaz @jsnajdr @tyxla do you have any ideas?) |
ciampo
force-pushed
the
add/ariakit-tooltip
branch
from
2610b74
to
5761d79
Compare
|
One easy fix would be to look for the spinner not globally ( Another idea is to give the spinner a more specific role. There is a StackOverflow question that offers many sensible alternatives -- |
There was a problem hiding this comment.
A better fix would be to somehow differentiate the spinner from the tooltip wrapper without using test-id (@diegohaz @jsnajdr @tyxla do you have any ideas?)
Alternatively to what @jsnajdr suggested, we could use a container.getElementsByClassName() query, with an ESLint ignore rule. This also isn't ideal, but at least it won't clutter the production component with an extra attribute that's only useful for testing. I personally prefer cluttering the test rather than the production code. Plus, with the test ID, we're already at our last resort, so it should be fine that we're aiming to test for an implementation detail.
I'm personally fine with the test ID too, but have a slight preference for the getElementsByClassName in this particular case, to keep the production code clean from unnecessary attributes.
In any case, it's worth trying out the role="progressbar" if we can. Happy to see it in a follow-up PR!
|
To recap, "final final" list of follow-ups:
|
|
Looks like this PR had a somewhat important (not in a good sense) impact on some performance metrics (block selection, inserter opening). Would be cool if we could investigate the reasons and address them. |
Absolutely, I was about to reach out to you to ask for some help in how to investigate the regressions, since I've never had to do it before. Was the Performance CI check supposed to flag this before merging? |
|
Haven't dug deeply into this theory, but is it possible that the Ariakit version just deals differently with various timeouts (show timeout, skip timeout, hide timeout), and playing with those timeouts, like reducing them a bit, could have a big positive impact? For example, the default Ariakit timeout is |
|
I believe that the delay was It is weird that inserting a block got slower with a new tooltip, since tooltips shouldn't be involved in those user actions. As noticed previously, it seems that ariakit renders the tooltip in the DOM at all times, and only toggles its visibility when needed. Maybe this could be affecting metrics? |
The performance test run before merging but at the moment doesn't notify about the impact, you have to manually open the job and check the results.
That seems like a good hint to me. Especially if these components react to changes... |
|
Also pinging @diegohaz to see if he can help us to look into this |
|
Yeah, if the tooltip was dynamically rendered before, and it's always rendered now, this could have an impact. The Ariakit Tooltip can be dynamically rendered too: const tooltipStore = Ariakit.useTooltipStore( {
placement: positionToPlacement( position ),
timeout: delay,
} );
+ const isTooltipOpen = tooltipStore.useState( 'open' );
return (
<>
<Ariakit.TooltipAnchor
onBlur={ tooltipStore.hide }
onClick={ tooltipStore.hide }
store={ tooltipStore }
render={ isOnlyChild ? children : undefined }
>
{ isOnlyChild ? undefined : children }
</Ariakit.TooltipAnchor>
- { isOnlyChild && ( text || shortcut ) &&
+ { isOnlyChild && ( text || shortcut ) && isTooltipOpen && (
<Ariakit.Tooltip |
ciampo
added
the
has dev note
|
Added a dev note in the PR description |
What?
Closes #48222
Fixes #43129
The goal for this PR is to refactor our current tooltip using Ariakit's tooltip without changing the existing API.
Why?
After multiple issues arose, we assessed our current 'legacy' Tooltip and decided it would be better to replace it with a new version, using Ariakit. The legacy tooltip has a complex way of handling events and has not been migrated to Typescript. The changes in this PR uses Ariakit's Tooltip with modifications, to create a typed component with simplified implementation details and improved accessibility.
More details can be found in the tracking issue: #48222
How?
By using Ariakit's tooltip and modifications to match the API and behaviors of our legacy tooltip. In the testing instructions below, details on each of the changes are outlined with testing steps.
Testing Instructions
npm run test:unit packages/components/src/tooltip/test/index.tsxUnit tests should match legacy and test for the same behaviors (+ one new test for Esc feature which is mentioned later in the testing steps).
Six other tests failed after replacing tooltip. Two were resolved outside of this PR (#53703 and #53704). The remaining four had to be resolved within this PR, as the failures are tied to the changes. These started to fail mainly because tests were checking
not.toBeInTheDocument, but this version of tooltip remains in the DOM when it is hidden. This is in the case of these tests:npm run test:unit packages/components/src/button/test/index.tsxnpm run test:unit packages/components/src/tab-panel/test/index.tsxThe tests for
ToggleControlGroupcalled a helper function which references a wrapper's CSS class that no longer wraps tooltip. Also, there is a custom tooltip componentWithTooltipadded with its own types, which needed to be updated as well.npm run test:unit packages/components/src/toggle-group-control/test/index.tsxIn
PostSavedState, the test query wasn't specific enough, so multiple elements were being found when only one was expected:npm run test:unit packages/editor/src/components/post-saved-state/test/index.jsExisting behavior:
The following behaviors are default in both versions of tooltip:
Only accepts one child component
This is an expected existing behavior, but there is a bug when the only child is an Icon component. The new tooltip fixes this issue and prevent the workarounds from breaking upon replacement. An example of this workaround can be found in
packages/edit-site/src/components/sidebar-navigation-screen-patterns/index.jswhich can be tested in the editor:We can also test to make sure the workaround isn't needed by removing the
spanwrapper. With the legacy tooltip, the anchor won't render and there will be a warning that looks like this in the console:In the new version, it will still warn, but the lock icon will render and the tooltip will behave as expected.
If the tooltip overflows, the position should be flipped.
Workarounds
A few behaviors differ between the tooltip components. Therefore, modifications have been made to match the legacy.
Leaving the document hides the tooltip
The intended default behavior in ariakit (context on why here) is to keep the tooltip visible when focus leaves the anchor to the document, but not in our legacy. To amend this, an `onBlur` has been added to the anchor. This scenario is covered in our unit tests but it can be manually tested:
Clicking on the anchor should hide the tooltip
New features
The only 'new' feature is the tooltip can now be closed by using the 'Esc' key, which is in ARIA guidelines for Tooltip. It can be disabled, but I think it is okay to add it in this first iteration as long as it doesn't break any existing features that use the Escape key. For example, Modal works the same as before, with Escape closing the modal rather than tooltip. I've also added a test for this case, but if there are other cases where this would be a problem, please share!
Tooltip should close on Esc keydown
As mentioned, there is a unit test for modal, but this can be tested manually as well:
Testing Instructions for Keyboard
Most of the above steps include keyboard testing but these are the main things to look for when using a keyboard:
npm run storybook:devtabkey to focus the anchor element (Button) and see a tooltip appearEsckey to close the tooltip✍️ Dev note
The
Tooltipand theTabPanelcomponents have been completely rewritten to leverage third-party, headless libraries (specificallyariakit) as the first tasks of a new experimental effort within the package.Both migrations were intentionally designed to avoid changes to the API or developer experience, while also bringing several benefits:
Specifically to the
Tooltipcomponent, the refactor also fixed a long-standing issue and presented the opportunity to align with other components already using a newplacementprop in lieu of the legacy, now deprecatedpositionprop.For
TabPanel, the only noteworthy change is thattabpanelelements now get a tabstop. This means that when focused on atab, pressing the[Tab]key will apply focus to thetabpanelitself, rather than jumping directly to the next focusable element within thetabpanelelement.