-
-
Notifications
You must be signed in to change notification settings - Fork 206
fix(Dialog): prevent mask close when dragging from content to mask #543
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
da17a42
8cbf1c5
fcb258d
f4c2f1d
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 |
|---|---|---|
|
|
@@ -110,37 +110,40 @@ const Dialog: React.FC<IDialogPropTypes> = (props) => { | |
| } | ||
|
|
||
| // >>> Content | ||
| const contentClickRef = useRef(false); | ||
| const contentTimeoutRef = useRef<ReturnType<typeof setTimeout>>(null); | ||
| const mouseDownOnMaskRef = useRef(false); | ||
| const mouseUpOnMaskRef = useRef(false); | ||
|
|
||
| // We need record content click in case content popup out of dialog | ||
| const onContentMouseDown: React.MouseEventHandler = () => { | ||
| clearTimeout(contentTimeoutRef.current); | ||
| contentClickRef.current = true; | ||
| }; | ||
|
|
||
| const onContentMouseUp: React.MouseEventHandler = () => { | ||
| contentTimeoutRef.current = setTimeout(() => { | ||
| contentClickRef.current = false; | ||
| }); | ||
| }; | ||
|
|
||
zkt2002 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // >>> Wrapper | ||
| // Close only when element not on dialog | ||
| let onWrapperClick: (e: React.SyntheticEvent) => void = null; | ||
| if (maskClosable) { | ||
| onWrapperClick = (e) => { | ||
| if (contentClickRef.current) { | ||
| contentClickRef.current = false; | ||
| } else if (wrapperRef.current === e.target) { | ||
| if ( | ||
| onClose && | ||
|
Member
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. onInternalClose 里本来就会处理 onClose,这边不用加到条件里。 |
||
| wrapperRef.current === e.target && | ||
| mouseDownOnMaskRef.current && | ||
| mouseUpOnMaskRef.current | ||
| ) { | ||
| onInternalClose(e); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| function onModulesMouseDownCapture(e: React.MouseEvent) { | ||
| mouseDownOnMaskRef.current = e.target === wrapperRef.current; | ||
| } | ||
zkt2002 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| function onModulesMouseUpCapture(e: React.MouseEvent) { | ||
| mouseUpOnMaskRef.current = e.target === wrapperRef.current; | ||
| } | ||
zkt2002 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // ========================= Effect ========================= | ||
| useEffect(() => { | ||
| if (visible) { | ||
| mouseDownOnMaskRef.current = false; | ||
| mouseUpOnMaskRef.current = false; | ||
| setAnimatedVisible(true); | ||
| saveLastOutSideActiveElementRef(); | ||
|
|
||
|
|
@@ -158,13 +161,7 @@ const Dialog: React.FC<IDialogPropTypes> = (props) => { | |
| } | ||
| }, [visible]); | ||
|
|
||
| // Remove direct should also check the scroll bar update | ||
| useEffect( | ||
| () => () => { | ||
| clearTimeout(contentTimeoutRef.current); | ||
| }, | ||
| [], | ||
| ); | ||
|
|
||
|
|
||
zkt2002 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| const mergedStyle: React.CSSProperties = { | ||
| zIndex, | ||
|
|
@@ -192,14 +189,14 @@ const Dialog: React.FC<IDialogPropTypes> = (props) => { | |
| className={clsx(`${prefixCls}-wrap`, wrapClassName, modalClassNames?.wrapper)} | ||
| ref={wrapperRef} | ||
| onClick={onWrapperClick} | ||
| onMouseDownCapture={onModulesMouseDownCapture} | ||
| onMouseUpCapture={onModulesMouseUpCapture} | ||
| style={mergedStyle} | ||
| {...wrapProps} | ||
| > | ||
| <Content | ||
| {...props} | ||
| isFixedPos={isFixedPos} | ||
| onMouseDown={onContentMouseDown} | ||
| onMouseUp={onContentMouseUp} | ||
| ref={contentRef} | ||
| closable={closable} | ||
| ariaId={ariaId} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
|
|
||
zkt2002 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| import React from 'react'; | ||
| import { render, fireEvent, act } from '@testing-library/react'; | ||
| import Dialog from '../src'; | ||
|
|
||
| describe('Dialog.MaskClosable', () => { | ||
| beforeEach(() => { | ||
| jest.useFakeTimers(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.useRealTimers(); | ||
| }); | ||
|
|
||
| it('should close when click on mask', () => { | ||
| const onClose = jest.fn(); | ||
| render( | ||
| <Dialog visible maskClosable onClose={onClose}> | ||
| Content | ||
| </Dialog> | ||
| ); | ||
|
|
||
| act(() => { | ||
| jest.runAllTimers(); | ||
| }); | ||
|
|
||
| const mask = document.querySelector('.rc-dialog-wrap'); | ||
| if (!mask) throw new Error('Mask not found'); | ||
|
|
||
| fireEvent.mouseDown(mask); | ||
| fireEvent.mouseUp(mask); | ||
| fireEvent.click(mask); | ||
| expect(onClose).toHaveBeenCalled(); | ||
|
Member
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. 这个 test 和 index.sepc.tsx 是一样的,而且也没有 timeout 需要模拟。可以删掉 |
||
| }); | ||
|
|
||
| it('should not close when dragging from content to mask', () => { | ||
|
Member
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. 这个demo 挪到 index.spec.tsx 里,不用额外写一个文件。就挪到 |
||
| const onClose = jest.fn(); | ||
| const { getByText } = render( | ||
| <Dialog visible maskClosable onClose={onClose}> | ||
| Content | ||
| </Dialog> | ||
| ); | ||
|
|
||
| act(() => { | ||
| jest.runAllTimers(); | ||
| }); | ||
|
|
||
| const content = getByText('Content'); | ||
| const mask = document.querySelector('.rc-dialog-wrap'); | ||
| if (!mask) throw new Error('Mask not found'); | ||
|
|
||
| // Simulate mouse down on content | ||
| fireEvent.mouseDown(content); | ||
| // Simulate mouse up on mask | ||
| fireEvent.mouseUp(mask); | ||
| // Simulate click on mask (since click follows mouseup) | ||
| fireEvent.click(mask); | ||
|
|
||
| expect(onClose).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should not close when dragging from mask to content', () => { | ||
| const onClose = jest.fn(); | ||
| const { getByText } = render( | ||
| <Dialog visible maskClosable onClose={onClose}> | ||
| Content | ||
| </Dialog> | ||
| ); | ||
|
|
||
| act(() => { | ||
| jest.runAllTimers(); | ||
| }); | ||
|
|
||
| const content = getByText('Content'); | ||
| const mask = document.querySelector('.rc-dialog-wrap'); | ||
| if (!mask) throw new Error('Mask not found'); | ||
|
|
||
| // Simulate mouse down on mask | ||
| fireEvent.mouseDown(mask); | ||
| // Simulate mouse up on content | ||
| fireEvent.mouseUp(content); | ||
| // Simulate click on mask (since click follows mouseup) | ||
| // Note: In real browser, click event might trigger on the common ancestor or user logic might vary, | ||
| // but here we simulate what happens if a click event reaches the wrapper. | ||
| // If we drag from mask to content, the click likely happens on content or common parent. | ||
| // But if propagation reaches wrapper, we want to ensure it doesn't close. | ||
| fireEvent.click(mask); | ||
|
|
||
| expect(onClose).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.