Tooltip: discuss simplifying the component

t-hamano commented 2 years ago

What problem does this address?

As mentioned in this comment, the purpose of this issue is to refactor the Toottip component and improve code readability.

The Tooltip component appears to add event handlers in order to emit events to its children — this behaviour seems to have been around a long time, but it also seems like it could be error-prone if it results in firing more events than it really should? Rather than just focusing on a single handler and element, do we need to revisit this behaviour across the eventHandlers in the component? This might not be at all related, but I noticed that the SelectControl component provides a noop function for some of its on methods, e.g. here — could the presence of noop props be confusing the Tooltip component into emitting events when it shouldn't? (This is probably a long shot, but just thought I'd mention it, because I find the logic of the Tooltip component a little challenging to follow) Yes. In particular, it looks like Tooltip is written in a very convoluted way, especially around event handlers. It would be good to understand if we can simplify its code!

What is your proposed solution?

I haven't yet decided on a specific direction, but I launched it as an issue to clearly manage it as a task.

cc @ciampo

ciampo commented 2 years ago

Since these changes may end up being a non-trivial refactor, I think it would be a good idea to do some preparatory work in order to give us more confidence for a future refactor.

The two main actions that come to mind are:

review existing unit tests, adding any missing scenarios and rewriting them to use testing-library
convert the component to TypeScript

What do you think?

t-hamano commented 2 years ago

It's a very good idea!

Does it mean that tests written in enzme should be refactored into testing-library? Many people seem to be converting to TypeScript and rewriting to testing-lobrary at the same time, but it may be too hard for me, so I'd like to split it up and deal with it 😅

ciampo commented 2 years ago

Does it mean that tests written in enzme should be refactored into testing-library?

Yes!

Many people seem to be converting to TypeScript and rewriting to testing-lobrary at the same time, but it may be too hard for me, so I'd like to split it up and deal with it 😅

That's totally ok! Feel free to work on the unit tests and the TypeScript refactor in two separate PRs, and to tag @mirka and myself for reviews :)

t-hamano commented 2 years ago

Realted issue: #35744

mirka commented 2 years ago

I just noticed we have a newer, Reakit-based tooltip component internally. It's not exported publicly, but it's already used once in production code (the Copy button in ColorPicker).

I don't know how feature complete the newer Tooltip component is, but it might be good to look into migrating to the newer one, if it's simpler and doesn't have the weird issues that the older one has.

ciampo commented 2 years ago

it might be good to look into migrating to the newer one, if it's simpler and doesn't have the weird issues that the older one has.

That's a great point! I will add that, instead of using the Tooltip based on reakit, we may want to create a new component using ariakit's latest APIs (example). I guess the next steps would be to assess what is currently supported by the "legacy" Tooltip component, and how we could support all of these features if we swapped the underlying implementation with reakit / ariakit

t-hamano commented 2 years ago

Thanks for the great ideas! Now then, I would like to confirm if it is possible to rewrite Tooltip internally based on reakit / ariakit in the legacy Tootip component while maintaining props.

ciampo commented 2 years ago

Alright, here's a deeper analysis:

An analysis of the currently available components

The current `Tooltip` component (let's call it "legacy") (demo)

The current Tooltip component exposes a few props:

children: the element to which the tooltip should anchor
text: the text in the tooltip
position: a string that is supposed to be in the format [top|bottom] [left|center|right] (which is passed straight to Popover, which actually is in the process of switching to a new placement prop)
delay (web only): waiting time before showing the tooltip, after the tooltip visibility is triggered (web only)
visible (native only): whether the tooltip should be displayed on initial render

With respect to the native version of the component, we could ignore it for the initial refactor, and as a follow-up, look into reaching feature parity, i.e. add support for the visible prop on the web counterpart (even though I would change the prop name) and for the delay prop on the web part.

The "experimental" `Tooltip` component (demo)

The experimental tooltip component is implemented via reakit, and it exposes quite a few props. A few notable differences with respect to the legacy Tooltip:

instead of text, it exposes a content prop which accepts any ReactElement (not just strings)
it exposes a gutter (which could be useful, although we may want to call it offset in order to be coherent with the Popover component)
it has an animated prop, which can be set to false to disable animations (plus an animationDuration prop)
it has a visible prop to determine if the tooltip should be visible on first render (it will still hide/show after the first user interaction) (this sounds like the same functionality implemented by the native version)
is has a focusable prop, to determine if the tooltip should be shown when the anchor is focused
instead of position, it has a placement prop (although the available values are different from both the legacy tooltip's position prop and the new popover's placement prop, as they are based on Popper)
it exposes a modal prop, which will render the popover in a Portal (instead of within the same dom node as the anchor) when set to true
is has a shortcut prop, I believe to trigger the component when that keyboard shortcut is pressed (although I didn't have much success trying it out)
it has a few unstable props (mainly for setting a delay and for fine-tuning the underlying popover's behavior)
and other props that are inherited from low-level reakit components

The upcoming `ariakit` `Tooltip` component (demo)

Using ariakit would be similar to how we use reakit in the experimental Tooltip component — the library exposes some lower-level building blocks (Tooltip, TooltipAnchor, TooltipArrow, useTooltipState) and we would be in change of styling them and creating a wrapper around those components — which would give the freedom to create the API surface that we want (which could minimize disruptions when migrating from the legacy component).

In terms of APIs, Tooltip's APIs are 99% based off ariakit's Popover component.

One notable difference is that ariakit switched from popper to floating-ui to implement its Popover, which is the same library used in our Popover — meaning that the we would have coherent values for the placement prop between Tooltip and Popover

Next steps

Personally, I think that the best way forward is to re-write Tooltip from scratch using ariakit:

the "experimental" Tooltip has become kind-of obsolete as reakit is about to be superseeded by ariakit
working on the current Tooltip (refactoring it to typescript, and then refactoring its internal implemetation) would probably take a much longer time

Things to take into account if we rewrite the component entirely are:

regressions may be harder to track (although that was very likely even if we refactored the original component)
we'd be switching to using a different Popover implementation instead of our internal one (although this would be also the case when the CustomSelectControl refactor lands)

Therefore, I propose this plan of action:

Bolster current unit tests (web):
- Rewrite using modern testing-library and user-event
- Assess if we should add any further tests
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 do folks think?

@t-hamano , would you feel like working on this? (these tasks could also be a good opportunity for @chad1008 !)

mirka commented 2 years ago

Sounds great, let's go with that. Thanks for breaking it down!

t-hamano commented 2 years ago

Thanks for the great suggestions! I'm not sure now if I can solve all the actions you suggested, but I will try these first two actions first:

Rewrite using modern testing-library and user-event

Assess if we should add any further tests

ciampo commented 2 years ago

Flagging here two related issues with the current Tooltip component:

t-hamano commented 2 years ago

Therefore, I propose this plan of action:

Bolster current unit tests (web):

Rewrite using modern testing-library and user-event

Assess if we should add any further tests

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

The first step was cleared by #43061 🚀 The next step is a bit of a hurdle for me, but I am willing to try 😅

ciampo commented 2 years ago

The second step will be also a partial discovery for us — happy to collaborate on it, if you're willing to do so!

Otherwise there's plenty of work still to be done in https://github.com/WordPress/gutenberg/issues/35744

ciampo commented 2 years ago

Related / may be fixed by the re-write:

brookewp commented 1 year ago

Thank you for completing the first step, @t-hamano! 🎉 I've started working on the second step so I'll close this issue and open a new tracking issue for part two!

t-hamano commented 1 year ago

Thank you for addressing this issue, @brookewp! Also, sorry for not working on the second step🙏 If there is anything I can do to help, I will.

brookewp commented 1 year ago

Also, sorry for not working on the second step🙏 If there is anything I can do to help, I will.

No need to be sorry - you've helped so much already! And thank you for the offer, I'll keep that in mind! 😄

WordPress / gutenberg