diff --git a/src/components/PopoverMenu/index.tsx b/src/components/PopoverMenu/index.tsx index 5366b668735e..8dbbdcc44247 100644 --- a/src/components/PopoverMenu/index.tsx +++ b/src/components/PopoverMenu/index.tsx @@ -34,6 +34,7 @@ import type {AnchorPosition} from '@src/styles'; import type {PendingAction} from '@src/types/onyx/OnyxCommon'; import type AnchorAlignment from '@src/types/utils/AnchorAlignment'; import type IconAsset from '@src/types/utils/IconAsset'; +import useCloseOnEscape from './v2/content/useCloseOnEscape'; type PopoverMenuItem = MenuItemProps & { /** Text label */ @@ -540,6 +541,14 @@ function BasePopoverMenu({ // can cause the parent view to scroll when the space bar is pressed. useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.SPACE, keyboardShortcutSpaceCallback, {isActive: isWeb && isVisible, shouldPreventDefault: false}); + const handleClose = useCallback(() => { + setCurrentMenuItems(menuItems); + setEnteredSubMenuIndexes(CONST.EMPTY_ARRAY); + onClose(); + }, [menuItems, onClose]); + + useCloseOnEscape(isVisible, handleClose); + const handleModalHide = () => { onModalHide?.(); setHasKeyBeenPressed(false); @@ -649,11 +658,7 @@ function BasePopoverMenu({ anchorPosition={anchorPosition} anchorRef={anchorRef} anchorAlignment={anchorAlignment} - onClose={() => { - setCurrentMenuItems(menuItems); - setEnteredSubMenuIndexes(CONST.EMPTY_ARRAY); - onClose(); - }} + onClose={handleClose} isVisible={isVisible} onModalHide={handleModalHide} onModalShow={onModalShow} diff --git a/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts b/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts new file mode 100644 index 000000000000..be97de7bf014 --- /dev/null +++ b/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts @@ -0,0 +1,21 @@ +import {useEffect} from 'react'; +import claimEscapeKeyDown from '@libs/claimEscapeKeyDown'; +import suppressNextEscapeKeyUp from '@libs/suppressNextEscapeKeyUp'; + +/** + * Closes the popover on Escape. Web-only via `claimEscapeKeyDown` (no-op on native); + * the parent-modal Escape race we're defending against is web-specific. + */ +function useCloseOnEscape(isVisible: boolean, close: () => void): void { + useEffect(() => { + if (!isVisible) { + return undefined; + } + return claimEscapeKeyDown(() => { + suppressNextEscapeKeyUp(); + close(); + }); + }, [isVisible, close]); +} + +export default useCloseOnEscape; diff --git a/src/components/PopoverMenu/v2/content/useContentController.ts b/src/components/PopoverMenu/v2/content/useContentController.ts index b989320d3a19..c499585f2c2e 100644 --- a/src/components/PopoverMenu/v2/content/useContentController.ts +++ b/src/components/PopoverMenu/v2/content/useContentController.ts @@ -1,5 +1,6 @@ import {useRootActions, useRootVisibility} from '@components/PopoverMenu/v2/root/RootContext'; import type {ContentClose, ContentFocus, ContentItemActions, ContentNavigation, ContentSubActions} from './ContentContext'; +import useCloseOnEscape from './useCloseOnEscape'; import useCloseOnModalCover from './useCloseOnModalCover'; import useCloseOnScreenBlur from './useCloseOnScreenBlur'; import useFocusableRegistry from './useFocusableRegistry'; @@ -26,6 +27,7 @@ function useContentController(componentName: string): { useCloseOnModalCover(isVisible, close); useCloseOnScreenBlur(close); + useCloseOnEscape(isVisible, close); return { navigation: {currentSubID: subNav.currentSubID, isAncestorOfCurrent: subNav.isAncestorOfCurrent}, diff --git a/src/libs/claimEscapeKeyDown/index.native.ts b/src/libs/claimEscapeKeyDown/index.native.ts new file mode 100644 index 000000000000..d08e60b7f9ce --- /dev/null +++ b/src/libs/claimEscapeKeyDown/index.native.ts @@ -0,0 +1,5 @@ +function claimEscapeKeyDown(): () => void { + return () => {}; +} + +export default claimEscapeKeyDown; diff --git a/src/libs/claimEscapeKeyDown/index.ts b/src/libs/claimEscapeKeyDown/index.ts new file mode 100644 index 000000000000..b74773a0efb7 --- /dev/null +++ b/src/libs/claimEscapeKeyDown/index.ts @@ -0,0 +1,18 @@ +/** + * `window` capture keydown for Escape — runs before `react-native-key-command`'s document-capture + * listener, so the shortcut stack never walks. Sidesteps the "parent re-subscribes and jumps ahead" + * race that the stack can't defend against. + */ +function claimEscapeKeyDown(handler: () => void): () => void { + const onKeyDown = (event: KeyboardEvent) => { + if (event.key !== 'Escape') { + return; + } + event.stopImmediatePropagation(); + handler(); + }; + window.addEventListener('keydown', onKeyDown, true); + return () => window.removeEventListener('keydown', onKeyDown, true); +} + +export default claimEscapeKeyDown; diff --git a/src/libs/suppressNextEscapeKeyUp/index.native.ts b/src/libs/suppressNextEscapeKeyUp/index.native.ts new file mode 100644 index 000000000000..8daf73c665ff --- /dev/null +++ b/src/libs/suppressNextEscapeKeyUp/index.native.ts @@ -0,0 +1,5 @@ +function suppressNextEscapeKeyUp(): () => void { + return () => {}; +} + +export default suppressNextEscapeKeyUp; diff --git a/src/libs/suppressNextEscapeKeyUp/index.ts b/src/libs/suppressNextEscapeKeyUp/index.ts new file mode 100644 index 000000000000..584b92b8ccf9 --- /dev/null +++ b/src/libs/suppressNextEscapeKeyUp/index.ts @@ -0,0 +1,36 @@ +/** + * Swallows the next Escape `keyup`. + * - `window` (not `document`) capture: runs before `PopoverProvider`'s keyup which would otherwise preempt and leak us. + * - Singleton: each install evicts any pending listener. + */ +let pending: ((event: KeyboardEvent) => void) | null = null; + +function suppressNextEscapeKeyUp(): () => void { + if (pending) { + window.removeEventListener('keyup', pending, true); + pending = null; + } + + const onKeyUp = (event: KeyboardEvent) => { + if (event.key !== 'Escape') { + return; + } + event.stopImmediatePropagation(); + window.removeEventListener('keyup', onKeyUp, true); + if (pending === onKeyUp) { + pending = null; + } + }; + + pending = onKeyUp; + window.addEventListener('keyup', onKeyUp, true); + + return () => { + window.removeEventListener('keyup', onKeyUp, true); + if (pending === onKeyUp) { + pending = null; + } + }; +} + +export default suppressNextEscapeKeyUp; diff --git a/tests/unit/PopoverMenuV2Test.tsx b/tests/unit/PopoverMenuV2Test.tsx index 7c97bed8e66c..75153688218a 100644 --- a/tests/unit/PopoverMenuV2Test.tsx +++ b/tests/unit/PopoverMenuV2Test.tsx @@ -112,6 +112,27 @@ function pressShortcut(shortcutKey: string): void { act(() => entry.callback()); } +// useCloseOnEscape installs via claimEscapeKeyDown — capture the handler so tests can fire it. +let mockActiveEscapeHandler: (() => void) | null = null; +jest.mock('@libs/claimEscapeKeyDown', () => ({ + __esModule: true, + default: (handler: () => void) => { + mockActiveEscapeHandler = handler; + return () => { + if (mockActiveEscapeHandler === handler) { + mockActiveEscapeHandler = null; + } + }; + }, +})); + +function pressEscape(): void { + if (!mockActiveEscapeHandler) { + return; + } + act(() => mockActiveEscapeHandler?.()); +} + const mockNavigationState: {blurListeners: Set<() => void>} = { blurListeners: new Set(), }; @@ -394,6 +415,67 @@ describe('PopoverMenu V2', () => { ); expect(onOpenChange).not.toHaveBeenCalledWith(false); }); + + it('closes when Escape is pressed while open', () => { + const onOpenChange = jest.fn(); + render( + + + {}} + /> + + , + ); + onOpenChange.mockClear(); + pressEscape(); + expect(onOpenChange).toHaveBeenCalledWith(false); + }); + + it('closes the whole popover on Escape even when a sub is the active level', () => { + const onOpenChange = jest.fn(); + render( + + + + + + + {}} + /> + + + + , + ); + press('More'); + onOpenChange.mockClear(); + pressEscape(); + expect(onOpenChange).toHaveBeenCalledWith(false); + }); + + it('Escape listener is not installed while the popover is closed', () => { + render( + + + {}} + /> + + , + ); + expect(mockActiveEscapeHandler).toBeNull(); + }); }); describe('Trigger', () => { diff --git a/tests/unit/claimEscapeKeyDownTest.ts b/tests/unit/claimEscapeKeyDownTest.ts new file mode 100644 index 000000000000..15fb989ddec3 --- /dev/null +++ b/tests/unit/claimEscapeKeyDownTest.ts @@ -0,0 +1,62 @@ +// Bypass haste: react-native preset defaults to iOS → would resolve to the no-op index.native.ts. +// eslint-disable-next-line import/extensions +const claimEscapeKeyDown = (require('../../src/libs/claimEscapeKeyDown/index.ts') as {default: (handler: () => void) => () => void}).default; + +function dispatchKeyDown(key: string): boolean { + let reached = false; + const sentinel = () => { + reached = true; + }; + document.addEventListener('keydown', sentinel, true); + document.body.dispatchEvent(new KeyboardEvent('keydown', {key, bubbles: true})); + document.removeEventListener('keydown', sentinel, true); + return reached; +} + +describe('claimEscapeKeyDown', () => { + it('calls the handler on Escape and stops document-level propagation', () => { + const handler = jest.fn(); + const cleanup = claimEscapeKeyDown(handler); + try { + expect(dispatchKeyDown('Escape')).toBe(false); + expect(handler).toHaveBeenCalledTimes(1); + } finally { + cleanup(); + } + }); + + it('ignores non-Escape keys', () => { + const handler = jest.fn(); + const cleanup = claimEscapeKeyDown(handler); + try { + expect(dispatchKeyDown('Enter')).toBe(true); + expect(handler).not.toHaveBeenCalled(); + } finally { + cleanup(); + } + }); + + it('cleanup detaches the listener', () => { + const handler = jest.fn(); + const cleanup = claimEscapeKeyDown(handler); + cleanup(); + expect(dispatchKeyDown('Escape')).toBe(true); + expect(handler).not.toHaveBeenCalled(); + }); + + it('runs ahead of a document-capture handler — preempts the shortcut-stack analog', () => { + // Simulate react-native-key-command's document-capture keydown listener (which would walk the shortcut stack). + const stackWalker = jest.fn(); + document.addEventListener('keydown', stackWalker, true); + const handler = jest.fn(); + const cleanup = claimEscapeKeyDown(handler); + try { + dispatchKeyDown('Escape'); + expect(handler).toHaveBeenCalledTimes(1); + expect(stackWalker).not.toHaveBeenCalled(); + } finally { + cleanup(); + document.removeEventListener('keydown', stackWalker, true); + } + }); +}); diff --git a/tests/unit/suppressNextEscapeKeyUpTest.ts b/tests/unit/suppressNextEscapeKeyUpTest.ts new file mode 100644 index 000000000000..f7a5292a29b3 --- /dev/null +++ b/tests/unit/suppressNextEscapeKeyUpTest.ts @@ -0,0 +1,71 @@ +// Bypass haste: react-native preset defaults to iOS → would resolve to the no-op index.native.ts. +// eslint-disable-next-line import/extensions +const suppressNextEscapeKeyUp = (require('../../src/libs/suppressNextEscapeKeyUp/index.ts') as {default: () => () => void}).default; + +function dispatchAndCheckPropagation(key: string): boolean { + let reached = false; + const sentinel = () => { + reached = true; + }; + document.addEventListener('keyup', sentinel, true); + // body+bubbles instead of dispatching on document: jsdom skips at-target capture listeners. + document.body.dispatchEvent(new KeyboardEvent('keyup', {key, bubbles: true})); + document.removeEventListener('keyup', sentinel, true); + return reached; +} + +afterEach(() => { + // Drain any pending suppressor — the helper has module-scope state. + dispatchAndCheckPropagation('Escape'); +}); + +describe('suppressNextEscapeKeyUp', () => { + it('stops propagation of the next Escape keyup', () => { + suppressNextEscapeKeyUp(); + expect(dispatchAndCheckPropagation('Escape')).toBe(false); + }); + + it('ignores non-Escape keys', () => { + suppressNextEscapeKeyUp(); + expect(dispatchAndCheckPropagation('Enter')).toBe(true); + }); + + it('removes itself after firing — the next Escape is not suppressed', () => { + suppressNextEscapeKeyUp(); + dispatchAndCheckPropagation('Escape'); + expect(dispatchAndCheckPropagation('Escape')).toBe(true); + }); + + it('singleton: a second install evicts the first; only one Escape is suppressed across two installs', () => { + suppressNextEscapeKeyUp(); + suppressNextEscapeKeyUp(); + expect(dispatchAndCheckPropagation('Escape')).toBe(false); + expect(dispatchAndCheckPropagation('Escape')).toBe(true); + }); + + it('cleanup return defuses a pending suppressor', () => { + const cleanup = suppressNextEscapeKeyUp(); + cleanup(); + expect(dispatchAndCheckPropagation('Escape')).toBe(true); + }); + + it('survives a document-capture preempt — no leak when an earlier document listener stops propagation', () => { + // PopoverProvider analog: stops Escape keyup on document capture, registered before our suppressor. + let preemptActive = true; + const preempt = (e: KeyboardEvent) => { + if (!preemptActive || e.key !== 'Escape') { + return; + } + e.stopImmediatePropagation(); + }; + document.addEventListener('keyup', preempt, true); + try { + suppressNextEscapeKeyUp(); + dispatchAndCheckPropagation('Escape'); + preemptActive = false; + expect(dispatchAndCheckPropagation('Escape')).toBe(true); + } finally { + document.removeEventListener('keyup', preempt, true); + } + }); +});