From 0c4c289c9f11338ac97632b18a6a87ff421e6ada Mon Sep 17 00:00:00 2001 From: TaduJR Date: Mon, 1 Jun 2026 09:43:13 +0300 Subject: [PATCH 1/7] =?UTF-8?q?fix:=20claim=20Escape=20in=20PopoverMenu=20?= =?UTF-8?q?(v1)=20+=20v2=20=E2=80=94=20prevent=20parent=20modal=20dismissa?= =?UTF-8?q?l?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/components/PopoverMenu/index.tsx | 10 +++ .../v2/content/useCloseOnEscape.ts | 16 +++++ .../v2/content/useContentController.ts | 2 + .../suppressNextEscapeKeyUp/index.native.ts | 5 ++ src/libs/suppressNextEscapeKeyUp/index.ts | 18 ++++++ tests/unit/PopoverMenuV2Test.tsx | 61 +++++++++++++++++++ 6 files changed, 112 insertions(+) create mode 100644 src/components/PopoverMenu/v2/content/useCloseOnEscape.ts create mode 100644 src/libs/suppressNextEscapeKeyUp/index.native.ts create mode 100644 src/libs/suppressNextEscapeKeyUp/index.ts diff --git a/src/components/PopoverMenu/index.tsx b/src/components/PopoverMenu/index.tsx index 5366b668735e..be41e479382a 100644 --- a/src/components/PopoverMenu/index.tsx +++ b/src/components/PopoverMenu/index.tsx @@ -27,6 +27,7 @@ import useWindowDimensions from '@hooks/useWindowDimensions'; import {isSafari} from '@libs/Browser'; import getPlatform from '@libs/getPlatform'; import {addKeyDownPressListener, removeKeyDownPressListener} from '@libs/KeyboardShortcut/KeyDownPressListener'; +import suppressNextEscapeKeyUp from '@libs/suppressNextEscapeKeyUp'; import variables from '@styles/variables'; import {close} from '@userActions/Modal'; import CONST from '@src/CONST'; @@ -540,6 +541,15 @@ 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}); + useKeyboardShortcut( + CONST.KEYBOARD_SHORTCUTS.ESCAPE, + () => { + suppressNextEscapeKeyUp(); + onClose(); + }, + {isActive: isVisible, shouldBubble: false}, + ); + const handleModalHide = () => { onModalHide?.(); setHasKeyBeenPressed(false); diff --git a/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts b/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts new file mode 100644 index 000000000000..2ff1fd0ab165 --- /dev/null +++ b/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts @@ -0,0 +1,16 @@ +import useKeyboardShortcut from '@hooks/useKeyboardShortcut'; +import suppressNextEscapeKeyUp from '@libs/suppressNextEscapeKeyUp'; +import CONST from '@src/CONST'; + +function useCloseOnEscape(isVisible: boolean, close: () => void): void { + useKeyboardShortcut( + CONST.KEYBOARD_SHORTCUTS.ESCAPE, + () => { + suppressNextEscapeKeyUp(); + close(); + }, + {isActive: isVisible, shouldBubble: false}, + ); +} + +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/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..fe5c259360e2 --- /dev/null +++ b/src/libs/suppressNextEscapeKeyUp/index.ts @@ -0,0 +1,18 @@ +/** + * Installed on `document` (not `document.body`) so it fires before any capture listener on body — + * specifically `ReanimatedModal`'s Escape keyup handler, which would otherwise dismiss the parent + * modal on the same keystroke. + */ +function suppressNextEscapeKeyUp(): () => void { + const onKeyUp = (event: KeyboardEvent) => { + if (event.key !== 'Escape') { + return; + } + event.stopImmediatePropagation(); + document.removeEventListener('keyup', onKeyUp, true); + }; + document.addEventListener('keyup', onKeyUp, true); + return () => document.removeEventListener('keyup', onKeyUp, true); +} + +export default suppressNextEscapeKeyUp; diff --git a/tests/unit/PopoverMenuV2Test.tsx b/tests/unit/PopoverMenuV2Test.tsx index 7c97bed8e66c..ab8f7be71188 100644 --- a/tests/unit/PopoverMenuV2Test.tsx +++ b/tests/unit/PopoverMenuV2Test.tsx @@ -394,6 +394,67 @@ describe('PopoverMenu V2', () => { ); expect(onOpenChange).not.toHaveBeenCalledWith(false); }); + + it('closes when Escape is pressed while open', () => { + const onOpenChange = jest.fn(); + render( + + + {}} + /> + + , + ); + onOpenChange.mockClear(); + pressShortcut('Escape'); + 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(); + pressShortcut('Escape'); + expect(onOpenChange).toHaveBeenCalledWith(false); + }); + + it('Escape shortcut is inactive while the popover is closed', () => { + render( + + + {}} + /> + + , + ); + expect(registeredShortcuts.Escape?.isActive).toBe(false); + }); }); describe('Trigger', () => { From e3db2eab09878ce742097f30690b3f9a8e48413e Mon Sep 17 00:00:00 2001 From: TaduJR Date: Mon, 1 Jun 2026 10:13:36 +0300 Subject: [PATCH 2/7] fix: route v1 Escape through handleClose so submenu state resets --- src/components/PopoverMenu/index.tsx | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/components/PopoverMenu/index.tsx b/src/components/PopoverMenu/index.tsx index be41e479382a..89ff52794d33 100644 --- a/src/components/PopoverMenu/index.tsx +++ b/src/components/PopoverMenu/index.tsx @@ -541,11 +541,17 @@ 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 = () => { + setCurrentMenuItems(menuItems); + setEnteredSubMenuIndexes(CONST.EMPTY_ARRAY); + onClose(); + }; + useKeyboardShortcut( CONST.KEYBOARD_SHORTCUTS.ESCAPE, () => { suppressNextEscapeKeyUp(); - onClose(); + handleClose(); }, {isActive: isVisible, shouldBubble: false}, ); @@ -659,11 +665,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} From 9eec7cdeb297bc958bf9117a4436fe809ae20fcf Mon Sep 17 00:00:00 2001 From: TaduJR Date: Mon, 1 Jun 2026 10:36:00 +0300 Subject: [PATCH 3/7] fix: singleton suppressNextEscapeKeyUp to prevent listener leak --- src/libs/suppressNextEscapeKeyUp/index.ts | 26 ++++++++++-- tests/unit/suppressNextEscapeKeyUpTest.ts | 51 +++++++++++++++++++++++ 2 files changed, 73 insertions(+), 4 deletions(-) create mode 100644 tests/unit/suppressNextEscapeKeyUpTest.ts diff --git a/src/libs/suppressNextEscapeKeyUp/index.ts b/src/libs/suppressNextEscapeKeyUp/index.ts index fe5c259360e2..057c0d97e945 100644 --- a/src/libs/suppressNextEscapeKeyUp/index.ts +++ b/src/libs/suppressNextEscapeKeyUp/index.ts @@ -1,18 +1,36 @@ /** - * Installed on `document` (not `document.body`) so it fires before any capture listener on body — - * specifically `ReanimatedModal`'s Escape keyup handler, which would otherwise dismiss the parent - * modal on the same keystroke. + * Swallows the next Escape `keyup` before any other handler sees it. + * - Listens on `document` (not `document.body`) so it precedes `ReanimatedModal`'s body keyup that would dismiss the parent modal. + * - Singleton: `PopoverProvider`'s earlier document-capture keyup can `stopImmediatePropagation` before ours runs, so each install evicts any pending listener. */ +let pending: ((event: KeyboardEvent) => void) | null = null; + function suppressNextEscapeKeyUp(): () => void { + if (pending) { + document.removeEventListener('keyup', pending, true); + pending = null; + } + const onKeyUp = (event: KeyboardEvent) => { if (event.key !== 'Escape') { return; } event.stopImmediatePropagation(); document.removeEventListener('keyup', onKeyUp, true); + if (pending === onKeyUp) { + pending = null; + } }; + + pending = onKeyUp; document.addEventListener('keyup', onKeyUp, true); - return () => document.removeEventListener('keyup', onKeyUp, true); + + return () => { + document.removeEventListener('keyup', onKeyUp, true); + if (pending === onKeyUp) { + pending = null; + } + }; } export default suppressNextEscapeKeyUp; diff --git a/tests/unit/suppressNextEscapeKeyUpTest.ts b/tests/unit/suppressNextEscapeKeyUpTest.ts new file mode 100644 index 000000000000..6da3ac07aa5f --- /dev/null +++ b/tests/unit/suppressNextEscapeKeyUpTest.ts @@ -0,0 +1,51 @@ +// 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); + }); +}); From 39f3ec11d5abaebc0e2cf1ae1fd8a41797967b30 Mon Sep 17 00:00:00 2001 From: TaduJR Date: Mon, 1 Jun 2026 10:54:43 +0300 Subject: [PATCH 4/7] fix: move suppressNextEscapeKeyUp to window capture so PopoverProvider can't preempt --- src/libs/suppressNextEscapeKeyUp/index.ts | 14 +++++++------- tests/unit/suppressNextEscapeKeyUpTest.ts | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/libs/suppressNextEscapeKeyUp/index.ts b/src/libs/suppressNextEscapeKeyUp/index.ts index 057c0d97e945..584b92b8ccf9 100644 --- a/src/libs/suppressNextEscapeKeyUp/index.ts +++ b/src/libs/suppressNextEscapeKeyUp/index.ts @@ -1,13 +1,13 @@ /** - * Swallows the next Escape `keyup` before any other handler sees it. - * - Listens on `document` (not `document.body`) so it precedes `ReanimatedModal`'s body keyup that would dismiss the parent modal. - * - Singleton: `PopoverProvider`'s earlier document-capture keyup can `stopImmediatePropagation` before ours runs, so each install evicts any pending listener. + * 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) { - document.removeEventListener('keyup', pending, true); + window.removeEventListener('keyup', pending, true); pending = null; } @@ -16,17 +16,17 @@ function suppressNextEscapeKeyUp(): () => void { return; } event.stopImmediatePropagation(); - document.removeEventListener('keyup', onKeyUp, true); + window.removeEventListener('keyup', onKeyUp, true); if (pending === onKeyUp) { pending = null; } }; pending = onKeyUp; - document.addEventListener('keyup', onKeyUp, true); + window.addEventListener('keyup', onKeyUp, true); return () => { - document.removeEventListener('keyup', onKeyUp, true); + window.removeEventListener('keyup', onKeyUp, true); if (pending === onKeyUp) { pending = null; } diff --git a/tests/unit/suppressNextEscapeKeyUpTest.ts b/tests/unit/suppressNextEscapeKeyUpTest.ts index 6da3ac07aa5f..f7a5292a29b3 100644 --- a/tests/unit/suppressNextEscapeKeyUpTest.ts +++ b/tests/unit/suppressNextEscapeKeyUpTest.ts @@ -48,4 +48,24 @@ describe('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); + } + }); }); From 25fb38f0081f2fd6ce2f366feb963d9dbf4cf661 Mon Sep 17 00:00:00 2001 From: TaduJR Date: Mon, 1 Jun 2026 11:37:25 +0300 Subject: [PATCH 5/7] fix: claim Escape keydown at window capture to bypass shortcut-stack race --- src/components/PopoverMenu/index.tsx | 16 ++++- .../v2/content/useCloseOnEscape.ts | 13 ++++ src/libs/claimEscapeKeyDown/index.native.ts | 5 ++ src/libs/claimEscapeKeyDown/index.ts | 18 ++++++ tests/unit/claimEscapeKeyDownTest.ts | 62 +++++++++++++++++++ 5 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 src/libs/claimEscapeKeyDown/index.native.ts create mode 100644 src/libs/claimEscapeKeyDown/index.ts create mode 100644 tests/unit/claimEscapeKeyDownTest.ts diff --git a/src/components/PopoverMenu/index.tsx b/src/components/PopoverMenu/index.tsx index 89ff52794d33..8e93d16f36b9 100644 --- a/src/components/PopoverMenu/index.tsx +++ b/src/components/PopoverMenu/index.tsx @@ -25,6 +25,7 @@ import useTheme from '@hooks/useTheme'; import useThemeStyles from '@hooks/useThemeStyles'; import useWindowDimensions from '@hooks/useWindowDimensions'; import {isSafari} from '@libs/Browser'; +import claimEscapeKeyDown from '@libs/claimEscapeKeyDown'; import getPlatform from '@libs/getPlatform'; import {addKeyDownPressListener, removeKeyDownPressListener} from '@libs/KeyboardShortcut/KeyDownPressListener'; import suppressNextEscapeKeyUp from '@libs/suppressNextEscapeKeyUp'; @@ -541,11 +542,22 @@ 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 = () => { + const handleClose = useCallback(() => { setCurrentMenuItems(menuItems); setEnteredSubMenuIndexes(CONST.EMPTY_ARRAY); onClose(); - }; + }, [menuItems, onClose]); + + // Web: window-capture keydown bypasses the shortcut stack. Native: useKeyboardShortcut below (the window listener no-ops on native). + useEffect(() => { + if (!isVisible) { + return undefined; + } + return claimEscapeKeyDown(() => { + suppressNextEscapeKeyUp(); + handleClose(); + }); + }, [isVisible, handleClose]); useKeyboardShortcut( CONST.KEYBOARD_SHORTCUTS.ESCAPE, diff --git a/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts b/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts index 2ff1fd0ab165..ec15769acfa2 100644 --- a/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts +++ b/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts @@ -1,8 +1,21 @@ +import {useEffect} from 'react'; import useKeyboardShortcut from '@hooks/useKeyboardShortcut'; +import claimEscapeKeyDown from '@libs/claimEscapeKeyDown'; import suppressNextEscapeKeyUp from '@libs/suppressNextEscapeKeyUp'; import CONST from '@src/CONST'; +// Web: window-capture keydown bypasses the shortcut stack. Native: useKeyboardShortcut below (the window listener no-ops on native). function useCloseOnEscape(isVisible: boolean, close: () => void): void { + useEffect(() => { + if (!isVisible) { + return undefined; + } + return claimEscapeKeyDown(() => { + suppressNextEscapeKeyUp(); + close(); + }); + }, [isVisible, close]); + useKeyboardShortcut( CONST.KEYBOARD_SHORTCUTS.ESCAPE, () => { 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/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); + } + }); +}); From 31b6538c11d2b6e4d231b1fe7a3ae1ddf7bbbf93 Mon Sep 17 00:00:00 2001 From: TaduJR Date: Thu, 4 Jun 2026 15:53:10 +0300 Subject: [PATCH 6/7] refactor: scope useCloseOnEscape to web --- src/components/PopoverMenu/index.tsx | 10 ------- .../v2/content/useCloseOnEscape.ts | 16 +++------- tests/unit/PopoverMenuV2Test.tsx | 29 ++++++++++++++++--- 3 files changed, 29 insertions(+), 26 deletions(-) diff --git a/src/components/PopoverMenu/index.tsx b/src/components/PopoverMenu/index.tsx index 8e93d16f36b9..99aca892ef03 100644 --- a/src/components/PopoverMenu/index.tsx +++ b/src/components/PopoverMenu/index.tsx @@ -548,7 +548,6 @@ function BasePopoverMenu({ onClose(); }, [menuItems, onClose]); - // Web: window-capture keydown bypasses the shortcut stack. Native: useKeyboardShortcut below (the window listener no-ops on native). useEffect(() => { if (!isVisible) { return undefined; @@ -559,15 +558,6 @@ function BasePopoverMenu({ }); }, [isVisible, handleClose]); - useKeyboardShortcut( - CONST.KEYBOARD_SHORTCUTS.ESCAPE, - () => { - suppressNextEscapeKeyUp(); - handleClose(); - }, - {isActive: isVisible, shouldBubble: false}, - ); - const handleModalHide = () => { onModalHide?.(); setHasKeyBeenPressed(false); diff --git a/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts b/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts index ec15769acfa2..be97de7bf014 100644 --- a/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts +++ b/src/components/PopoverMenu/v2/content/useCloseOnEscape.ts @@ -1,10 +1,11 @@ import {useEffect} from 'react'; -import useKeyboardShortcut from '@hooks/useKeyboardShortcut'; import claimEscapeKeyDown from '@libs/claimEscapeKeyDown'; import suppressNextEscapeKeyUp from '@libs/suppressNextEscapeKeyUp'; -import CONST from '@src/CONST'; -// Web: window-capture keydown bypasses the shortcut stack. Native: useKeyboardShortcut below (the window listener no-ops on native). +/** + * 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) { @@ -15,15 +16,6 @@ function useCloseOnEscape(isVisible: boolean, close: () => void): void { close(); }); }, [isVisible, close]); - - useKeyboardShortcut( - CONST.KEYBOARD_SHORTCUTS.ESCAPE, - () => { - suppressNextEscapeKeyUp(); - close(); - }, - {isActive: isVisible, shouldBubble: false}, - ); } export default useCloseOnEscape; diff --git a/tests/unit/PopoverMenuV2Test.tsx b/tests/unit/PopoverMenuV2Test.tsx index ab8f7be71188..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(), }; @@ -411,7 +432,7 @@ describe('PopoverMenu V2', () => { , ); onOpenChange.mockClear(); - pressShortcut('Escape'); + pressEscape(); expect(onOpenChange).toHaveBeenCalledWith(false); }); @@ -438,11 +459,11 @@ describe('PopoverMenu V2', () => { ); press('More'); onOpenChange.mockClear(); - pressShortcut('Escape'); + pressEscape(); expect(onOpenChange).toHaveBeenCalledWith(false); }); - it('Escape shortcut is inactive while the popover is closed', () => { + it('Escape listener is not installed while the popover is closed', () => { render( @@ -453,7 +474,7 @@ describe('PopoverMenu V2', () => { , ); - expect(registeredShortcuts.Escape?.isActive).toBe(false); + expect(mockActiveEscapeHandler).toBeNull(); }); }); From af275723199b4d56870a476a17b28231f2bcf145 Mon Sep 17 00:00:00 2001 From: TaduJR Date: Thu, 4 Jun 2026 16:10:15 +0300 Subject: [PATCH 7/7] refactor: reuse v2 useCloseOnEscape from v1 --- src/components/PopoverMenu/index.tsx | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/components/PopoverMenu/index.tsx b/src/components/PopoverMenu/index.tsx index 99aca892ef03..8dbbdcc44247 100644 --- a/src/components/PopoverMenu/index.tsx +++ b/src/components/PopoverMenu/index.tsx @@ -25,10 +25,8 @@ import useTheme from '@hooks/useTheme'; import useThemeStyles from '@hooks/useThemeStyles'; import useWindowDimensions from '@hooks/useWindowDimensions'; import {isSafari} from '@libs/Browser'; -import claimEscapeKeyDown from '@libs/claimEscapeKeyDown'; import getPlatform from '@libs/getPlatform'; import {addKeyDownPressListener, removeKeyDownPressListener} from '@libs/KeyboardShortcut/KeyDownPressListener'; -import suppressNextEscapeKeyUp from '@libs/suppressNextEscapeKeyUp'; import variables from '@styles/variables'; import {close} from '@userActions/Modal'; import CONST from '@src/CONST'; @@ -36,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 */ @@ -548,15 +547,7 @@ function BasePopoverMenu({ onClose(); }, [menuItems, onClose]); - useEffect(() => { - if (!isVisible) { - return undefined; - } - return claimEscapeKeyDown(() => { - suppressNextEscapeKeyUp(); - handleClose(); - }); - }, [isVisible, handleClose]); + useCloseOnEscape(isVisible, handleClose); const handleModalHide = () => { onModalHide?.();