Skip to content

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.

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

Merged
zombieJ merged 5 commits into from
Feb 5, 2026
+36 −28
Merged

fix(Dialog): prevent mask close when dragging from content to mask #543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
da17a42
fix(Dialog): prevent mask close when dragging from content to mask
Jan 9, 2026
8cbf1c5
refactor: optimize logic and reset refs based on review
Jan 9, 2026
fcb258d
style: optimize code formatting and naming convention
Jan 9, 2026
f4c2f1d
fix: prevent modal close when dragging from content to mask
Jan 14, 2026
a808a3c
fix(Dialog): 移除 maskClosable 条件下不必要的 onClose 存在性检查，确保拖拽行为不会意外触发对话框关闭。…
Feb 2, 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
35 changes: 8 additions & 27 deletions src/Dialog/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,37 +110,27 @@ 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);

// >>> 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 (wrapperRef.current === e.target && mouseDownOnMaskRef.current) {
onInternalClose(e);
}
};
Comment on lines 119 to 123
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 5, 2026

Choose a reason for hiding this comment

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

medium

To improve robustness, it's a good practice to reset mouseDownOnMaskRef.current to false after every click attempt on the wrapper. This ensures that the mousedown state is only valid for the immediately following click, preventing unexpected behavior in complex event sequences. This also correctly handles clicks on the content that bubble up to the wrapper, resetting the state as expected.

    onWrapperClick = (e) => {
      if (wrapperRef.current === e.target && mouseDownOnMaskRef.current) {
        onInternalClose(e);
      }
      mouseDownOnMaskRef.current = false;
    };

}

function onWrapperMouseDown(e: React.MouseEvent) {
mouseDownOnMaskRef.current = e.target === wrapperRef.current;
}

// ========================= Effect =========================
useEffect(() => {
if (visible) {
mouseDownOnMaskRef.current = false;
setAnimatedVisible(true);
saveLastOutSideActiveElementRef();

Expand All @@ -158,14 +148,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,
Expand All @@ -192,14 +174,13 @@ const Dialog: React.FC<IDialogPropTypes> = (props) => {
className={clsx(`${prefixCls}-wrap`, wrapClassName, modalClassNames?.wrapper)}
ref={wrapperRef}
onClick={onWrapperClick}
onMouseDown={onWrapperMouseDown}
style={mergedStyle}
{...wrapProps}
>
<Content
{...props}
isFixedPos={isFixedPos}
onMouseDown={onContentMouseDown}
onMouseUp={onContentMouseUp}
ref={contentRef}
closable={closable}
ariaId={ariaId}
Expand Down
29 changes: 28 additions & 1 deletion tests/index.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,10 @@ describe('dialog', () => {
const { rerender } = render(<Dialog onClose={onClose} visible />);

// Mask close
fireEvent.click(document.querySelector('.rc-dialog-wrap'));
const mask = document.querySelector('.rc-dialog-wrap');
fireEvent.mouseDown(mask);
fireEvent.mouseUp(mask);
fireEvent.click(mask);
jest.runAllTimers();
expect(onClose).toHaveBeenCalled();
onClose.mockReset();
Expand All @@ -185,6 +188,30 @@ describe('dialog', () => {
expect(onClose).not.toHaveBeenCalled();
});

it('should not close when dragging from content to mask', () => {
const onClose = jest.fn();
const { getByText } = render(
<Dialog visible maskClosable onClose={onClose}>
Content
</Dialog>
);

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();
});
Comment on lines +191 to +213
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 5, 2026

Choose a reason for hiding this comment

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

medium

The test name 'should not close when dragging from content to mask' and the associated comments are slightly misleading. The test currently simulates a mousedown on the content followed by a separate click on the mask, rather than a continuous drag action. To improve clarity and maintainability for future developers, it would be beneficial to rename the test and update the comments to accurately reflect the event sequence being tested.

  it('should not close on mask click if mouse down was on content', () => {
    const onClose = jest.fn();
    const { getByText } = render(
      <Dialog visible maskClosable onClose={onClose}>
        Content
      </Dialog>,
    );

    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, then a click on the mask.
    // The dialog should not close because the mousedown did not originate on the mask.
    fireEvent.mouseDown(content);
    fireEvent.mouseUp(mask); // Not strictly required, but makes sequence more explicit
    fireEvent.click(mask);

    expect(onClose).not.toHaveBeenCalled();
  });


it('renderToBody', () => {
const container = document.createElement('div');
document.body.appendChild(container);
Expand Down
Loading