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

DropdownMenuV2: prevent default on Escape key presses #55962

Merged
merged 6 commits into from Nov 9, 2023

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Nov 8, 2023

What?

This PR changes the default behaviour of the new experimental DropdownMenu component so that the keyboard event related to pressing the Escape key is default prevented.

Why?

As pointed out in #55625 (comment), pressing the Escape key to close the dropdown menu can have unintended consequences (like, for example, causing a browser on MacOs to exit full screen mode).

How?

Instead of using a boolean for the hideOnEscape prop, we can use a callback function that accepts the event object. We can then call preventDefault() on it.

Also, note that this PR only changes the default behaviour of the component. Folks wishing to have the keyboard event bubbling up can simply pass a different value for the prop.

Testing Instructions

  1. Make sure that, when pressing the Escape key, the dropdown menu still closes correctly
  2. Make sure that pressing the event associated to the Escape key pressed when closing the dropdown menu is default prevented:
    • With changes from this PR downloaded on a local machine, run npm run storybook:dev
    • Visit the dropdown menu v2 storybook example, and open the example in a new window (ie. likely this URL)
    • Open the browser dev tools, and in the console type document.addEventListener('keydown', (e) => console.log(e.key, e.defaultPrevented))
    • Interact with the dropdown menu. When pressing the Escape key with an open menu, the console output should be Escape true. When pressing the escape key when the menu is closed, the console output should be Escape false.
  3. As an optional test, load the Menu's storybook example on MacOS safari and make the Safari window full screen. Pressing the Escape key while the menu is open should not cause Safari to exit full screen.

@ciampo ciampo assigned ciampo and youknowriad and unassigned youknowriad Nov 8, 2023
@ciampo ciampo requested review from youknowriad and a team November 8, 2023 10:44
@ciampo ciampo added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Nov 8, 2023
@@ -181,7 +181,13 @@ const UnconnectedDropdownMenu = (
children,
shift,
modal = true,
hideOnEscape = true,
hideOnEscape = ( event ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, since we're just passing down this prop and it's ariakit that is handling the ESCAPE key, I think the ideal fix would have to be done in ariakit directly

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with a fix here for now, but maybe this could benefit more users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@diegohaz maybe you can share some perspective as to why this is not the case in ariakit?

Copy link
Member

Choose a reason for hiding this comment

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

I can't remember any discussion about hideOnEscape and fullscreen mode in particular, so there's likely no specific reason. It seems it just hasn't been considered yet.

But there's an issue with fullscreen and portal: ariakit/ariakit#865

@ciampo ciampo force-pushed the fix/dropdown-menu-esc-prevent-default branch from 0d19ff6 to 897f3fc Compare November 9, 2023 15:14
@ciampo ciampo changed the title DropdownMenuV2: prevent default on Escape key presses DropdownMenuV2: prevent default and stop propagation on Escape key presses Nov 9, 2023
// Pressing Escape can cause unexpected consequences (ie. exiting
// full screen mode on MacOs, close parent modals...).
event.preventDefault();
event.stopPropagation();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any particular reason to stop propagation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without it, I thought that, when a DropdownMenu is rendered inside a Modal, pressing Escape would close the DropdownMenu and the parent modal.

That was my impression because, when talking to Diego, I seem to remember him suggesting this change specifically for when a DropdownMenu is rendered inside a Modal.

But I just tested without it, and things seem to work well.

@ciampo ciampo changed the title DropdownMenuV2: prevent default and stop propagation on Escape key presses DropdownMenuV2: prevent default on Escape key presses Nov 9, 2023
@ciampo ciampo enabled auto-merge (squash) November 9, 2023 15:39
Copy link

github-actions bot commented Nov 9, 2023

Flaky tests detected in 897f3fc.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6813741126
📝 Reported issues:

@ciampo ciampo merged commit 36617fb into trunk Nov 9, 2023
50 checks passed
@ciampo ciampo deleted the fix/dropdown-menu-esc-prevent-default branch November 9, 2023 16:48
@github-actions github-actions bot added this to the Gutenberg 17.1 milestone Nov 9, 2023
cbravobernal pushed a commit that referenced this pull request Nov 14, 2023
* DropdownMenuV2: prevent ESC key presses from propagating outside of the component by default

* CHANGELOG

* Storybook control improvements

* Don't expose 'hideOnEscape' prop, keep internal

* Memoize wrapperProps

* Remove stopPropagation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants