From 750e23afd5d85d8920a76be1bb019ca213376609 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Tue, 4 Aug 2026 16:57:00 +0200 Subject: [PATCH 1/8] Menu: Restore focus handoff when items close --- .../components/src/menu/checkbox-item.tsx | 12 +- packages/components/src/menu/item.tsx | 19 +- packages/components/src/menu/radio-item.tsx | 15 +- packages/components/src/menu/test/index.tsx | 349 +++++++++++++----- packages/components/src/menu/types.ts | 35 +- .../src/menu/use-menu-item-hide-on-click.ts | 58 +++ 6 files changed, 347 insertions(+), 141 deletions(-) create mode 100644 packages/components/src/menu/use-menu-item-hide-on-click.ts diff --git a/packages/components/src/menu/checkbox-item.tsx b/packages/components/src/menu/checkbox-item.tsx index 621a28ac024376..f95073f4bff6ef 100644 --- a/packages/components/src/menu/checkbox-item.tsx +++ b/packages/components/src/menu/checkbox-item.tsx @@ -5,33 +5,35 @@ import type { WordPressComponentProps } from '../context'; import { Context } from './context'; import type { CheckboxItemProps } from './types'; import * as Styled from './styles'; +import { useMenuItemHideOnClick } from './use-menu-item-hide-on-click'; export const CheckboxItem = forwardRef< HTMLDivElement, WordPressComponentProps< CheckboxItemProps, 'div', false > >( function CheckboxItem( - { suffix, children, disabled = false, hideOnClick = false, ...props }, + { suffix, children, disabled = false, ...props }, ref ) { const menuContext = useContext( Context ); + const store = menuContext?.store; + const hideOnClick = useMenuItemHideOnClick( store ); - if ( ! menuContext?.store ) { + if ( ! store ) { throw new Error( 'Menu.CheckboxItem can only be rendered inside a Menu component' ); } - return ( } // Override some ariakit inline styles style={ { width: 'auto', height: 'auto' } } diff --git a/packages/components/src/menu/item.tsx b/packages/components/src/menu/item.tsx index b4f85c219c150b..8e266d91dd2ccc 100644 --- a/packages/components/src/menu/item.tsx +++ b/packages/components/src/menu/item.tsx @@ -3,25 +3,20 @@ import type { WordPressComponentProps } from '../context'; import type { ItemProps } from './types'; import * as Styled from './styles'; import { Context } from './context'; +import { useMenuItemHideOnClick } from './use-menu-item-hide-on-click'; export const Item = forwardRef< HTMLDivElement, WordPressComponentProps< ItemProps, 'div', false > >( function Item( - { - prefix, - suffix, - children, - disabled = false, - hideOnClick = true, - store, - ...props - }, + { prefix, suffix, children, disabled = false, store, ...props }, ref ) { const menuContext = useContext( Context ); + const computedStore = store ?? menuContext?.store; + const hideOnClick = useMenuItemHideOnClick( computedStore ); - if ( ! menuContext?.store ) { + if ( ! menuContext?.store || ! computedStore ) { throw new Error( 'Menu.Item can only be rendered inside a Menu component' ); @@ -31,16 +26,14 @@ export const Item = forwardRef< // created by the top-level menu component). But in rare cases (ie. // `Menu.SubmenuTriggerItem`), the context store wouldn't be correct. This is // why the component accepts a `store` prop to override the context store. - const computedStore = store ?? menuContext.store; - return ( { prefix } diff --git a/packages/components/src/menu/radio-item.tsx b/packages/components/src/menu/radio-item.tsx index 912fedf4be4a70..1063fc31112e1b 100644 --- a/packages/components/src/menu/radio-item.tsx +++ b/packages/components/src/menu/radio-item.tsx @@ -6,6 +6,7 @@ import type { WordPressComponentProps } from '../context'; import { Context } from './context'; import type { RadioItemProps } from './types'; import * as Styled from './styles'; +import { useMenuItemHideOnClick } from './use-menu-item-hide-on-click'; const radioCheck = ( @@ -16,29 +17,27 @@ const radioCheck = ( export const RadioItem = forwardRef< HTMLDivElement, WordPressComponentProps< RadioItemProps, 'div', false > ->( function RadioItem( - { suffix, children, disabled = false, hideOnClick = false, ...props }, - ref -) { +>( function RadioItem( { suffix, children, disabled = false, ...props }, ref ) { const menuContext = useContext( Context ); + const store = menuContext?.store; + const hideOnClick = useMenuItemHideOnClick( store ); - if ( ! menuContext?.store ) { + if ( ! store ) { throw new Error( 'Menu.RadioItem can only be rendered inside a Menu component' ); } - return ( } // Override some ariakit inline styles style={ { width: 'auto', height: 'auto' } } diff --git a/packages/components/src/menu/test/index.tsx b/packages/components/src/menu/test/index.tsx index 98e2cf6d0b6c9e..67381becedf036 100644 --- a/packages/components/src/menu/test/index.tsx +++ b/packages/components/src/menu/test/index.tsx @@ -2,6 +2,7 @@ import { act, render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { useState } from '@wordpress/element'; import { Menu } from '..'; +import Modal from '../../modal'; const waitForFocusedMenu = () => waitFor( () => expect( screen.getByRole( 'menu' ) ).toHaveFocus() ); @@ -11,10 +12,63 @@ const waitForFocusedMenuItem = ( name: string ) => expect( screen.getByRole( 'menuitem', { name } ) ).toHaveFocus() ); +const waitForClosedMenu = () => + waitFor( () => + expect( screen.queryByRole( 'menu' ) ).not.toBeInTheDocument() + ); + +const openMenu = async ( user: ReturnType< typeof userEvent.setup > ) => { + await user.click( screen.getByRole( 'button', { name: 'Open dropdown' } ) ); + await waitForFocusedMenu(); +}; + +const isBodyScrollLocked = () => + document.body.style.overflow === 'hidden' || + document.body.classList.contains( 'modal-open' ); + const resetTypeahead = () => { act( () => jest.advanceTimersByTime( 500 ) ); }; +const MenuWithModal = ( { nested = false }: { nested?: boolean } ) => { + const [ isModalOpen, setIsModalOpen ] = useState( false ); + const modalItem = ( + setIsModalOpen( true ) }> + Open modal + + ); + + return ( + <> + + Open dropdown + + { nested ? ( + + + Open submenu + + { modalItem } + + ) : ( + modalItem + ) } + + + { isModalOpen && ( + setIsModalOpen( false ) } + > + + + ) } + + ); +}; + // Open dropdown => open menu // Submenu trigger item => open submenu @@ -222,24 +276,28 @@ describe( 'Menu', () => { ); } ); - it( 'should close when clicking outside of the content', async () => { + it( 'should retain outside focus when outside interaction closes the menu', async () => { render( - - Open dropdown - - Menu item - - + <> + + Open dropdown + + Menu item + + + + ); expect( screen.getByRole( 'menu' ) ).toBeInTheDocument(); + const outsideDestination = screen.getByRole( 'button', { + name: 'Outside destination', + } ); - // Click on the body (ie. outside of the dropdown menu) - await user.click( document.body ); + await user.click( outsideDestination ); - await waitFor( () => - expect( screen.queryByRole( 'menu' ) ).not.toBeInTheDocument() - ); + await waitForClosedMenu(); + expect( outsideDestination ).toHaveFocus(); } ); it( 'should close when clicking on a menu item', async () => { @@ -262,7 +320,94 @@ describe( 'Menu', () => { ); } ); - it( 'should not close when clicking on a menu item when the `hideOnClick` prop is set to `false`', async () => { + it( 'should return focus to the root trigger after a modal opened by pointer closes', async () => { + render( ); + + const trigger = screen.getByRole( 'button', { + name: 'Open dropdown', + } ); + + await user.click( trigger ); + await waitForFocusedMenu(); + await user.click( + screen.getByRole( 'menuitem', { name: 'Open modal' } ) + ); + await waitForClosedMenu(); + expect( screen.getByRole( 'dialog' ) ).toBeInTheDocument(); + await user.click( + screen.getByRole( 'button', { name: 'Close modal' } ) + ); + + await waitFor( () => expect( trigger ).toHaveFocus() ); + } ); + + it.each( [ + [ 'Enter', '{Enter}' ], + [ 'Space', ' ' ], + ] )( 'should return focus after %s opens a modal', async ( _, key ) => { + render( ); + + const trigger = screen.getByRole( 'button', { + name: 'Open dropdown', + } ); + await openMenu( user ); + await user.keyboard( '{ArrowDown}' ); + await waitForFocusedMenuItem( 'Open modal' ); + + await user.keyboard( key ); + await waitForClosedMenu(); + expect( screen.getByRole( 'dialog' ) ).toBeInTheDocument(); + await user.click( + screen.getByRole( 'button', { name: 'Close modal' } ) + ); + + await waitFor( () => expect( trigger ).toHaveFocus() ); + } ); + + it( 'should return focus to the root trigger after a nested menu opens a modal', async () => { + render( ); + + const trigger = screen.getByRole( 'button', { + name: 'Open dropdown', + } ); + await openMenu( user ); + await user.keyboard( '{ArrowDown}' ); + await waitForFocusedMenuItem( 'Open submenu' ); + await user.keyboard( '{ArrowRight}' ); + await waitForFocusedMenuItem( 'Open modal' ); + await user.keyboard( '{Enter}' ); + await waitForClosedMenu(); + await user.click( + screen.getByRole( 'button', { name: 'Close modal' } ) + ); + + await waitFor( () => expect( trigger ).toHaveFocus() ); + } ); + + it( 'should keep body scroll locked while handing off to a modal', async () => { + render( ); + + expect( isBodyScrollLocked() ).toBe( false ); + await openMenu( user ); + expect( isBodyScrollLocked() ).toBe( true ); + + await user.click( + screen.getByRole( 'menuitem', { name: 'Open modal' } ) + ); + await waitForClosedMenu(); + expect( screen.getByRole( 'dialog' ) ).toBeInTheDocument(); + expect( isBodyScrollLocked() ).toBe( true ); + + await user.click( + screen.getByRole( 'button', { name: 'Close modal' } ) + ); + await waitFor( () => + expect( screen.queryByRole( 'dialog' ) ).not.toBeInTheDocument() + ); + await waitFor( () => expect( isBodyScrollLocked() ).toBe( false ) ); + } ); + + it( 'should close when `hideOnClick` is `false`', async () => { render( Open dropdown @@ -274,7 +419,74 @@ describe( 'Menu', () => { expect( screen.getByRole( 'menu' ) ).toBeVisible(); - // Clicking a menu item will close the menu + await user.click( screen.getByRole( 'menuitem' ) ); + + await waitForClosedMenu(); + } ); + + it( 'should close without invoking a `hideOnClick` callback', async () => { + const hideOnClick = jest.fn( () => false ); + render( + + Open dropdown + + + Menu item + + + + ); + + await user.click( screen.getByRole( 'menuitem' ) ); + + await waitForClosedMenu(); + expect( hideOnClick ).not.toHaveBeenCalled(); + } ); + + it( 'should retain focus moved outside by an item click handler', async () => { + let outsideDestination: HTMLButtonElement | null = null; + render( + <> + + + Open dropdown + + outsideDestination?.focus() } + > + Menu item + + + + + ); + + await user.click( screen.getByRole( 'menuitem' ) ); + + await waitForClosedMenu(); + expect( outsideDestination ).toHaveFocus(); + } ); + + it( 'should not close when item activation is prevented', async () => { + render( + + Open dropdown + + event.preventDefault() } + > + Menu item + + + + ); + await user.click( screen.getByRole( 'menuitem' ) ); expect( screen.getByRole( 'menu' ) ).toBeVisible(); @@ -434,8 +646,9 @@ describe( 'Menu', () => { ).toHaveFocus(); } ); - it( 'should check radio items and keep the menu open when clicking (controlled)', async () => { + it( 'should check radio items and close the menu when clicking (controlled)', async () => { const onRadioValueChangeSpy = jest.fn(); + const hideOnClickSpy = jest.fn( () => false ); const ControlledRadioGroup = () => { const [ radioValue, setRadioValue ] = useState( 'two' ); @@ -455,6 +668,7 @@ describe( 'Menu', () => { value="radio-one" checked={ radioValue === 'radio-one' } onChange={ onRadioChange } + hideOnClick={ hideOnClickSpy } > Radio item one @@ -463,6 +677,7 @@ describe( 'Menu', () => { value="radio-two" checked={ radioValue === 'radio-two' } onChange={ onRadioChange } + hideOnClick={ hideOnClickSpy } > Radio item two @@ -492,12 +707,13 @@ describe( 'Menu', () => { await user.click( screen.getByRole( 'menuitemradio', { name: 'Radio item one' } ) ); + await waitForClosedMenu(); expect( onRadioValueChangeSpy ).toHaveBeenCalledTimes( 1 ); expect( onRadioValueChangeSpy ).toHaveBeenLastCalledWith( 'radio-one' ); - // Make sure that first radio is checked + await openMenu( user ); expect( screen.getByRole( 'menuitemradio', { name: 'Radio item one' } ) ).toBeChecked(); @@ -505,25 +721,26 @@ describe( 'Menu', () => { screen.getByRole( 'menuitemradio', { name: 'Radio item two' } ) ).not.toBeChecked(); - // Click second radio item, make sure that the callback fires await user.click( screen.getByRole( 'menuitemradio', { name: 'Radio item two' } ) ); + await waitForClosedMenu(); expect( onRadioValueChangeSpy ).toHaveBeenCalledTimes( 2 ); expect( onRadioValueChangeSpy ).toHaveBeenLastCalledWith( 'radio-two' ); - // Make sure that second radio is selected + await openMenu( user ); expect( screen.getByRole( 'menuitemradio', { name: 'Radio item one' } ) ).not.toBeChecked(); expect( screen.getByRole( 'menuitemradio', { name: 'Radio item two' } ) ).toBeChecked(); + expect( hideOnClickSpy ).not.toHaveBeenCalled(); } ); - it( 'should check radio items and keep the menu open when clicking (uncontrolled)', async () => { + it( 'should check radio items and close the menu when clicking (uncontrolled)', async () => { const onRadioValueChangeSpy = jest.fn(); render( @@ -572,39 +789,16 @@ describe( 'Menu', () => { await user.click( screen.getByRole( 'menuitemradio', { name: 'Radio item one' } ) ); + await waitForClosedMenu(); expect( onRadioValueChangeSpy ).toHaveBeenCalledTimes( 1 ); expect( onRadioValueChangeSpy ).toHaveBeenLastCalledWith( 'radio-one' ); - - // Make sure that first radio is checked - expect( - screen.getByRole( 'menuitemradio', { name: 'Radio item one' } ) - ).toBeChecked(); - expect( - screen.getByRole( 'menuitemradio', { name: 'Radio item two' } ) - ).not.toBeChecked(); - - // Click second radio item, make sure that the callback fires - await user.click( - screen.getByRole( 'menuitemradio', { name: 'Radio item two' } ) - ); - expect( onRadioValueChangeSpy ).toHaveBeenCalledTimes( 2 ); - expect( onRadioValueChangeSpy ).toHaveBeenLastCalledWith( - 'radio-two' - ); - - // Make sure that second radio is selected - expect( - screen.getByRole( 'menuitemradio', { name: 'Radio item one' } ) - ).not.toBeChecked(); - expect( - screen.getByRole( 'menuitemradio', { name: 'Radio item two' } ) - ).toBeChecked(); } ); - it( 'should check checkbox items and keep the menu open when clicking (controlled)', async () => { + it( 'should check checkbox items and close the menu when clicking (controlled)', async () => { const onCheckboxValueChangeSpy = jest.fn(); + const hideOnClickSpy = jest.fn( () => false ); const ControlledRadioGroup = () => { const [ itemOneChecked, setItemOneChecked ] = @@ -620,6 +814,7 @@ describe( 'Menu', () => { name="item-one" value="item-one-value" checked={ itemOneChecked } + hideOnClick={ hideOnClickSpy } onChange={ ( e ) => { onCheckboxValueChangeSpy( e.target.name, @@ -636,6 +831,7 @@ describe( 'Menu', () => { name="item-two" value="item-two-value" checked={ itemTwoChecked } + hideOnClick={ hideOnClickSpy } onChange={ ( e ) => { onCheckboxValueChangeSpy( e.target.name, @@ -680,6 +876,7 @@ describe( 'Menu', () => { name: 'Checkbox item one', } ) ); + await waitForClosedMenu(); expect( onCheckboxValueChangeSpy ).toHaveBeenCalledTimes( 1 ); expect( onCheckboxValueChangeSpy ).toHaveBeenLastCalledWith( 'item-one', @@ -687,6 +884,7 @@ describe( 'Menu', () => { true ); + await openMenu( user ); // Make sure that first checkbox is checked expect( screen.getByRole( 'menuitemcheckbox', { @@ -700,6 +898,7 @@ describe( 'Menu', () => { name: 'Checkbox item two', } ) ); + await waitForClosedMenu(); expect( onCheckboxValueChangeSpy ).toHaveBeenCalledTimes( 2 ); expect( onCheckboxValueChangeSpy ).toHaveBeenLastCalledWith( 'item-two', @@ -707,6 +906,7 @@ describe( 'Menu', () => { true ); + await openMenu( user ); // Make sure that second checkbox is selected expect( screen.getByRole( 'menuitemcheckbox', { @@ -720,6 +920,7 @@ describe( 'Menu', () => { name: 'Checkbox item two', } ) ); + await waitForClosedMenu(); expect( onCheckboxValueChangeSpy ).toHaveBeenCalledTimes( 3 ); expect( onCheckboxValueChangeSpy ).toHaveBeenLastCalledWith( 'item-two', @@ -727,15 +928,17 @@ describe( 'Menu', () => { false ); + await openMenu( user ); // Make sure that second checkbox is unselected expect( screen.getByRole( 'menuitemcheckbox', { name: 'Checkbox item two', } ) ).not.toBeChecked(); + expect( hideOnClickSpy ).not.toHaveBeenCalled(); } ); - it( 'should check checkbox items and keep the menu open when clicking (uncontrolled)', async () => { + it( 'should check checkbox items and close the menu when clicking (uncontrolled)', async () => { const onCheckboxValueChangeSpy = jest.fn(); render( @@ -794,65 +997,19 @@ describe( 'Menu', () => { } ) ).toBeChecked(); - // Click first checkbox item, make sure that the callback fires - await user.click( - screen.getByRole( 'menuitemcheckbox', { - name: 'Checkbox item one', - } ) - ); - expect( onCheckboxValueChangeSpy ).toHaveBeenCalledTimes( 1 ); - expect( onCheckboxValueChangeSpy ).toHaveBeenLastCalledWith( - 'item-one', - 'item-one-value', - true - ); - - // Make sure that first checkbox is checked - expect( - screen.getByRole( 'menuitemcheckbox', { - name: 'Checkbox item one', - } ) - ).toBeChecked(); - - // Click second checkbox item, make sure that the callback fires + // Click the checked item, make sure that the callback fires await user.click( screen.getByRole( 'menuitemcheckbox', { name: 'Checkbox item two', } ) ); - expect( onCheckboxValueChangeSpy ).toHaveBeenCalledTimes( 2 ); + await waitForClosedMenu(); + expect( onCheckboxValueChangeSpy ).toHaveBeenCalledTimes( 1 ); expect( onCheckboxValueChangeSpy ).toHaveBeenLastCalledWith( 'item-two', 'item-two-value', false ); - - // Make sure that second checkbox is unchecked - expect( - screen.getByRole( 'menuitemcheckbox', { - name: 'Checkbox item two', - } ) - ).not.toBeChecked(); - - // Click second checkbox item, make sure that the callback fires - await user.click( - screen.getByRole( 'menuitemcheckbox', { - name: 'Checkbox item two', - } ) - ); - expect( onCheckboxValueChangeSpy ).toHaveBeenCalledTimes( 3 ); - expect( onCheckboxValueChangeSpy ).toHaveBeenLastCalledWith( - 'item-two', - 'item-two-value', - true - ); - - // Make sure that second checkbox is unselected - expect( - screen.getByRole( 'menuitemcheckbox', { - name: 'Checkbox item two', - } ) - ).toBeChecked(); } ); } ); @@ -920,7 +1077,7 @@ describe( 'Menu', () => { // The outer button can be focused by pressing tab. Doing so will cause // the Menu to close. await user.tab(); - expect( outerButton ).toBeVisible(); + expect( outerButton ).toHaveFocus(); await waitFor( () => expect( screen.queryByRole( 'menu' ) ).not.toBeInTheDocument() ); diff --git a/packages/components/src/menu/types.ts b/packages/components/src/menu/types.ts index ac208b6139392a..7fadb31f864a84 100644 --- a/packages/components/src/menu/types.ts +++ b/packages/components/src/menu/types.ts @@ -169,13 +169,12 @@ export interface ItemProps { */ suffix?: React.ReactNode; /** - * Determines if the menu should hide when this item is clicked. + * This prop has no effect and remains available for backward compatibility. + * Boolean and callback values are ignored, and callback values are not + * invoked. Every eligible `Menu.Item` activation closes the menu. * - * **Note**: This behavior isn't triggered if this menu item is rendered as a - * link and modifier keys are used to either open the link in a new tab or - * download it. - * - * @default true + * Ariakit's safeguards for disabled items, prevented events, modified links, + * downloads, and submenu triggers still apply. */ hideOnClick?: Ariakit.MenuItemProps[ 'hideOnClick' ]; /** @@ -197,7 +196,7 @@ export interface ItemProps { * The ariakit menu store. This prop is only meant for internal use. * @ignore */ - store?: Ariakit.MenuItemProps[ 'store' ]; + store?: Ariakit.MenuStore; } export interface CheckboxItemProps { @@ -212,13 +211,12 @@ export interface CheckboxItemProps { */ suffix?: React.ReactNode; /** - * Determines if the menu should hide when this item is clicked. - * - * **Note**: This behavior isn't triggered if this menu item is rendered as a - * link and modifier keys are used to either open the link in a new tab or - * download it. + * This prop has no effect and remains available for backward compatibility. + * Boolean and callback values are ignored, and callback values are not + * invoked. Every eligible `Menu.CheckboxItem` activation closes the menu. * - * @default false + * Ariakit's safeguards for disabled items, prevented events, modified links, + * downloads, and submenu triggers still apply. */ hideOnClick?: Ariakit.MenuItemCheckboxProps[ 'hideOnClick' ]; /** @@ -276,13 +274,12 @@ export interface RadioItemProps { */ suffix?: React.ReactNode; /** - * Determines if the menu should hide when this item is clicked. - * - * **Note**: This behavior isn't triggered if this menu item is rendered as a - * link and modifier keys are used to either open the link in a new tab or - * download it. + * This prop has no effect and remains available for backward compatibility. + * Boolean and callback values are ignored, and callback values are not + * invoked. Every eligible `Menu.RadioItem` activation closes the menu. * - * @default false + * Ariakit's safeguards for disabled items, prevented events, modified links, + * downloads, and submenu triggers still apply. */ hideOnClick?: Ariakit.MenuItemRadioProps[ 'hideOnClick' ]; /** diff --git a/packages/components/src/menu/use-menu-item-hide-on-click.ts b/packages/components/src/menu/use-menu-item-hide-on-click.ts new file mode 100644 index 00000000000000..b6e487b70ca3ff --- /dev/null +++ b/packages/components/src/menu/use-menu-item-hide-on-click.ts @@ -0,0 +1,58 @@ +/** + * External dependencies + */ +import type * as Ariakit from '@ariakit/react'; + +/** + * WordPress dependencies + */ +import { useCallback } from '@wordpress/element'; + +function focusDisclosureElement( disclosureElement: HTMLElement ) { + disclosureElement.focus(); + + if ( disclosureElement.ownerDocument.activeElement === disclosureElement ) { + return; + } + + // A modal Ariakit menu disables the document tree outside the menu. Modern + // browsers use `inert`; Ariakit's fallback replaces each element's `focus` + // method. Ariakit restores either mechanism as soon as the menu hides. + const inertAncestor = disclosureElement.closest< HTMLElement >( '[inert]' ); + if ( inertAncestor ) { + inertAncestor.inert = false; + } + + disclosureElement.ownerDocument.defaultView?.HTMLElement.prototype.focus.call( + disclosureElement + ); +} + +export function useMenuItemHideOnClick( store?: Ariakit.MenuStore ) { + return useCallback( () => { + if ( ! store ) { + return true; + } + + const menuElement = store.getState().contentElement; + const activeElement = menuElement?.ownerDocument.activeElement; + + if ( menuElement?.contains( activeElement ?? null ) ) { + let rootStore = store; + while ( rootStore.parent ) { + rootStore = rootStore.parent; + } + + const disclosureElement = rootStore.getState().disclosureElement; + + // Ariakit calls this immediately before it hides and unmounts the + // menu. Move focus first so an overlay opened by the item's onClick + // captures the root trigger as its focus return target. + if ( disclosureElement ) { + focusDisclosureElement( disclosureElement ); + } + } + + return true; + }, [ store ] ); +} From 4bb1eb31300dc0124ffbcc5ec3ba9feebc78ecfd Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Tue, 4 Aug 2026 18:29:03 +0200 Subject: [PATCH 2/8] Menu: Preserve default modal dismissal coverage --- packages/components/src/menu/test/index.tsx | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/components/src/menu/test/index.tsx b/packages/components/src/menu/test/index.tsx index 67381becedf036..bc85c6eed92bdb 100644 --- a/packages/components/src/menu/test/index.tsx +++ b/packages/components/src/menu/test/index.tsx @@ -276,7 +276,25 @@ describe( 'Menu', () => { ); } ); - it( 'should retain outside focus when outside interaction closes the menu', async () => { + it( 'should close when clicking outside of the content', async () => { + render( + + Open dropdown + + Menu item + + + ); + + expect( screen.getByRole( 'menu' ) ).toBeInTheDocument(); + + // Click on the body (ie. outside of the dropdown menu) + await user.click( document.body ); + + await waitForClosedMenu(); + } ); + + it( 'should retain outside focus when outside interaction closes a non-modal menu', async () => { render( <> From bb3953ecfb3d30e0789bc2c36c13752b25faca47 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Tue, 4 Aug 2026 23:05:59 +0200 Subject: [PATCH 3/8] Menu: Preserve hideOnClick behavior during focus handoff --- .../components/src/menu/checkbox-item.tsx | 6 +- packages/components/src/menu/item.tsx | 17 +- packages/components/src/menu/radio-item.tsx | 9 +- packages/components/src/menu/test/index.tsx | 204 +++++++++++++++--- packages/components/src/menu/types.ts | 35 +-- .../src/menu/use-menu-item-hide-on-click.ts | 56 +++-- 6 files changed, 252 insertions(+), 75 deletions(-) diff --git a/packages/components/src/menu/checkbox-item.tsx b/packages/components/src/menu/checkbox-item.tsx index f95073f4bff6ef..485ab8064102cd 100644 --- a/packages/components/src/menu/checkbox-item.tsx +++ b/packages/components/src/menu/checkbox-item.tsx @@ -11,12 +11,12 @@ export const CheckboxItem = forwardRef< HTMLDivElement, WordPressComponentProps< CheckboxItemProps, 'div', false > >( function CheckboxItem( - { suffix, children, disabled = false, ...props }, + { suffix, children, disabled = false, hideOnClick = false, ...props }, ref ) { const menuContext = useContext( Context ); const store = menuContext?.store; - const hideOnClick = useMenuItemHideOnClick( store ); + const computedHideOnClick = useMenuItemHideOnClick( store, hideOnClick ); if ( ! store ) { throw new Error( @@ -30,7 +30,7 @@ export const CheckboxItem = forwardRef< accessibleWhenDisabled disabled={ disabled } store={ store } - hideOnClick={ hideOnClick } + hideOnClick={ computedHideOnClick } > >( function Item( - { prefix, suffix, children, disabled = false, store, ...props }, + { + prefix, + suffix, + children, + disabled = false, + hideOnClick = true, + store, + ...props + }, ref ) { const menuContext = useContext( Context ); const computedStore = store ?? menuContext?.store; - const hideOnClick = useMenuItemHideOnClick( computedStore ); + const computedHideOnClick = useMenuItemHideOnClick( + computedStore, + hideOnClick + ); if ( ! menuContext?.store || ! computedStore ) { throw new Error( @@ -33,7 +44,7 @@ export const Item = forwardRef< accessibleWhenDisabled disabled={ disabled } store={ computedStore } - hideOnClick={ hideOnClick } + hideOnClick={ computedHideOnClick } > { prefix } diff --git a/packages/components/src/menu/radio-item.tsx b/packages/components/src/menu/radio-item.tsx index 1063fc31112e1b..2de8164db8158d 100644 --- a/packages/components/src/menu/radio-item.tsx +++ b/packages/components/src/menu/radio-item.tsx @@ -17,10 +17,13 @@ const radioCheck = ( export const RadioItem = forwardRef< HTMLDivElement, WordPressComponentProps< RadioItemProps, 'div', false > ->( function RadioItem( { suffix, children, disabled = false, ...props }, ref ) { +>( function RadioItem( + { suffix, children, disabled = false, hideOnClick = false, ...props }, + ref +) { const menuContext = useContext( Context ); const store = menuContext?.store; - const hideOnClick = useMenuItemHideOnClick( store ); + const computedHideOnClick = useMenuItemHideOnClick( store, hideOnClick ); if ( ! store ) { throw new Error( @@ -34,7 +37,7 @@ export const RadioItem = forwardRef< accessibleWhenDisabled disabled={ disabled } store={ store } - hideOnClick={ hideOnClick } + hideOnClick={ computedHideOnClick } > { await waitFor( () => expect( isBodyScrollLocked() ).toBe( false ) ); } ); - it( 'should close when `hideOnClick` is `false`', async () => { + it( 'should stay open when `hideOnClick` is `false`', async () => { render( Open dropdown @@ -439,10 +439,10 @@ describe( 'Menu', () => { await user.click( screen.getByRole( 'menuitem' ) ); - await waitForClosedMenu(); + expect( screen.getByRole( 'menu' ) ).toBeVisible(); } ); - it( 'should close without invoking a `hideOnClick` callback', async () => { + it( 'should use a `hideOnClick` callback to decide whether to close', async () => { const hideOnClick = jest.fn( () => false ); render( @@ -457,10 +457,67 @@ describe( 'Menu', () => { await user.click( screen.getByRole( 'menuitem' ) ); + expect( screen.getByRole( 'menu' ) ).toBeVisible(); + expect( hideOnClick ).toHaveBeenCalledTimes( 1 ); + } ); + + it( 'should close when a `hideOnClick` callback returns `true`', async () => { + const hideOnClick = jest.fn( () => true ); + render( + + Open dropdown + + + Menu item + + + + ); + + await user.click( screen.getByRole( 'menuitem' ) ); + await waitForClosedMenu(); - expect( hideOnClick ).not.toHaveBeenCalled(); + expect( hideOnClick ).toHaveBeenCalledTimes( 1 ); } ); + it.each( [ 'checkbox', 'radio' ] )( + 'should close a %s item when `hideOnClick` is `true`', + async ( itemType ) => { + render( + + Open dropdown + + { itemType === 'checkbox' ? ( + + Checkbox item + + ) : ( + + Radio item + + ) } + + + ); + + await user.click( screen.getByRole( `menuitem${ itemType }` ) ); + + await waitFor( () => + expect( + screen.queryByRole( 'menu' ) + ).not.toBeInTheDocument() + ); + } + ); + it( 'should retain focus moved outside by an item click handler', async () => { let outsideDestination: HTMLButtonElement | null = null; render( @@ -491,6 +548,39 @@ describe( 'Menu', () => { expect( outsideDestination ).toHaveFocus(); } ); + it( 'should retain focus moved outside by a `hideOnClick` callback', async () => { + let outsideDestination: HTMLButtonElement | null = null; + render( + <> + + + Open dropdown + + { + outsideDestination?.focus(); + return true; + } } + > + Menu item + + + + + ); + + await user.click( screen.getByRole( 'menuitem' ) ); + + await waitForClosedMenu(); + expect( outsideDestination ).toHaveFocus(); + } ); + it( 'should not close when item activation is prevented', async () => { render( @@ -664,9 +754,8 @@ describe( 'Menu', () => { ).toHaveFocus(); } ); - it( 'should check radio items and close the menu when clicking (controlled)', async () => { + it( 'should check radio items and keep the menu open when clicking (controlled)', async () => { const onRadioValueChangeSpy = jest.fn(); - const hideOnClickSpy = jest.fn( () => false ); const ControlledRadioGroup = () => { const [ radioValue, setRadioValue ] = useState( 'two' ); @@ -686,7 +775,6 @@ describe( 'Menu', () => { value="radio-one" checked={ radioValue === 'radio-one' } onChange={ onRadioChange } - hideOnClick={ hideOnClickSpy } > Radio item one @@ -695,7 +783,6 @@ describe( 'Menu', () => { value="radio-two" checked={ radioValue === 'radio-two' } onChange={ onRadioChange } - hideOnClick={ hideOnClickSpy } > Radio item two @@ -725,13 +812,12 @@ describe( 'Menu', () => { await user.click( screen.getByRole( 'menuitemradio', { name: 'Radio item one' } ) ); - await waitForClosedMenu(); expect( onRadioValueChangeSpy ).toHaveBeenCalledTimes( 1 ); expect( onRadioValueChangeSpy ).toHaveBeenLastCalledWith( 'radio-one' ); - await openMenu( user ); + // Make sure that first radio is checked expect( screen.getByRole( 'menuitemradio', { name: 'Radio item one' } ) ).toBeChecked(); @@ -742,23 +828,21 @@ describe( 'Menu', () => { await user.click( screen.getByRole( 'menuitemradio', { name: 'Radio item two' } ) ); - await waitForClosedMenu(); expect( onRadioValueChangeSpy ).toHaveBeenCalledTimes( 2 ); expect( onRadioValueChangeSpy ).toHaveBeenLastCalledWith( 'radio-two' ); - await openMenu( user ); + // Make sure that second radio is selected expect( screen.getByRole( 'menuitemradio', { name: 'Radio item one' } ) ).not.toBeChecked(); expect( screen.getByRole( 'menuitemradio', { name: 'Radio item two' } ) ).toBeChecked(); - expect( hideOnClickSpy ).not.toHaveBeenCalled(); } ); - it( 'should check radio items and close the menu when clicking (uncontrolled)', async () => { + it( 'should check radio items and keep the menu open when clicking (uncontrolled)', async () => { const onRadioValueChangeSpy = jest.fn(); render( @@ -807,16 +891,39 @@ describe( 'Menu', () => { await user.click( screen.getByRole( 'menuitemradio', { name: 'Radio item one' } ) ); - await waitForClosedMenu(); expect( onRadioValueChangeSpy ).toHaveBeenCalledTimes( 1 ); expect( onRadioValueChangeSpy ).toHaveBeenLastCalledWith( 'radio-one' ); + + // Make sure that first radio is checked + expect( + screen.getByRole( 'menuitemradio', { name: 'Radio item one' } ) + ).toBeChecked(); + expect( + screen.getByRole( 'menuitemradio', { name: 'Radio item two' } ) + ).not.toBeChecked(); + + // Click second radio item, make sure that the callback fires + await user.click( + screen.getByRole( 'menuitemradio', { name: 'Radio item two' } ) + ); + expect( onRadioValueChangeSpy ).toHaveBeenCalledTimes( 2 ); + expect( onRadioValueChangeSpy ).toHaveBeenLastCalledWith( + 'radio-two' + ); + + // Make sure that second radio is selected + expect( + screen.getByRole( 'menuitemradio', { name: 'Radio item one' } ) + ).not.toBeChecked(); + expect( + screen.getByRole( 'menuitemradio', { name: 'Radio item two' } ) + ).toBeChecked(); } ); - it( 'should check checkbox items and close the menu when clicking (controlled)', async () => { + it( 'should check checkbox items and keep the menu open when clicking (controlled)', async () => { const onCheckboxValueChangeSpy = jest.fn(); - const hideOnClickSpy = jest.fn( () => false ); const ControlledRadioGroup = () => { const [ itemOneChecked, setItemOneChecked ] = @@ -832,7 +939,6 @@ describe( 'Menu', () => { name="item-one" value="item-one-value" checked={ itemOneChecked } - hideOnClick={ hideOnClickSpy } onChange={ ( e ) => { onCheckboxValueChangeSpy( e.target.name, @@ -849,7 +955,6 @@ describe( 'Menu', () => { name="item-two" value="item-two-value" checked={ itemTwoChecked } - hideOnClick={ hideOnClickSpy } onChange={ ( e ) => { onCheckboxValueChangeSpy( e.target.name, @@ -894,7 +999,6 @@ describe( 'Menu', () => { name: 'Checkbox item one', } ) ); - await waitForClosedMenu(); expect( onCheckboxValueChangeSpy ).toHaveBeenCalledTimes( 1 ); expect( onCheckboxValueChangeSpy ).toHaveBeenLastCalledWith( 'item-one', @@ -902,7 +1006,6 @@ describe( 'Menu', () => { true ); - await openMenu( user ); // Make sure that first checkbox is checked expect( screen.getByRole( 'menuitemcheckbox', { @@ -916,7 +1019,6 @@ describe( 'Menu', () => { name: 'Checkbox item two', } ) ); - await waitForClosedMenu(); expect( onCheckboxValueChangeSpy ).toHaveBeenCalledTimes( 2 ); expect( onCheckboxValueChangeSpy ).toHaveBeenLastCalledWith( 'item-two', @@ -924,7 +1026,6 @@ describe( 'Menu', () => { true ); - await openMenu( user ); // Make sure that second checkbox is selected expect( screen.getByRole( 'menuitemcheckbox', { @@ -938,7 +1039,6 @@ describe( 'Menu', () => { name: 'Checkbox item two', } ) ); - await waitForClosedMenu(); expect( onCheckboxValueChangeSpy ).toHaveBeenCalledTimes( 3 ); expect( onCheckboxValueChangeSpy ).toHaveBeenLastCalledWith( 'item-two', @@ -946,17 +1046,15 @@ describe( 'Menu', () => { false ); - await openMenu( user ); // Make sure that second checkbox is unselected expect( screen.getByRole( 'menuitemcheckbox', { name: 'Checkbox item two', } ) ).not.toBeChecked(); - expect( hideOnClickSpy ).not.toHaveBeenCalled(); } ); - it( 'should check checkbox items and close the menu when clicking (uncontrolled)', async () => { + it( 'should check checkbox items and keep the menu open when clicking (uncontrolled)', async () => { const onCheckboxValueChangeSpy = jest.fn(); render( @@ -1015,19 +1113,65 @@ describe( 'Menu', () => { } ) ).toBeChecked(); - // Click the checked item, make sure that the callback fires + // Click first checkbox item, make sure that the callback fires await user.click( screen.getByRole( 'menuitemcheckbox', { - name: 'Checkbox item two', + name: 'Checkbox item one', } ) ); - await waitForClosedMenu(); expect( onCheckboxValueChangeSpy ).toHaveBeenCalledTimes( 1 ); + expect( onCheckboxValueChangeSpy ).toHaveBeenLastCalledWith( + 'item-one', + 'item-one-value', + true + ); + + // Make sure that first checkbox is checked + expect( + screen.getByRole( 'menuitemcheckbox', { + name: 'Checkbox item one', + } ) + ).toBeChecked(); + + // Click second checkbox item, make sure that the callback fires + await user.click( + screen.getByRole( 'menuitemcheckbox', { + name: 'Checkbox item two', + } ) + ); + expect( onCheckboxValueChangeSpy ).toHaveBeenCalledTimes( 2 ); expect( onCheckboxValueChangeSpy ).toHaveBeenLastCalledWith( 'item-two', 'item-two-value', false ); + + // Make sure that second checkbox is unchecked + expect( + screen.getByRole( 'menuitemcheckbox', { + name: 'Checkbox item two', + } ) + ).not.toBeChecked(); + + // Click second checkbox item, make sure that the callback fires + await user.click( + screen.getByRole( 'menuitemcheckbox', { + name: 'Checkbox item two', + } ) + ); + expect( onCheckboxValueChangeSpy ).toHaveBeenCalledTimes( 3 ); + expect( onCheckboxValueChangeSpy ).toHaveBeenLastCalledWith( + 'item-two', + 'item-two-value', + true + ); + + // Make sure that second checkbox is selected + expect( + screen.getByRole( 'menuitemcheckbox', { + name: 'Checkbox item two', + } ) + ).toBeChecked(); } ); } ); diff --git a/packages/components/src/menu/types.ts b/packages/components/src/menu/types.ts index 7fadb31f864a84..ac208b6139392a 100644 --- a/packages/components/src/menu/types.ts +++ b/packages/components/src/menu/types.ts @@ -169,12 +169,13 @@ export interface ItemProps { */ suffix?: React.ReactNode; /** - * This prop has no effect and remains available for backward compatibility. - * Boolean and callback values are ignored, and callback values are not - * invoked. Every eligible `Menu.Item` activation closes the menu. + * Determines if the menu should hide when this item is clicked. * - * Ariakit's safeguards for disabled items, prevented events, modified links, - * downloads, and submenu triggers still apply. + * **Note**: This behavior isn't triggered if this menu item is rendered as a + * link and modifier keys are used to either open the link in a new tab or + * download it. + * + * @default true */ hideOnClick?: Ariakit.MenuItemProps[ 'hideOnClick' ]; /** @@ -196,7 +197,7 @@ export interface ItemProps { * The ariakit menu store. This prop is only meant for internal use. * @ignore */ - store?: Ariakit.MenuStore; + store?: Ariakit.MenuItemProps[ 'store' ]; } export interface CheckboxItemProps { @@ -211,12 +212,13 @@ export interface CheckboxItemProps { */ suffix?: React.ReactNode; /** - * This prop has no effect and remains available for backward compatibility. - * Boolean and callback values are ignored, and callback values are not - * invoked. Every eligible `Menu.CheckboxItem` activation closes the menu. + * Determines if the menu should hide when this item is clicked. + * + * **Note**: This behavior isn't triggered if this menu item is rendered as a + * link and modifier keys are used to either open the link in a new tab or + * download it. * - * Ariakit's safeguards for disabled items, prevented events, modified links, - * downloads, and submenu triggers still apply. + * @default false */ hideOnClick?: Ariakit.MenuItemCheckboxProps[ 'hideOnClick' ]; /** @@ -274,12 +276,13 @@ export interface RadioItemProps { */ suffix?: React.ReactNode; /** - * This prop has no effect and remains available for backward compatibility. - * Boolean and callback values are ignored, and callback values are not - * invoked. Every eligible `Menu.RadioItem` activation closes the menu. + * Determines if the menu should hide when this item is clicked. + * + * **Note**: This behavior isn't triggered if this menu item is rendered as a + * link and modifier keys are used to either open the link in a new tab or + * download it. * - * Ariakit's safeguards for disabled items, prevented events, modified links, - * downloads, and submenu triggers still apply. + * @default false */ hideOnClick?: Ariakit.MenuItemRadioProps[ 'hideOnClick' ]; /** diff --git a/packages/components/src/menu/use-menu-item-hide-on-click.ts b/packages/components/src/menu/use-menu-item-hide-on-click.ts index b6e487b70ca3ff..981ac5ce89bf7d 100644 --- a/packages/components/src/menu/use-menu-item-hide-on-click.ts +++ b/packages/components/src/menu/use-menu-item-hide-on-click.ts @@ -28,31 +28,47 @@ function focusDisclosureElement( disclosureElement: HTMLElement ) { ); } -export function useMenuItemHideOnClick( store?: Ariakit.MenuStore ) { - return useCallback( () => { - if ( ! store ) { - return true; - } +export function useMenuItemHideOnClick( + store: Ariakit.MenuItemProps[ 'store' ], + hideOnClick: Ariakit.MenuItemProps[ 'hideOnClick' ] +) { + return useCallback( + ( event: React.MouseEvent< HTMLElement > ) => { + const shouldHide = + typeof hideOnClick === 'function' + ? hideOnClick( event ) + : hideOnClick; - const menuElement = store.getState().contentElement; - const activeElement = menuElement?.ownerDocument.activeElement; + if ( ! shouldHide ) { + return false; + } - if ( menuElement?.contains( activeElement ?? null ) ) { - let rootStore = store; - while ( rootStore.parent ) { - rootStore = rootStore.parent; + if ( ! store || ! ( 'hideAll' in store ) ) { + return true; } - const disclosureElement = rootStore.getState().disclosureElement; + const menuElement = store.getState().contentElement; + const activeElement = menuElement?.ownerDocument.activeElement; + + if ( menuElement?.contains( activeElement ?? null ) ) { + let rootStore = store; + while ( rootStore.parent ) { + rootStore = rootStore.parent; + } - // Ariakit calls this immediately before it hides and unmounts the - // menu. Move focus first so an overlay opened by the item's onClick - // captures the root trigger as its focus return target. - if ( disclosureElement ) { - focusDisclosureElement( disclosureElement ); + const disclosureElement = + rootStore.getState().disclosureElement; + + // Ariakit calls this immediately before it hides and unmounts the + // menu. Move focus first so an overlay opened by the item's onClick + // captures the root trigger as its focus return target. + if ( disclosureElement ) { + focusDisclosureElement( disclosureElement ); + } } - } - return true; - }, [ store ] ); + return true; + }, + [ hideOnClick, store ] + ); } From a1735d44ebd2bfe44c69b6a30cf0486f1d6d6a90 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Tue, 4 Aug 2026 23:09:53 +0200 Subject: [PATCH 4/8] Menu: Add focus handoff changelog --- packages/components/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index bdd73f22921ae0..cfff52258a1c4a 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -19,6 +19,7 @@ ### Bug Fixes +- `Menu`: Return focus to the root trigger after closing a legacy `Modal` opened from a menu item, while preserving the menu-to-Modal scroll-lock handoff ([#81164](https://github.com/WordPress/gutenberg/pull/81164)). - `Button`: Suppress the browser focus ring when keyboard-focused and pressed ([#81113](https://github.com/WordPress/gutenberg/pull/81113)). ### TypeScript From c0c2fa2d1eb701f6b7ebacc08fb9ad85aa8dde69 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Tue, 4 Aug 2026 23:18:10 +0200 Subject: [PATCH 5/8] Menu: Cover closing and keep-open Modal stories --- packages/components/src/menu/stories/index.story.tsx | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/components/src/menu/stories/index.story.tsx b/packages/components/src/menu/stories/index.story.tsx index fc46a30c5e08c8..34f2d421549653 100644 --- a/packages/components/src/menu/stories/index.story.tsx +++ b/packages/components/src/menu/stories/index.story.tsx @@ -415,10 +415,7 @@ export const WithModals: StoryObj< typeof Menu > = { Open menu - setOuterModalOpen( true ) } - hideOnClick={ false } - > + setOuterModalOpen( true ) }> Open outer modal Date: Mon, 10 Aug 2026 13:30:38 +0200 Subject: [PATCH 6/8] Menu: Simplify Modal regression coverage --- .../src/menu/stories/index.story.tsx | 52 +++---------------- packages/components/src/menu/test/index.tsx | 7 +-- tools/eslint/suppressions.json | 5 -- 3 files changed, 10 insertions(+), 54 deletions(-) diff --git a/packages/components/src/menu/stories/index.story.tsx b/packages/components/src/menu/stories/index.story.tsx index 34f2d421549653..7ec90cb19111da 100644 --- a/packages/components/src/menu/stories/index.story.tsx +++ b/packages/components/src/menu/stories/index.story.tsx @@ -1,9 +1,7 @@ import type { StoryObj, Meta } from '@storybook/react-vite'; -import { css } from '@emotion/react'; import { fn } from 'storybook/test'; import { customLink, formatCapitalize } from '@wordpress/icons'; import { useState, useMemo, useContext } from '@wordpress/element'; -import { useCx } from '../../utils'; import { Menu } from '..'; import Icon from '../../icon'; import Button from '../../button'; @@ -390,19 +388,9 @@ export const WithRadios: StoryObj< typeof Menu > = { }, }; -const modalOnTopOfMenuPopover = css` - && { - z-index: 1000000; - } -`; - -export const WithModals: StoryObj< typeof Menu > = { - render: function WithModals( props: Props ) { - const [ isOuterModalOpen, setOuterModalOpen ] = useState( false ); - const [ isInnerModalOpen, setInnerModalOpen ] = useState( false ); - - const cx = useCx(); - const modalOverlayClassName = cx( modalOnTopOfMenuPopover ); +export const WithModal: StoryObj< typeof Menu > = { + render: function WithModal( props: Props ) { + const [ isModalOpen, setModalOpen ] = useState( false ); return ( <> @@ -415,39 +403,15 @@ export const WithModals: StoryObj< typeof Menu > = { Open menu - setOuterModalOpen( true ) }> - Open outer modal - - setInnerModalOpen( true ) } - hideOnClick={ false } - > - Open inner modal + setModalOpen( true ) }> + Open modal - { isInnerModalOpen && ( - - setInnerModalOpen( false ) - } - overlayClassName={ modalOverlayClassName } - > - Modal's contents - - - ) } - { isOuterModalOpen && ( - setOuterModalOpen( false ) } - overlayClassName={ modalOverlayClassName } - > + { isModalOpen && ( + setModalOpen( false ) }> Modal's contents - diff --git a/packages/components/src/menu/test/index.tsx b/packages/components/src/menu/test/index.tsx index 09a801ef863822..96d2c8dc124298 100644 --- a/packages/components/src/menu/test/index.tsx +++ b/packages/components/src/menu/test/index.tsx @@ -359,10 +359,7 @@ describe( 'Menu', () => { await waitFor( () => expect( trigger ).toHaveFocus() ); } ); - it.each( [ - [ 'Enter', '{Enter}' ], - [ 'Space', ' ' ], - ] )( 'should return focus after %s opens a modal', async ( _, key ) => { + it( 'should return focus after Enter opens a modal', async () => { render( ); const trigger = screen.getByRole( 'button', { @@ -372,7 +369,7 @@ describe( 'Menu', () => { await user.keyboard( '{ArrowDown}' ); await waitForFocusedMenuItem( 'Open modal' ); - await user.keyboard( key ); + await user.keyboard( '{Enter}' ); await waitForClosedMenu(); expect( screen.getByRole( 'dialog' ) ).toBeInTheDocument(); await user.click( diff --git a/tools/eslint/suppressions.json b/tools/eslint/suppressions.json index fde40e58c2c6c3..071393a4beb49b 100644 --- a/tools/eslint/suppressions.json +++ b/tools/eslint/suppressions.json @@ -761,11 +761,6 @@ "count": 3 } }, - "packages/components/src/menu/stories/index.story.tsx": { - "no-restricted-imports": { - "count": 1 - } - }, "packages/components/src/menu/styles.ts": { "no-restricted-imports": { "count": 2 From 44b09ab64b82eee90f27e1a041d5b43824e995eb Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Mon, 10 Aug 2026 13:33:34 +0200 Subject: [PATCH 7/8] Modal: Add default scroll lock styles --- packages/components/CHANGELOG.md | 1 + packages/components/src/menu/test/index.tsx | 3 +++ packages/components/src/modal/README.md | 1 + packages/components/src/modal/style.scss | 4 ++++ packages/components/src/modal/types.ts | 2 ++ 5 files changed, 11 insertions(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index cfff52258a1c4a..3493ff4d8aa809 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -19,6 +19,7 @@ ### Bug Fixes +- `Modal`: Prevent background page scrolling without relying on WordPress global styles when using the default `bodyOpenClassName` ([#81164](https://github.com/WordPress/gutenberg/pull/81164)). - `Menu`: Return focus to the root trigger after closing a legacy `Modal` opened from a menu item, while preserving the menu-to-Modal scroll-lock handoff ([#81164](https://github.com/WordPress/gutenberg/pull/81164)). - `Button`: Suppress the browser focus ring when keyboard-focused and pressed ([#81113](https://github.com/WordPress/gutenberg/pull/81113)). diff --git a/packages/components/src/menu/test/index.tsx b/packages/components/src/menu/test/index.tsx index 96d2c8dc124298..28e0df27cf7e57 100644 --- a/packages/components/src/menu/test/index.tsx +++ b/packages/components/src/menu/test/index.tsx @@ -22,6 +22,9 @@ const openMenu = async ( user: ReturnType< typeof userEvent.setup > ) => { await waitForFocusedMenu(); }; +// TODO: Add browser coverage for the computed overflow once component stories +// use interaction tests. Jest mocks stylesheet imports, so this only verifies +// the handoff between Ariakit's inline lock and Modal's body class. const isBodyScrollLocked = () => document.body.style.overflow === 'hidden' || document.body.classList.contains( 'modal-open' ); diff --git a/packages/components/src/modal/README.md b/packages/components/src/modal/README.md index c7c0c8ec6d6348..0b3009b1bcfb65 100644 --- a/packages/components/src/modal/README.md +++ b/packages/components/src/modal/README.md @@ -163,6 +163,7 @@ Titles are required for accessibility reasons, see `contentLabel` and `title` fo #### `bodyOpenClassName`: `string` Class name added to the body element when the modal is open. +If you use a custom class name, its styles must set `overflow: hidden` to preserve the Modal's scroll lock. - Required: No - Default: `modal-open` diff --git a/packages/components/src/modal/style.scss b/packages/components/src/modal/style.scss index 5e705069f221c1..5bec63365e86a8 100644 --- a/packages/components/src/modal/style.scss +++ b/packages/components/src/modal/style.scss @@ -5,6 +5,10 @@ @use "@wordpress/base-styles/z-index" as *; @use "../utils/theme-variables" as *; +body.modal-open { + overflow: hidden; +} + // The scrim behind the modal window. .components-modal__screen-overlay { position: fixed; diff --git a/packages/components/src/modal/types.ts b/packages/components/src/modal/types.ts index 7f7aa612128ee4..76f9869505e079 100644 --- a/packages/components/src/modal/types.ts +++ b/packages/components/src/modal/types.ts @@ -20,6 +20,8 @@ export type ModalProps = { }; /** * Class name added to the body element when the modal is open. + * If you use a custom class name, its styles must set `overflow: hidden` + * to preserve the Modal's scroll lock. * * @default 'modal-open' */ From 3eb5247ec7d8a3efa3aaa2b6e2b1d12de739fc04 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Mon, 10 Aug 2026 13:36:26 +0200 Subject: [PATCH 8/8] Menu: Follow current import conventions --- .../components/src/menu/use-menu-item-hide-on-click.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/packages/components/src/menu/use-menu-item-hide-on-click.ts b/packages/components/src/menu/use-menu-item-hide-on-click.ts index 981ac5ce89bf7d..f648e1940e0cb9 100644 --- a/packages/components/src/menu/use-menu-item-hide-on-click.ts +++ b/packages/components/src/menu/use-menu-item-hide-on-click.ts @@ -1,11 +1,4 @@ -/** - * External dependencies - */ import type * as Ariakit from '@ariakit/react'; - -/** - * WordPress dependencies - */ import { useCallback } from '@wordpress/element'; function focusDisclosureElement( disclosureElement: HTMLElement ) {