Skip to content
Closed
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
26 changes: 21 additions & 5 deletions src/Preview/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -327,10 +327,25 @@ const Preview: React.FC<PreviewProps> = props => {
}
};

const escClosingRef = useRef(false);
Copy link
Member

Choose a reason for hiding this comment

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

test 来一个?

Copy link
Member Author

Choose a reason for hiding this comment

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

AI加的,看能接受不?

const openRef = useRef(open);
openRef.current = open;

// >>>>> Effect: Keyboard
const onKeyDown = useEvent((event: KeyboardEvent) => {
if (open) {
const { keyCode } = event;
if (openRef.current) {
const { keyCode, key } = event;

if (keyCode === KeyCode.ESC || key === 'Escape') {
escClosingRef.current = true;
openRef.current = false;
event.preventDefault();
if (keyCode === KeyCode.ESC) {
event.stopPropagation();
}
Comment on lines +347 to +349
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

stopPropagation 仅在 keyCode 分支调用,key 分支缺失。

当浏览器仅提供 key: 'Escape' 而无 keyCode 时(现代浏览器趋势),事件仍会传播到父级监听器,可能导致父层对话框同时关闭,与 PR 目标"防止事件冒泡以确保嵌套层按顺序关闭"不一致。

🐛 建议修复:统一调用 stopPropagation
       if (keyCode === KeyCode.ESC || key === 'Escape') {
         if (escClosingRef.current) {
           return;
         }

         escClosingRef.current = true;
         openRef.current = false;
         event.preventDefault();
-        if (keyCode === KeyCode.ESC) {
-          event.stopPropagation();
-        }
+        event.stopPropagation();
         onClose?.();

         setTimeout(() => {
           escClosingRef.current = false;
         }, 0);
         return;
       }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Preview/index.tsx` around lines 347 - 349, The ESC handling only calls
event.stopPropagation() in the keyCode branch (if (keyCode === KeyCode.ESC)),
but not when the browser supplies event.key ('Escape') only; update the Preview
keyboard handler to ensure event.stopPropagation() is always invoked for Escape
regardless of which property is present — either by adding a matching branch
that checks event.key === 'Escape' and calls event.stopPropagation(), or by
normalizing the key check (e.g., derive isEscape from keyCode or key) before the
existing logic so the same stopPropagation call runs for both cases.

onClose?.();
Comment on lines +339 to +346
Copy link
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
Copy link

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) 在捕获阶段执行,早于任何子层 keydownPortal.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 All @@ -344,10 +359,10 @@ const Preview: React.FC<PreviewProps> = props => {

useEffect(() => {
if (open) {
window.addEventListener('keydown', onKeyDown);
window.addEventListener('keydown', onKeyDown, true);

return () => {
window.removeEventListener('keydown', onKeyDown);
window.removeEventListener('keydown', onKeyDown, true);
};
}
}, [open]);
Expand All @@ -364,6 +379,7 @@ const Preview: React.FC<PreviewProps> = props => {
const onVisibleChanged = (nextVisible: boolean) => {
if (!nextVisible) {
setLockScroll(false);
escClosingRef.current = false;
}
afterOpenChange?.(nextVisible);
};
Expand All @@ -377,7 +393,7 @@ const Preview: React.FC<PreviewProps> = props => {
}, [open]);

const onEsc: PortalProps['onEsc'] = ({ top }) => {
if (top) {
if (top && !escClosingRef.current) {
onClose?.();
}
};
Expand Down
91 changes: 91 additions & 0 deletions tests/preview.portal.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import { act, fireEvent, render } from '@testing-library/react';
import React from 'react';

jest.mock('@rc-component/motion', () => {
const MockCSSMotion = ({ children }: any) => children({ className: '', style: {} });
return {
__esModule: true,
default: MockCSSMotion,
};
});

jest.mock('@rc-component/portal', () => {
const React = require('react');

const MockPortal = (props: any) => {
(global as any).__portalProps = props;
return <>{props.children}</>;
};

return {
__esModule: true,
default: MockPortal,
};
});

import Preview from '../src/Preview';

describe('Preview portal esc fallback', () => {
it('uses capture phase for window keydown listener', () => {
const addSpy = jest.spyOn(window, 'addEventListener');
const removeSpy = jest.spyOn(window, 'removeEventListener');

const { unmount } = render(
<Preview prefixCls="rc-image-preview" open src="x" mousePosition={null} onClose={jest.fn()} />,
);

expect(addSpy).toHaveBeenCalledWith('keydown', expect.any(Function), true);

unmount();
expect(removeSpy).toHaveBeenCalledWith('keydown', expect.any(Function), true);

addSpy.mockRestore();
removeSpy.mockRestore();
});

it('keeps portal onEsc as fallback', () => {
const onClose = jest.fn();

render(
<Preview prefixCls="rc-image-preview" open src="x" mousePosition={null} onClose={onClose} />,
);

act(() => {
(global as any).__portalProps.onEsc({ top: true });
});

expect(onClose).toHaveBeenCalledTimes(1);
});

it('avoids duplicate close when keydown esc already handled (key only)', () => {
const onClose = jest.fn();

render(
<Preview prefixCls="rc-image-preview" open src="x" mousePosition={null} onClose={onClose} />,
);

fireEvent.keyDown(window, { key: 'Escape' });

act(() => {
(global as any).__portalProps.onEsc({ top: true });
});

expect(onClose).toHaveBeenCalledTimes(1);
});

it('avoids duplicate close when keydown esc already handled (keyCode only)', () => {
const onClose = jest.fn();

render(
<Preview prefixCls="rc-image-preview" open src="x" mousePosition={null} onClose={onClose} />,
);

fireEvent.keyDown(window, { keyCode: 27 });

act(() => {
(global as any).__portalProps.onEsc({ top: true });
});

expect(onClose).toHaveBeenCalledTimes(1);
});
});
Loading