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 #48440

Merged
merged 57 commits into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
73f1884
Add ariakit dependency
brookewp Feb 23, 2023
488f4c9
Initial creation of component
brookewp Feb 25, 2023
f69c2c8
Add shortcut from legacy tooltip
brookewp Mar 17, 2023
ab1bc4e
Replace initial tests with legacy tooltip tests
brookewp Mar 17, 2023
60ee046
Add anchor styles for tooltip placement
brookewp Mar 17, 2023
16d2d12
Ensure tooltip renders only if one child is passed
brookewp Mar 17, 2023
1aebcf9
Replace query with tooltip role and add user actions to ensure tests …
brookewp Mar 22, 2023
0d20541
Remove forwarding refs to match legacy tooltip
brookewp Mar 24, 2023
6d0eb21
Add legacy positioning
brookewp Mar 24, 2023
cfb6c4a
Remove unnecessary code and add describedby id
brookewp Mar 24, 2023
32d654f
Replace cloneElement with Radix Slot
brookewp Mar 30, 2023
5386d21
Replace emotion with sass
brookewp Mar 30, 2023
53fd2cd
Update tests and types
brookewp Mar 30, 2023
adc88cb
Updated ariakit package and update-related props and types
brookewp Jul 8, 2023
b18b13a
Remove radix-ui/react-slot related package changes
brookewp Jul 11, 2023
9ffb2c9
Add arrow to fix floating-ui errors
brookewp Jul 21, 2023
3e1fcca
Replace zero value in temp fix to avoid NaN errors
brookewp Jul 25, 2023
324ba78
resolve test failures after package update
brookewp Jul 26, 2023
6f6d495
Match legacy styles
brookewp Jul 27, 2023
2eb848a
Hide tooltip onBlur to match legacy behavior
brookewp Jul 27, 2023
64062b4
Revert "resolve test failures after package update"
brookewp Jul 31, 2023
e7088d1
Prevent leaking by ensuring tooltip is hidden after each test
brookewp Aug 5, 2023
a5c8ff3
Remove ‘overlay’ option to match legacy
brookewp Aug 5, 2023
5c94f87
hide on anchor interact to match legacy
brookewp Aug 8, 2023
e40a31c
Temporarily replace placement with position for rollout
brookewp Aug 8, 2023
60e0ac3
Update story after storybook upgrade
brookewp Aug 10, 2023
ca2fb50
Update descriptions in readme and types
brookewp Aug 10, 2023
89ad595
Replace legacy tooltip with ariakit tooltip
brookewp Aug 12, 2023
a415651
Add test with modal for new Esc feature
brookewp Aug 11, 2023
2207553
Error for multiple children but continue to render anchor without too…
brookewp Aug 12, 2023
52fdc3d
Update snapshots
brookewp Aug 14, 2023
800f258
Update assertions in failing tests
brookewp Aug 15, 2023
875d9b5
Replace waitFor with findBy
brookewp Aug 16, 2023
fef59d6
Replace workaround with utility function
brookewp Aug 17, 2023
60ff328
Clean up variables
brookewp Aug 17, 2023
56c353d
Update package-lock
brookewp Aug 18, 2023
4bd9455
Add cleanup to test where toBePositioned was used previously
brookewp Aug 18, 2023
ef7d837
Update Changelog
brookewp Aug 18, 2023
814ad63
Merge branch 'trunk' into add/ariakit-tooltip
brookewp Aug 21, 2023
956bffd
Update event handling
brookewp Aug 21, 2023
beb9a29
Fix failing test
brookewp Aug 21, 2023
8854357
Add cleanup for other components testing tooltip
brookewp Aug 23, 2023
26059b6
Remove Arrow workaround after floating-ui/core update
brookewp Aug 24, 2023
53a1f1c
Merge branch 'trunk' into add/ariakit-tooltip
brookewp Aug 25, 2023
dc53c00
Refine test query to avoid conflict
brookewp Aug 25, 2023
bb98faf
Update tests based on feedback
brookewp Aug 31, 2023
50b7f5d
remove unnecessary type and variable
brookewp Aug 31, 2023
75e4ea5
Simplify by removing unnecessary logic and adding default value to po…
brookewp Sep 2, 2023
ed4c6aa
Add timeout to test for custom delay
brookewp Sep 2, 2023
cf59cd4
Ensure anchor shows when Icon or multiple children
brookewp Sep 2, 2023
6c54d5c
Update types and README
brookewp Sep 5, 2023
45ba198
Merge branch 'trunk' into add/ariakit-tooltip
brookewp Sep 5, 2023
59fec6c
Update snapshots
brookewp Sep 5, 2023
b43abd7
Remove type ignore after update in #54101
brookewp Sep 6, 2023
869bdde
Merge branch 'trunk' into add/ariakit-tooltip
brookewp Sep 7, 2023
968add1
Merge branch 'trunk' into add/ariakit-tooltip
ciampo Sep 7, 2023
5761d79
Fix link control tests by querying explicitly for the spinner
ciampo Sep 7, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions packages/base-styles/_z-index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,6 @@ $z-layers: (
// And block manager sticky disabled block count is higher still
".edit-post-block-manager__disabled-blocks-count": 2,

// Needs to be higher than any other element as this is an overlay to catch all events
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer have a custom event catcher

".components-tooltip .event-catcher": 100002,

// Needs to appear below other color circular picker related UI elements.
".components-circular-option-picker__option-wrapper::before": -1,

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ exports[`ColorPaletteControl matches the snapshot 1`] = `
class="components-circular-option-picker__option-wrapper"
>
<button
aria-describedby="tooltip-0"
aria-label="Color: red"
aria-pressed="true"
class="components-button components-circular-option-picker__option is-pressed"
Expand Down
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changelog needs to be updated, but I'll wait until the Popover changes are merged, and this PR is updated respectively.

Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

### Enhancements

- `Tooltip`: Replace the existing tooltip to simplify the implementation and improve accessibility while maintaining the same behaviors and API ([#48440](https://github.com/WordPress/gutenberg/pull/48440)).
- Make the `Popover.Slot` optional and render popovers at the bottom of the document's body by default. ([#53889](https://github.com/WordPress/gutenberg/pull/53889)).
- `ProgressBar`: Add transition to determinate indicator ([#53877](https://github.com/WordPress/gutenberg/pull/53877)).
- Prevent nested `SlotFillProvider` from rendering ([#53940](https://github.com/WordPress/gutenberg/pull/53940)).
Expand Down
15 changes: 11 additions & 4 deletions packages/components/src/button/test/index.tsx
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests were checking not.toBeInTheDocument, but this version of tooltip remains in the DOM when it is hidden

Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { plusCircle } from '@wordpress/icons';
*/
import Button from '..';
import Tooltip from '../../tooltip';
import cleanupTooltip from '../../tooltip/test/utils';

jest.mock( '../../icon', () => () => <div data-testid="test-icon" /> );

Expand Down Expand Up @@ -193,7 +194,7 @@ describe( 'Button', () => {

render( <Button icon={ plusCircle } label="WordPress" /> );

expect( screen.queryByText( 'WordPress' ) ).not.toBeInTheDocument();
expect( screen.queryByText( 'WordPress' ) ).not.toBeVisible();

// Move focus to the button
await user.tab();
Expand Down Expand Up @@ -230,12 +231,14 @@ describe( 'Button', () => {
/>
);

expect( screen.queryByText( 'Label' ) ).not.toBeInTheDocument();
expect( screen.queryByText( 'Label' ) ).not.toBeVisible();

// Move focus to the button
await user.tab();

expect( screen.getByText( 'Label' ) ).toBeVisible();

await cleanupTooltip( user );
} );

it( 'should populate tooltip with description content for buttons with visible labels (buttons with children)', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of unrelated to this PR, I don't think this test has the right assertion based on the test title. The test queries the button but not the tootlip. I bring this up because I would have thought we'd use the cleanup function here too. But this could be a follow-up PR.

Expand Down Expand Up @@ -287,12 +290,14 @@ describe( 'Button', () => {
<Button icon={ plusCircle } label="WordPress" children={ [] } />
);

expect( screen.queryByText( 'WordPress' ) ).not.toBeInTheDocument();
expect( screen.queryByText( 'WordPress' ) ).not.toBeVisible();

// Move focus to the button
await user.tab();

expect( screen.getByText( 'WordPress' ) ).toBeVisible();

await cleanupTooltip( user );
} );

it( 'should not show the tooltip when icon and children defined', async () => {
Expand Down Expand Up @@ -321,12 +326,14 @@ describe( 'Button', () => {
</Button>
);

expect( screen.queryByText( 'WordPress' ) ).not.toBeInTheDocument();
expect( screen.queryByText( 'WordPress' ) ).not.toBeVisible();

// Move focus to the button
await user.tab();

expect( screen.getByText( 'WordPress' ) ).toBeVisible();

await cleanupTooltip( user );
} );
} );

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ exports[`ColorPalette should allow disabling custom color picker 1`] = `
class="components-circular-option-picker__option-wrapper"
>
<button
aria-describedby="tooltip-15"
aria-label="Color: red"
aria-pressed="true"
class="components-button components-circular-option-picker__option is-pressed"
Expand All @@ -64,6 +65,7 @@ exports[`ColorPalette should allow disabling custom color picker 1`] = `
class="components-circular-option-picker__option-wrapper"
>
<button
aria-describedby="tooltip-16"
aria-label="Color: green"
aria-pressed="false"
class="components-button components-circular-option-picker__option"
Expand All @@ -75,6 +77,7 @@ exports[`ColorPalette should allow disabling custom color picker 1`] = `
class="components-circular-option-picker__option-wrapper"
>
<button
aria-describedby="tooltip-17"
aria-label="Color: blue"
aria-pressed="false"
class="components-button components-circular-option-picker__option"
Expand Down Expand Up @@ -229,6 +232,7 @@ exports[`ColorPalette should render a dynamic toolbar of colors 1`] = `
class="components-circular-option-picker__option-wrapper"
>
<button
aria-describedby="tooltip-0"
aria-label="Color: red"
aria-pressed="true"
class="components-button components-circular-option-picker__option is-pressed"
Expand All @@ -253,6 +257,7 @@ exports[`ColorPalette should render a dynamic toolbar of colors 1`] = `
class="components-circular-option-picker__option-wrapper"
>
<button
aria-describedby="tooltip-1"
aria-label="Color: green"
aria-pressed="false"
class="components-button components-circular-option-picker__option"
Expand All @@ -264,6 +269,7 @@ exports[`ColorPalette should render a dynamic toolbar of colors 1`] = `
class="components-circular-option-picker__option-wrapper"
>
<button
aria-describedby="tooltip-2"
aria-label="Color: blue"
aria-pressed="false"
class="components-button components-circular-option-picker__option"
Expand Down
6 changes: 5 additions & 1 deletion packages/components/src/color-palette/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,11 @@ describe( 'ColorPalette', () => {
// Clear the color, confirm that the relative values are cleared/updated.
await user.click( screen.getByRole( 'button', { name: 'Clear' } ) );
expect( screen.getByText( 'No color selected' ) ).toBeVisible();
expect( screen.queryByText( colorName ) ).not.toBeInTheDocument();
expect(
screen.queryByText( colorName, {
selector: '.components-color-palette__custom-color-name',
} )
).not.toBeInTheDocument();
expect( screen.queryByText( colorCode ) ).not.toBeInTheDocument();
expect(
screen.getByRole( 'button', {
Expand Down
23 changes: 14 additions & 9 deletions packages/components/src/tab-panel/test/index.tsx
ciampo marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { wordpress, category, media } from '@wordpress/icons';
* Internal dependencies
*/
import TabPanel from '..';
import cleanupTooltip from '../../tooltip/test/utils';

const TABS = [
{
Expand Down Expand Up @@ -116,7 +117,7 @@ describe.each( [
for ( let i = 0; i < allTabs.length; i++ ) {
expect(
screen.queryByText( TABS_WITH_ICON[ i ].title )
).not.toBeInTheDocument();
).not.toBeVisible();

await user.hover( allTabs[ i ] );

Expand All @@ -128,6 +129,8 @@ describe.each( [

await user.unhover( allTabs[ i ] );
}

await cleanupTooltip( user );
} );

it( 'should display a tooltip when moving the selection via the keyboard on tabs provided with an icon', async () => {
Expand Down Expand Up @@ -157,38 +160,40 @@ describe.each( [

// Tab to focus the tablist. Make sure alpha is focused, and that the
// corresponding tooltip is shown.
expect( screen.queryByText( 'Alpha' ) ).not.toBeInTheDocument();
expect( screen.queryByText( 'Alpha' ) ).not.toBeVisible();
await user.keyboard( '[Tab]' );
expect( mockOnSelect ).toHaveBeenCalledTimes( 1 );
expect( screen.getByText( 'Alpha' ) ).toBeInTheDocument();
expect( screen.getByText( 'Alpha' ) ).toBeVisible();
expect( await getSelectedTab() ).toHaveFocus();

// Move selection with arrow keys. Make sure beta is focused, and that
// the corresponding tooltip is shown.
expect( screen.queryByText( 'Beta' ) ).not.toBeInTheDocument();
expect( screen.queryByText( 'Beta' ) ).not.toBeVisible();
await user.keyboard( '[ArrowRight]' );
expect( mockOnSelect ).toHaveBeenCalledTimes( 2 );
expect( mockOnSelect ).toHaveBeenLastCalledWith( 'beta' );
expect( screen.getByText( 'Beta' ) ).toBeInTheDocument();
expect( screen.getByText( 'Beta' ) ).toBeVisible();
expect( await getSelectedTab() ).toHaveFocus();

// Move selection with arrow keys. Make sure gamma is focused, and that
// the corresponding tooltip is shown.
expect( screen.queryByText( 'Gamma' ) ).not.toBeInTheDocument();
expect( screen.queryByText( 'Gamma' ) ).not.toBeVisible();
await user.keyboard( '[ArrowRight]' );
expect( mockOnSelect ).toHaveBeenCalledTimes( 3 );
expect( mockOnSelect ).toHaveBeenLastCalledWith( 'gamma' );
expect( screen.getByText( 'Gamma' ) ).toBeInTheDocument();
expect( screen.getByText( 'Gamma' ) ).toBeVisible();
expect( await getSelectedTab() ).toHaveFocus();

// Move selection with arrow keys. Make sure beta is focused, and that
// the corresponding tooltip is shown.
expect( screen.queryByText( 'Beta' ) ).not.toBeInTheDocument();
expect( screen.queryByText( 'Beta' ) ).not.toBeVisible();
await user.keyboard( '[ArrowLeft]' );
expect( mockOnSelect ).toHaveBeenCalledTimes( 4 );
expect( mockOnSelect ).toHaveBeenLastCalledWith( 'beta' );
expect( screen.getByText( 'Beta' ) ).toBeInTheDocument();
expect( screen.getByText( 'Beta' ) ).toBeVisible();
expect( await getSelectedTab() ).toHaveFocus();

await cleanupTooltip( user );
} );
} );

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ exports[`ToggleGroupControl should render correctly with icons 1`] = `
>
<button
aria-checked="true"
aria-describedby="tooltip-0"
aria-label="Uppercase"
class="emotion-14 components-toggle-group-control-option-base"
data-value="uppercase"
Expand Down Expand Up @@ -305,6 +306,7 @@ exports[`ToggleGroupControl should render correctly with icons 1`] = `
>
<button
aria-checked="false"
aria-describedby="tooltip-1"
aria-label="Lowercase"
class="emotion-19 components-toggle-group-control-option-base"
data-value="lowercase"
Expand Down
13 changes: 3 additions & 10 deletions packages/components/src/toggle-group-control/test/index.tsx
ciampo marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,7 @@ import {
ToggleGroupControlOption,
ToggleGroupControlOptionIcon,
} from '../index';

function getWrappingPopoverElement( element: HTMLElement ) {
return element.closest( '.components-popover' );
}
import cleanupTooltip from '../../tooltip/test/utils';

describe( 'ToggleGroupControl', () => {
const options = (
Expand Down Expand Up @@ -115,13 +112,9 @@ describe( 'ToggleGroupControl', () => {
'Click for Delicious Gnocchi'
);

await waitFor( () =>
expect(
getWrappingPopoverElement( tooltip )
).toBePositionedPopover()
);
await waitFor( () => expect( tooltip ).toBeVisible() );

expect( tooltip ).toBeVisible();
await cleanupTooltip( user );
} );

it( 'should not render tooltip', async () => {
Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/toggle-group-control/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type { RadioStateReturn } from 'reakit';
* Internal dependencies
*/
import type { BaseControlProps } from '../base-control/types';
import type { TooltipProps } from '../tooltip/types';

export type ToggleGroupControlOptionBaseProps = {
children: ReactNode;
Expand Down Expand Up @@ -58,7 +59,7 @@ export type WithToolTipProps = {
/**
* React children
*/
children: ReactNode;
children: TooltipProps[ 'children' ];
/**
* Label for the Tooltip component.
*/
Expand Down
45 changes: 17 additions & 28 deletions packages/components/src/tooltip/README.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# Tooltip
# ToolTip

Tooltip is a React component to render floating help text relative to a node when it receives focus or when the user places the mouse cursor atop it. If the tooltip exceeds the bounds of the page in the direction it opens, its position will be flipped automatically.
Tooltip is a React component to render floating help text relative to a node when it receives focus or it is hovered upon by a mouse. If the tooltip exceeds the bounds of the page in the direction it opens, its position will be flipped automatically.

Accessibility note: the tooltip text is hidden from screen readers and assistive technologies that understand ARIA. To make it accessible, use an `aria-label` attribute on the same element the tooltip is applied to, preferably using the same text used for the tooltip.
This component utilizes [Ariakit's tooltip](https://ariakit.org/components/tooltip) which is based on the [WAI-ARIA Tooltip Pattern](https://www.w3.org/WAI/ARIA/apg/patterns/tooltip/).

ciampo marked this conversation as resolved.
Show resolved Hide resolved
## Usage

Expand All @@ -22,49 +22,38 @@ const MyTooltip = () => (

The component accepts the following props:

### position

The direction in which the tooltip should open relative to its parent node. Specify y- and x-axis as a space-separated string. Supports `"top"`, `"bottom"` y axis, and `"left"`, `"center"`, `"right"` x axis.

- Type: `String`
- Required: No
- Default: `"bottom"`

### children
#### `children`: `Element`

The element to which the tooltip should anchor.

**NOTE:** You must pass only a single child. Tooltip renders itself as a clone of `children` with a [`Popover`](/packages/components/src/popover/README.md) added as an additional child.
**NOTE:** Accepts only one child element.

- Type: `Element`
- Required: Yes

### text
#### `delay`: `number`

The tooltip text to show on focus or hover.
The amount of time in milliseconds to wait before showing the tooltip.

- Type: `String`
- Required: No
- Default: `700`

#### `position`: `string`

### shortcut (web only)
The direction in which the tooltip should open relative to its parent node. Specify y- and x-axis as a space-separated string. Supports `"top"`, `"bottom"` y axis, and `"left"`, `"center"`, `"right"` x axis.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've discussed this, but noting so we don't forget: Let's start the deprecation process for this (positionplacement) as a follow-up issue once this PR is merged.

- Type: `string` or `object`
- Required: No
- Default: `"bottom"`

If shortcut is a string, it is expecting the display text. If shortcut is an object, it will accept the properties of `display` (string) and `ariaLabel` (string).
#### `shortcut`: `string` | `object`

### delay (web only)
An option for adding accessible keyboard shortcuts.

Time in milliseconds to wait before showing tooltip after the tooltip's visibility is toggled. This prop is currently only available for the web platforms.
If shortcut is a string, it is expecting the display text. If shortcut is an object, it will accept the properties of `display`: `string` and `ariaLabel`: `string`.

- Type: `Number`
- Required: No
- Default: 700

### visible (native only)
#### `text`: `string`

Whether the tooltip should be displayed on initial render. This prop is currently only available for the native mobile app built with React Native.
The text shown in the tooltip when anchor element is focused or hovered.

- Type: `Boolean`
- Required: No
- Default: `false`
Loading
Loading