Skip to content

fix: handle Escape in preview keydown for nested portal CI #499

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
yoyo837 wants to merge 7 commits into from
+112 −5
Closed

fix: handle Escape in preview keydown for nested portal CI #499

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b3c0dc6
fix: close preview on Escape in nested portal contexts
yoyo837 Mar 17, 2026
ad02d5d
fix: use capture phase and stopImmediatePropagation for ESC key handling
yoyo837 Mar 17, 2026
be59728
fix: remove stopImmediatePropagation to allow Portal stack to handle …
yoyo837 Mar 17, 2026
d1bbe3b
fix: keep portal onEsc fallback without double close
yoyo837 Mar 17, 2026
3793804
test: cover esc fallback and capture-phase listener
yoyo837 Mar 17, 2026
b59d9b0
fix: refine esc propagation and split key/keyCode tests
yoyo837 Mar 17, 2026
fb3f070
refactor: simplify esc guard to improve determinism
yoyo837 Mar 17, 2026
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
16 changes: 8 additions & 8 deletions src/Preview/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,14 @@ const Preview: React.FC<PreviewProps> = props => {
// >>>>> Effect: Keyboard
const onKeyDown = useEvent((event: KeyboardEvent) => {
if (open) {
const { keyCode } = event;
const { keyCode, key } = event;

if (keyCode === KeyCode.ESC || key === 'Escape') {
event.preventDefault();
event.stopPropagation();

@gemini-code-assist gemini-code-assist Bot Mar 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using event.stopPropagation() here will not prevent other event listeners attached directly to the window object from firing. It only stops the event from bubbling up the DOM tree. Since both the Preview and a parent Dialog component might register their own keydown listeners on window, this could lead to unexpected behavior where both close simultaneously if the event listener registration order is not as expected.

To ensure that only the topmost overlay's event handler runs and prevents any others on window from executing, you should use event.stopImmediatePropagation(). This will stop the invocation of any subsequent listeners on the same element (window in this case).

Suggested change
event.stopPropagation();
event.stopImmediatePropagation();

onClose?.();
Comment on lines +339 to +346

@yoyo837 yoyo837 Mar 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Review it again.

return;
Comment on lines 335 to +347

@coderabbitai coderabbitai Bot Mar 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the Preview component file and the specific lines mentioned
cd /repo && head -c 500000 src/Preview/index.tsx | wc -l

Repository: react-component/image

Length of output: 120

🏁 Script executed:

# Get the relevant code sections
sed -n '333,345p' src/Preview/index.tsx

Repository: react-component/image

Length of output: 433

🏁 Script executed:

# Check the other mentioned line ranges
sed -n '358,364p' src/Preview/index.tsx
sed -n '392,395p' src/Preview/index.tsx

Repository: react-component/image

Length of output: 373

🏁 Script executed:

# Search for window.addEventListener in the file to see capture phase usage
rg -n "addEventListener.*keydown" src/Preview/index.tsx

Repository: react-component/image

Length of output: 127

🏁 Script executed:

# Look for escClosingRef definition and usage
rg -n "escClosingRef" src/Preview/index.tsx

Repository: react-component/image

Length of output: 239

🏁 Script executed:

# Find Portal component and understand its onEsc handling
fd Portal src/ -type f

Repository: react-component/image

Length of output: 235

🏁 Script executed:

# Check if there's a Portal component and how it handles ESC
rg -n "onEsc" src/ -A 3

Repository: react-component/image

Length of output: 469

🏁 Script executed:

find . -name "*.ts" -o -name "*.tsx" | xargs grep -l "onEsc.*top"

Repository: react-component/image

Length of output: 247

🏁 Script executed:

# Look for Portal component definition
rg -n "interface PortalProps|type PortalProps" -A 10

Repository: react-component/image

Length of output: 962

🏁 Script executed:

# Check how Portal uses the onEsc callback and what 'top' means
rg -n "onEsc\(\{.*top" -B 5 -A 5

Repository: react-component/image

Length of output: 1260

🏁 Script executed:

# Check the test file to understand expected behavior
cat tests/preview.portal.test.tsx

Repository: react-component/image

Length of output: 2045

🏁 Script executed:

# Search for any nested Portal or child component ESC handling tests
rg -n "imageRender|actionsRender" tests/ -A 5 -B 5

Repository: react-component/image

Length of output: 2796

🏁 Script executed:

# Look for any tests with nested Dropdown or Portal scenarios
rg -n "Dropdown|nested" tests/

Repository: react-component/image

Length of output: 47

window capture 阶段的 ESC 处理会直接关闭 Preview，无法让内部子浮层优先响应。

window.addEventListener('keydown', onKeyDown, true) 在捕获阶段执行，早于任何子层 keydown 或 Portal.onEsc 回调。这里直接调用 onClose()，设置 escClosingRef.current = true 后，虽然能防止 Portal.onEsc 重复调用 onClose，但无法让子浮层（如 imageRender/actionsRender 中的 Dropdown 或嵌套 Portal）优先处理 ESC。按 ESC 时 Preview 会先关闭，打破了 Portal 的 top 语义。此外只有 preventDefault() 而无 stopPropagation()，父层的原生 keydown 监听仍会收到事件。

建议：

  1. 把直接 onClose 的路径收窄为真正的 fallback（检查子层是否已处理）
  2. 补充"Preview 内嵌 Dropdown/子 Portal + ESC 优先权"的回归用例
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/Preview/index.tsx` around lines 333 - 345, The capture-phase global
keydown listener (window.addEventListener(..., true)) in onKeyDown currently
forces Preview to close before child Portals/Dropdowns can handle ESC; change
the global listener to the bubble phase (remove the third-arg true / use false)
so descendant handlers get priority, and in onKeyDown first check
event.defaultPrevented and return early (do not call onClose) to treat
child-handled ESC as authoritative; when you do call onClose, keep escClosingRef
handling but also call event.stopPropagation() in addition to
event.preventDefault() to avoid parent/native handlers receiving the event;
update tests to cover "Preview with nested Dropdown/Portal ESC priority" to
ensure child handlers can prevent Preview closing.

}

if (showLeftOrRightSwitches) {
if (keyCode === KeyCode.LEFT) {
Expand Down Expand Up @@ -376,12 +383,6 @@ const Preview: React.FC<PreviewProps> = props => {
}
}, [open]);

const onEsc: PortalProps['onEsc'] = ({ top }) => {
if (top) {
onClose?.();
}
};

// ========================== Render ==========================
const bodyStyle: React.CSSProperties = {
...styles.body,
Expand All @@ -396,7 +397,6 @@ const Preview: React.FC<PreviewProps> = props => {
autoDestroy={false}
getContainer={getContainer}
autoLock={lockScroll}
onEsc={onEsc}
>
<CSSMotion
motionName={motionName}
Expand Down
Loading