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: render tooltip markup in the DOM only when open #54312

Merged
merged 5 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,6 @@ exports[`ColorPaletteControl matches the snapshot 1`] = `
class="components-circular-option-picker__option-wrapper"
>
<button
aria-describedby="tooltip-0"
aria-label="Color: red"
aria-selected="true"
class="components-button components-circular-option-picker__option is-pressed"
Expand Down
16 changes: 4 additions & 12 deletions packages/block-editor/src/components/link-control/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,7 @@ describe( 'Basic rendering', () => {
await user.type( searchInput, 'Hello' );

// Wait for the spinner SVG icon to be rendered.
expect(
await screen.findByTestId( 'components-spinner' )
).toBeVisible();
expect( await screen.findByRole( 'presentation' ) ).toBeVisible();
// Check the suggestions list is not rendered yet.
expect( screen.queryByRole( 'listbox' ) ).not.toBeInTheDocument();

Expand All @@ -192,9 +190,7 @@ describe( 'Basic rendering', () => {
// Check the suggestions list is rendered.
expect( resultsList ).toBeVisible();
// Check the spinner SVG icon is not rendered any longer.
expect(
screen.queryByTestId( 'components-spinner' )
).not.toBeInTheDocument();
expect( screen.queryByRole( 'presentation' ) ).not.toBeInTheDocument();

const searchResultElements =
within( resultsList ).getAllByRole( 'option' );
Expand Down Expand Up @@ -459,18 +455,14 @@ describe( 'Searching for a link', () => {
// Simulate searching for a term.
await user.type( searchInput, searchTerm );

expect(
await screen.findByTestId( 'components-spinner' )
).toBeVisible();
expect( await screen.findByRole( 'presentation' ) ).toBeVisible();
expect( screen.queryByRole( 'listbox' ) ).not.toBeInTheDocument();

// make the search suggestions fetch return a response
resolver( fauxEntitySuggestions );

expect( await screen.findByRole( 'listbox' ) ).toBeVisible();
expect(
screen.queryByTestId( 'components-spinner' )
).not.toBeInTheDocument();
expect( screen.queryByRole( 'presentation' ) ).not.toBeInTheDocument();
} );

it.each( [ 'With spaces', 'Uppercase', 'lowercase' ] )(
Expand Down
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

### Bug Fix

- `Tooltip`: dynamically render in the DOM only when visible ([#54312](https://github.com/WordPress/gutenberg/pull/54312)).
- `PaletteEdit`: Fix padding in RTL languages ([#54034](https://github.com/WordPress/gutenberg/pull/54034)).
- `CircularOptionPicker`: make focus styles resilient to button size changes ([#54196](https://github.com/WordPress/gutenberg/pull/54196)).

Expand Down
8 changes: 4 additions & 4 deletions packages/components/src/button/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ describe( 'Button', () => {

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

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

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

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

// Move focus to the button
await user.tab();
Expand Down Expand Up @@ -290,7 +290,7 @@ describe( 'Button', () => {
<Button icon={ plusCircle } label="WordPress" children={ [] } />
);

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

// Move focus to the button
await user.tab();
Expand Down Expand Up @@ -326,7 +326,7 @@ describe( 'Button', () => {
</Button>
);

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

// Move focus to the button
await user.tab();
Expand Down
1 change: 0 additions & 1 deletion packages/components/src/spinner/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ export function UnforwardedSpinner(
focusable="false"
{ ...props }
ref={ forwardedRef }
data-testid="components-spinner"
>
{ /* Gray circular background */ }
<SpinnerTrack
Expand Down
18 changes: 9 additions & 9 deletions packages/components/src/tab-panel/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ describe.each( [
for ( let i = 0; i < allTabs.length; i++ ) {
expect(
screen.queryByText( TABS_WITH_ICON[ i ].title )
).not.toBeVisible();
).not.toBeInTheDocument();

await user.hover( allTabs[ i ] );

Expand Down Expand Up @@ -160,37 +160,37 @@ describe.each( [

// Tab to focus the tablist. Make sure alpha is focused, and that the
// corresponding tooltip is shown.
expect( screen.queryByText( 'Alpha' ) ).not.toBeVisible();
expect( screen.queryByText( 'Alpha' ) ).not.toBeInTheDocument();
await user.keyboard( '[Tab]' );
expect( mockOnSelect ).toHaveBeenCalledTimes( 1 );
expect( screen.getByText( 'Alpha' ) ).toBeVisible();
expect( screen.getByText( 'Alpha' ) ).toBeInTheDocument();
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.toBeVisible();
expect( screen.queryByText( 'Beta' ) ).not.toBeInTheDocument();
await user.keyboard( '[ArrowRight]' );
expect( mockOnSelect ).toHaveBeenCalledTimes( 2 );
expect( mockOnSelect ).toHaveBeenLastCalledWith( 'beta' );
expect( screen.getByText( 'Beta' ) ).toBeVisible();
expect( screen.getByText( 'Beta' ) ).toBeInTheDocument();
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.toBeVisible();
expect( screen.queryByText( 'Gamma' ) ).not.toBeInTheDocument();
await user.keyboard( '[ArrowRight]' );
expect( mockOnSelect ).toHaveBeenCalledTimes( 3 );
expect( mockOnSelect ).toHaveBeenLastCalledWith( 'gamma' );
expect( screen.getByText( 'Gamma' ) ).toBeVisible();
expect( screen.getByText( 'Gamma' ) ).toBeInTheDocument();
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.toBeVisible();
expect( screen.queryByText( 'Beta' ) ).not.toBeInTheDocument();
await user.keyboard( '[ArrowLeft]' );
expect( mockOnSelect ).toHaveBeenCalledTimes( 4 );
expect( mockOnSelect ).toHaveBeenLastCalledWith( 'beta' );
expect( screen.getByText( 'Beta' ) ).toBeVisible();
expect( screen.getByText( 'Beta' ) ).toBeInTheDocument();
expect( await getSelectedTab() ).toHaveFocus();

await cleanupTooltip( user );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,6 @@ exports[`ToggleGroupControl controlled should render correctly with icons 1`] =
>
<button
aria-checked="true"
aria-describedby="tooltip-4"
aria-label="Uppercase"
class="emotion-12 components-toggle-group-control-option-base"
data-active-item=""
Expand Down Expand Up @@ -285,7 +284,6 @@ exports[`ToggleGroupControl controlled should render correctly with icons 1`] =
>
<button
aria-checked="false"
aria-describedby="tooltip-5"
aria-label="Lowercase"
class="emotion-18 components-toggle-group-control-option-base"
data-command=""
Expand Down Expand Up @@ -789,7 +787,6 @@ exports[`ToggleGroupControl uncontrolled should render correctly with icons 1`]
>
<button
aria-checked="true"
aria-describedby="tooltip-0"
aria-label="Uppercase"
class="emotion-12 components-toggle-group-control-option-base"
data-active-item=""
Expand Down Expand Up @@ -828,7 +825,6 @@ exports[`ToggleGroupControl uncontrolled should render correctly with icons 1`]
>
<button
aria-checked="false"
aria-describedby="tooltip-1"
aria-label="Lowercase"
class="emotion-18 components-toggle-group-control-option-base"
data-command=""
Expand Down
4 changes: 3 additions & 1 deletion packages/components/src/tooltip/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ function Tooltip( props: TooltipProps ) {
timeout: delay,
} );

const isTooltipOpen = tooltipStore.useState( 'open' );

return (
<>
<Ariakit.TooltipAnchor
Expand All @@ -59,7 +61,7 @@ function Tooltip( props: TooltipProps ) {
>
{ isOnlyChild ? undefined : children }
</Ariakit.TooltipAnchor>
{ isOnlyChild && ( text || shortcut ) && (
{ isOnlyChild && ( text || shortcut ) && isTooltipOpen && (
<Ariakit.Tooltip
className="components-tooltip"
gutter={ 4 }
Expand Down
24 changes: 18 additions & 6 deletions packages/components/src/tooltip/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -272,17 +272,21 @@ describe( 'Tooltip', () => {
</Modal>
);

const tooltip = await screen.findByRole( 'tooltip', { hidden: true } );

expect( tooltip ).toBeInTheDocument();
expect(
screen.queryByRole( 'tooltip', { name: /close/i } )
).not.toBeInTheDocument();

await user.hover(
screen.getByRole( 'button', {
name: /Close/i,
} )
);

await waitFor( () => expect( tooltip ).toBeVisible() );
await waitFor( () =>
expect(
screen.getByRole( 'tooltip', { name: /close/i } )
).toBeVisible()
);

await user.keyboard( '[Escape]' );

Expand All @@ -291,11 +295,19 @@ describe( 'Tooltip', () => {
await cleanupTooltip( user );
} );

it( 'should associate the tooltip text with its anchor via the accessible description', async () => {
it( 'should associate the tooltip text with its anchor via the accessible description when visible', async () => {
const user = userEvent.setup();

render( <Tooltip { ...props } /> );

await user.hover(
screen.getByRole( 'button', {
name: /Button/i,
} )
);

expect(
screen.getByRole( 'button', { description: 'tooltip text' } )
await screen.findByRole( 'button', { description: 'tooltip text' } )
).toBeInTheDocument();
} );
} );
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,6 @@ exports[`PostPublishPanel should render the spinner if the post is being saved 1
>
<svg
class="components-spinner emotion-0 emotion-1"
data-testid="components-spinner"
focusable="false"
height="16"
role="presentation"
Expand Down