Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/components/PopoverMenu/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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}
Expand Down
21 changes: 21 additions & 0 deletions src/components/PopoverMenu/v2/content/useCloseOnEscape.ts
Original file line number Diff line number Diff line change
@@ -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 {
Comment thread
TaduJR marked this conversation as resolved.
useEffect(() => {
if (!isVisible) {
return undefined;
}
return claimEscapeKeyDown(() => {
suppressNextEscapeKeyUp();
close();
});
}, [isVisible, close]);
}

export default useCloseOnEscape;
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -26,6 +27,7 @@ function useContentController(componentName: string): {

useCloseOnModalCover(isVisible, close);
useCloseOnScreenBlur(close);
useCloseOnEscape(isVisible, close);

return {
navigation: {currentSubID: subNav.currentSubID, isAncestorOfCurrent: subNav.isAncestorOfCurrent},
Expand Down
5 changes: 5 additions & 0 deletions src/libs/claimEscapeKeyDown/index.native.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
function claimEscapeKeyDown(): () => void {
return () => {};
}

export default claimEscapeKeyDown;
18 changes: 18 additions & 0 deletions src/libs/claimEscapeKeyDown/index.ts
Original file line number Diff line number Diff line change
@@ -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;
5 changes: 5 additions & 0 deletions src/libs/suppressNextEscapeKeyUp/index.native.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
function suppressNextEscapeKeyUp(): () => void {
return () => {};
}

export default suppressNextEscapeKeyUp;
36 changes: 36 additions & 0 deletions src/libs/suppressNextEscapeKeyUp/index.ts
Original file line number Diff line number Diff line change
@@ -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;
82 changes: 82 additions & 0 deletions tests/unit/PopoverMenuV2Test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,27 @@
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 () => {

Check failure on line 121 in tests/unit/PopoverMenuV2Test.tsx

View workflow job for this annotation

GitHub Actions / ESLint check

Prefer an early return to a conditionally-wrapped function body

Check failure on line 121 in tests/unit/PopoverMenuV2Test.tsx

View workflow job for this annotation

GitHub Actions / ESLint check

Prefer an early return to a conditionally-wrapped function body
if (mockActiveEscapeHandler === handler) {
mockActiveEscapeHandler = null;
}
};
},
}));

function pressEscape(): void {
if (!mockActiveEscapeHandler) {
return;
}
act(() => mockActiveEscapeHandler?.());
}

const mockNavigationState: {blurListeners: Set<() => void>} = {
blurListeners: new Set(),
};
Expand Down Expand Up @@ -394,6 +415,67 @@
);
expect(onOpenChange).not.toHaveBeenCalledWith(false);
});

it('closes when Escape is pressed while open', () => {
const onOpenChange = jest.fn();
render(
<Harness
initialOpen
onOpenChange={onOpenChange}
>
<PopoverMenu.Content>
<PopoverMenu.Item
text="A"
onSelect={() => {}}
/>
</PopoverMenu.Content>
</Harness>,
);
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(
<Harness
initialOpen
onOpenChange={onOpenChange}
>
<PopoverMenu.Content>
<PopoverMenu.Sub id="sub">
<PopoverMenu.Sub.Trigger text="More" />
<PopoverMenu.Sub.Content>
<PopoverMenu.Sub.BackButton text="Back" />
<PopoverMenu.Item
text="Inner"
onSelect={() => {}}
/>
</PopoverMenu.Sub.Content>
</PopoverMenu.Sub>
</PopoverMenu.Content>
</Harness>,
);
press('More');
onOpenChange.mockClear();
pressEscape();
expect(onOpenChange).toHaveBeenCalledWith(false);
});

it('Escape listener is not installed while the popover is closed', () => {
render(
<Harness>
<PopoverMenu.Content>
<PopoverMenu.Item
text="A"
onSelect={() => {}}
/>
</PopoverMenu.Content>
</Harness>,
);
expect(mockActiveEscapeHandler).toBeNull();
});
});

describe('Trigger', () => {
Expand Down
62 changes: 62 additions & 0 deletions tests/unit/claimEscapeKeyDownTest.ts
Original file line number Diff line number Diff line change
@@ -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);
}
});
});
71 changes: 71 additions & 0 deletions tests/unit/suppressNextEscapeKeyUpTest.ts
Original file line number Diff line number Diff line change
@@ -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);
}
});
});
Loading