add event listener to close tip on escape #7566

britt6612 · 2025-04-01T14:40:53Z

What does this PR do?

This PR adds an event listener to the tooltip so that when a user clicks escape the tooltip will close

Where should the reviewer start?

tip.js

What testing has been done on this PR?

storybook

How should this be manually tested?

open storybook of any of the tip stories and open in a new window then hover over tip and press escape

Do Jest tests follow these best practices?

screen is used for querying.
The correct query is used. (Refer to this list of queries)
asFragment() is used for snapshot testing.

Any background context you want to provide?

Tip is currently not passing these WCAG points for WCAG 1.4.13:

Dismissible
A mechanism is available to dismiss the additional content without moving pointer hover or keyboard focus, unless the additional content communicates an input error or does not obscure or replace other content;

What are the relevant issues?

Part 1 of #7552

Screenshots (if appropriate)

Do the grommet docs need to be updated?

no

Should this PR be mentioned in the release notes?

yes

Is this change backwards compatible or is it a breaking change?

backwards compatible

jcfilben

On the tip/simple story when I hover onto the tooltip content and then try to press 'esc' the tip doesn't close.

If I set defaultVisible=true and then hover over the target and press 'esc' the tip doesn't close.

src/js/components/Tip/Tip.js

jcfilben · 2025-04-01T17:34:27Z

On the tip/simple story when I hover onto the tooltip content and then try to press 'esc' the tip doesn't close.

If I set defaultVisible=true and then hover over the target and press 'esc' the tip doesn't close.

Update: confirmed these are working as expected when I open the story in a new window

MikeKingdom · 2025-04-03T20:24:19Z

src/js/components/Tip/Tip.js

@@ -21,6 +21,20 @@ const Tip = forwardRef(

    const componentRef = useForwardedRef(tipRef);

+    useEffect(() => {


Just occurred to me that we may want to use <Keyboard> instead like we do in Layer for onEsc and avoid the useEffect and such altogether. I was just reading something this morning about avoiding this kind of usage of useEffect

it was originally not working with keyboard but now that I see its more not working in the storybook iframe let me try and switch out with keyboard

Just occurred to me that we may want to use <Keyboard> instead like we do in Layer for onEsc and avoid the useEffect and such altogether. I was just reading something this morning about avoiding this kind of usage of useEffect

This usage of useEffect is very typical and appropriate. The article I read was trying to advocate using this newer API instead:

import { useSyncExternalStore } from 'react'; // Simplified example for browser window size function useWindowSize() { return useSyncExternalStore( // Subscribe function callback => { window.addEventListener('resize', callback); return () => window.removeEventListener('resize', callback); }, // Get snapshot function () => ({ width: window.innerWidth, height: window.innerHeight }) ); }

aw gotcha will I just switched it out with keyboard but my test that I originally had was now failing. But if you want to test this way out and check out the code lmk I can work on the test in backround

jcfilben

Behavior looks good, just holding off on marking as approved until jest test is ready

britt6612 · 2025-04-04T05:48:21Z

Here is the test I have

  test('pressing Escape key closes tooltip', async () => {
    const onEsc = jest.fn();
    render(
      <Grommet>
        <Tip content="tooltip text" defaultVisible>
          <button>Hover me</button>
        </Tip>
      </Grommet>,
    );

    expect(screen.getByText('tooltip text')).toBeInTheDocument();

    // Simulate pressing the Escape key
    fireEvent.keyDown(document, { key: 'Escape', keyCode: 27, which: 27 });

    // Ensure Escape key is pressed
    expect(onEsc).toHaveBeenCalledTimes(1);

    // await waitFor(() => {
    //   expect(screen.queryByText('tooltip text')).not.toBeInTheDocument();
    // });
  });

unfortunately though in the logs its showing Escape key is presses 0 times. Im not sure if either of you have ideas on why this wouldnt be called with the fireEvent

MikeKingdom · 2025-04-04T16:08:32Z

unfortunately though in the logs its showing Escape key is presses 0 times. Im not sure if either of you have ideas on why this wouldnt be called with the fireEvent

I've never had much luck with certain key handlers in jest tests. I think if they are at the window level they don't really work like they do in the browser.

MikeKingdom · 2025-04-04T16:17:01Z

unfortunately though in the logs its showing Escape key is presses 0 times. Im not sure if either of you have ideas on why this wouldnt be called with the fireEvent

I've never had much luck with certain key handlers in jest tests. I think if they are at the window level they don't really work like they do in the browser.

Just a thought after testing the tip, can you try adding focus to whatever component you're rendering? Storybook only works if you put focus in the story pane somewhere

britt6612 · 2025-04-04T16:23:00Z

unfortunately though in the logs its showing Escape key is presses 0 times. Im not sure if either of you have ideas on why this wouldnt be called with the fireEvent

I've never had much luck with certain key handlers in jest tests. I think if they are at the window level they don't really work like they do in the browser.

Just a thought after testing the tip, can you try adding focus to whatever component you're rendering? Storybook only works if you put focus in the story pane somewhere

yes I can do that for the test. I kinda wanted to test the hover first then press escape but yes if I focus the button then press escape the test will be fine

UPDATE: Added the test where I added focus to the button then press escape @jcfilben @MikeKingdom

MikeKingdom

Looks good. Just waiting on the circle ci tests

jcfilben

Look good!

britt6612 added 2 commits April 1, 2025 07:52

add event listener to close tip on escape

7bb480e

add test

b8d5440

britt6612 requested review from jcfilben and taysea April 1, 2025 14:47

britt6612 added 2 commits April 1, 2025 09:43

change window

edd8c2d

take out extra space

e5c348d

jcfilben reviewed Apr 1, 2025

View reviewed changes

src/js/components/Tip/Tip.js Outdated Show resolved Hide resolved

britt6612 requested a review from MikeKingdom April 3, 2025 18:33

MikeKingdom approved these changes Apr 3, 2025

View reviewed changes

MikeKingdom reviewed Apr 3, 2025

View reviewed changes

britt6612 and others added 2 commits April 3, 2025 16:09

add keyboard

5e3ecec

Update Tip.js

caf6ae5

britt6612 requested a review from MikeKingdom April 3, 2025 22:12

jcfilben reviewed Apr 3, 2025

View reviewed changes

britt6612 requested a review from jcfilben April 4, 2025 05:48

add test with focus

8f1c20c

MikeKingdom reviewed Apr 4, 2025

View reviewed changes

jcfilben approved these changes Apr 4, 2025

View reviewed changes

Merge branch 'master' into fix-tooltip-7552

7a3496a

MikeKingdom approved these changes Apr 4, 2025

View reviewed changes

jcfilben merged commit a2d957b into grommet:master Apr 4, 2025
13 of 14 checks passed

jcfilben mentioned this pull request Apr 8, 2025

April 2025 Grommet Release grommet/hpe-design-system#4945

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

add event listener to close tip on escape #7566

add event listener to close tip on escape #7566

Uh oh!

britt6612 commented Apr 1, 2025

Uh oh!

jcfilben left a comment

Uh oh!

Uh oh!

jcfilben commented Apr 1, 2025

Uh oh!

MikeKingdom Apr 3, 2025

Uh oh!

britt6612 Apr 3, 2025

Uh oh!

MikeKingdom Apr 3, 2025

Uh oh!

britt6612 Apr 3, 2025

Uh oh!

jcfilben left a comment

Uh oh!

britt6612 commented Apr 4, 2025 •

edited

Loading

Uh oh!

MikeKingdom commented Apr 4, 2025

Uh oh!

MikeKingdom commented Apr 4, 2025

Uh oh!

britt6612 commented Apr 4, 2025 •

edited

Loading

Uh oh!

MikeKingdom left a comment

Uh oh!

jcfilben left a comment

Uh oh!

Uh oh!

Uh oh!

		@@ -21,6 +21,20 @@ const Tip = forwardRef(

		const componentRef = useForwardedRef(tipRef);

		useEffect(() => {

add event listener to close tip on escape #7566

add event listener to close tip on escape #7566

Uh oh!

Conversation

britt6612 commented Apr 1, 2025

What does this PR do?

Where should the reviewer start?

What testing has been done on this PR?

How should this be manually tested?

Do Jest tests follow these best practices?

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

Uh oh!

jcfilben left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcfilben commented Apr 1, 2025

Uh oh!

MikeKingdom Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

britt6612 Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

MikeKingdom Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

britt6612 Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

jcfilben left a comment

Choose a reason for hiding this comment

Uh oh!

britt6612 commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeKingdom commented Apr 4, 2025

Uh oh!

MikeKingdom commented Apr 4, 2025

Uh oh!

britt6612 commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeKingdom left a comment

Choose a reason for hiding this comment

Uh oh!

jcfilben left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

britt6612 commented Apr 4, 2025 •

edited

Loading

britt6612 commented Apr 4, 2025 •

edited

Loading