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.

Sign up for GitHub

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

Jump to bottom

DropdownMenuV2: prevent default on Escape key presses #55962

Merged
merged 6 commits into from
Nov 9, 2023
Merged

DropdownMenuV2: prevent default on Escape key presses #55962

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
afdebdc
DropdownMenuV2: prevent ESC key presses from propagating outside of t…
ciampo Nov 8, 2023
a2f5a8b
CHANGELOG
ciampo Nov 8, 2023
7ae4f6d
Storybook control improvements
ciampo Nov 8, 2023
43fb40f
Don't expose 'hideOnEscape' prop, keep internal
ciampo Nov 9, 2023
897f3fc
Memoize wrapperProps
ciampo Nov 9, 2023
de0a7e8
Remove stopPropagation
ciampo Nov 9, 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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
- `Tabs`: Add `focusable` prop to the `Tabs.TabPanel` sub-component ([#55287](https://github.com/WordPress/gutenberg/pull/55287))
- `Tabs`: Update sub-components to accept relevant HTML element props ([#55860](https://github.com/WordPress/gutenberg/pull/55860))
- `DropdownMenuV2`: Fix radio menu item check icon not rendering correctly in some browsers ([#55964](https://github.com/WordPress/gutenberg/pull/55964))
- `DropdownMenuV2`: prevent default when pressing Escape key to close menu ([#55962](https://github.com/WordPress/gutenberg/pull/55962))

### Enhancements

Expand Down
7 changes: 0 additions & 7 deletions packages/components/src/dropdown-menu-v2-ariakit/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,6 @@ The skidding of the popover along the anchor element. Can be set to negative val
- Required: no
- Default: `0` for root-level menus, `-8` for nested menus

##### `hideOnEscape`: `boolean | ( ( event: KeyboardEvent | React.KeyboardEvent< Element > ) => boolean )`

Determines whether the menu popover will be hidden when the user presses the Escape key.

- Required: no
- Default: `true`

### `DropdownMenuItem`

Used to render a menu item.
Expand Down
32 changes: 25 additions & 7 deletions packages/components/src/dropdown-menu-v2-ariakit/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
useMemo,
cloneElement,
isValidElement,
useCallback,
} from '@wordpress/element';
import { isRTL } from '@wordpress/i18n';
import { check, chevronRightSmall } from '@wordpress/icons';
Expand Down Expand Up @@ -180,7 +181,6 @@ const UnconnectedDropdownMenu = (
children,
shift,
modal = true,
hideOnEscape = true,

// From internal components context
variant,
Expand Down Expand Up @@ -248,6 +248,29 @@ const UnconnectedDropdownMenu = (
);
}

const hideOnEscape = useCallback(
( event: React.KeyboardEvent< Element > ) => {
// 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.

// Returning `true` causes the menu to hide.
return true;
},
[]
);

const wrapperProps = useMemo(
() => ( {
dir: computedDirection,
style: {
direction:
computedDirection as React.CSSProperties[ 'direction' ],
},
} ),
[ computedDirection ]
);

return (
<>
{ /* Menu trigger */ }
Expand Down Expand Up @@ -280,12 +303,7 @@ const UnconnectedDropdownMenu = (
hideOnHoverOutside={ false }
data-side={ appliedPlacementSide }
variant={ variant }
wrapperProps={ {
dir: computedDirection,
style: {
direction: computedDirection,
},
} }
wrapperProps={ wrapperProps }
hideOnEscape={ hideOnEscape }
unmountOnHide
>
Expand Down
5 changes: 1 addition & 4 deletions packages/components/src/dropdown-menu-v2-ariakit/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const meta: Meta< typeof DropdownMenu > = {
},
argTypes: {
children: { control: { type: null } },
trigger: { control: { type: null } },
},
parameters: {
actions: { argTypesRegex: '^on.*' },
Expand Down Expand Up @@ -494,10 +495,6 @@ export const InsideModal: StoryFn< typeof DropdownMenu > = ( props ) => {
};
InsideModal.args = {
...Default.args,
hideOnEscape: ( e ) => {
e.stopPropagation();
return true;
},
};
InsideModal.parameters = {
docs: {
Expand Down
26 changes: 0 additions & 26 deletions packages/components/src/dropdown-menu-v2-ariakit/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,32 +197,6 @@ describe( 'DropdownMenu', () => {
);
} );

it( 'should not close when pressing the escape key if the `hideOnEscape` prop is set to `false`', async () => {
render(
<DropdownMenu
trigger={ <button>Open dropdown</button> }
hideOnEscape={ false }
>
<DropdownMenuItem>Dropdown menu item</DropdownMenuItem>
</DropdownMenu>
);

const trigger = screen.getByRole( 'button', {
name: 'Open dropdown',
} );

await click( trigger );

// Focuses menu on mouse click, focuses first item on keyboard press
// Can be changed with a custom useEffect
expect( screen.getByRole( 'menu' ) ).toHaveFocus();

// Pressing esc will close the menu and move focus to the toggle
await press.Escape();

expect( screen.getByRole( 'menu' ) ).toHaveFocus();
} );

it( 'should close when clicking outside of the content', async () => {
render(
<DropdownMenu
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/dropdown-menu-v2-ariakit/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export interface DropdownMenuProps {
* Determines whether the menu popover will be hidden when the user presses
* the Escape key.
*
* @default true
* @default `( event ) => { event.preventDefault(); return true; }`
*/
hideOnEscape?:
| boolean
Expand Down
Loading