-
-
Notifications
You must be signed in to change notification settings - Fork 118
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.
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
Changes from all commits
b3c0dc6
ad02d5d
be59728
d1bbe3b
3793804
b59d9b0
fb3f070
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -327,10 +327,25 @@ const Preview: React.FC<PreviewProps> = props => { | |
| } | ||
| }; | ||
|
|
||
| const escClosingRef = useRef(false); | ||
| 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
当浏览器仅提供 🐛 建议修复:统一调用 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 |
||
| onClose?.(); | ||
|
Comment on lines
+339
to
+346
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot Review it again. |
||
| return; | ||
|
Comment on lines
335
to
+347
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 -lRepository: react-component/image Length of output: 120 🏁 Script executed: # Get the relevant code sections
sed -n '333,345p' src/Preview/index.tsxRepository: 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.tsxRepository: 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.tsxRepository: react-component/image Length of output: 127 🏁 Script executed: # Look for escClosingRef definition and usage
rg -n "escClosingRef" src/Preview/index.tsxRepository: react-component/image Length of output: 239 🏁 Script executed: # Find Portal component and understand its onEsc handling
fd Portal src/ -type fRepository: 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 3Repository: 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 10Repository: 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 5Repository: react-component/image Length of output: 1260 🏁 Script executed: # Check the test file to understand expected behavior
cat tests/preview.portal.test.tsxRepository: 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 5Repository: 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,无法让内部子浮层优先响应。
建议:
🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| if (showLeftOrRightSwitches) { | ||
| if (keyCode === KeyCode.LEFT) { | ||
|
|
@@ -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]); | ||
|
|
@@ -364,6 +379,7 @@ const Preview: React.FC<PreviewProps> = props => { | |
| const onVisibleChanged = (nextVisible: boolean) => { | ||
| if (!nextVisible) { | ||
| setLockScroll(false); | ||
| escClosingRef.current = false; | ||
| } | ||
| afterOpenChange?.(nextVisible); | ||
| }; | ||
|
|
@@ -377,7 +393,7 @@ const Preview: React.FC<PreviewProps> = props => { | |
| }, [open]); | ||
|
|
||
| const onEsc: PortalProps['onEsc'] = ({ top }) => { | ||
| if (top) { | ||
| if (top && !escClosingRef.current) { | ||
| onClose?.(); | ||
| } | ||
| }; | ||
|
|
||
| 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); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test 来一个?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI加的,看能接受不?