-
-
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
Changes from 3 commits
da17a42
8cbf1c5
fcb258d
f4c2f1d
a808a3c
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,38 @@ const Dialog: React.FC<IDialogPropTypes> = (props) => { | |
| } | ||
|
|
||
| // >>> Content | ||
| const contentClickRef = useRef(false); | ||
| const contentTimeoutRef = useRef<ReturnType<typeof setTimeout>>(null); | ||
|
|
||
| // 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; | ||
| }); | ||
| }; | ||
| const mouseDownOnMaskRef = useRef(false); | ||
| const mouseUpOnMaskRef = useRef(false); | ||
|
|
||
| // >>> 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 && | ||
zkt2002 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| wrapperRef.current === e.target && | ||
| mouseDownOnMaskRef.current && | ||
| mouseUpOnMaskRef.current | ||
| ) { | ||
| onInternalClose(e); | ||
| } | ||
| }; | ||
|
Comment on lines
119
to
123
Contributor
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. To improve robustness, it's a good practice to reset |
||
| } | ||
|
|
||
| function onWrapperMouseDownCapture(e: React.MouseEvent) { | ||
| mouseDownOnMaskRef.current = e.target === wrapperRef.current; | ||
| } | ||
|
|
||
| function onWrapperMouseUpCapture(e: React.MouseEvent) { | ||
| mouseUpOnMaskRef.current = e.target === wrapperRef.current; | ||
| } | ||
|
|
||
| // ========================= Effect ========================= | ||
| useEffect(() => { | ||
| if (visible) { | ||
| mouseDownOnMaskRef.current = false; | ||
| mouseUpOnMaskRef.current = false; | ||
| setAnimatedVisible(true); | ||
| saveLastOutSideActiveElementRef(); | ||
|
|
||
|
|
@@ -158,14 +159,6 @@ const Dialog: React.FC<IDialogPropTypes> = (props) => { | |
| } | ||
| }, [visible]); | ||
|
|
||
| // Remove direct should also check the scroll bar update | ||
| useEffect( | ||
| () => () => { | ||
| clearTimeout(contentTimeoutRef.current); | ||
| }, | ||
| [], | ||
| ); | ||
|
|
||
| const mergedStyle: React.CSSProperties = { | ||
| zIndex, | ||
| ...wrapStyle, | ||
|
|
@@ -192,14 +185,14 @@ const Dialog: React.FC<IDialogPropTypes> = (props) => { | |
| className={clsx(`${prefixCls}-wrap`, wrapClassName, modalClassNames?.wrapper)} | ||
| ref={wrapperRef} | ||
| onClick={onWrapperClick} | ||
| onMouseDownCapture={onWrapperMouseDownCapture} | ||
zkt2002 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| onMouseUpCapture={onWrapperMouseUpCapture} | ||
| 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,90 @@ | ||
| 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(); | ||
zkt2002 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| }); | ||
|
|
||
| it('should not close when dragging from content to mask', () => { | ||
zkt2002 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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.