fix(Dialog): prevent mask close when dragging from content to mask #543
Conversation
- Use event capture to track `mousedown` and `mouseup` on the wrapper - Ensure close is only triggered when the click action fully occurs on the mask - Fix issue where dragging from content to mask (e.g. slider or text selection) triggers unintended close Closes ant-design/ant-design#56348
Walkthrough将 Dialog 的遮罩关闭逻辑由基于内容的点击计时器改为基于遮罩鼠标事件的双引用追踪（mouseDownOnMaskRef、mouseUpOnMaskRef），新增 wrapper 捕获事件并更新相关单元测试以覆盖拖拽与完整鼠标交互场景。 Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Wrapper as Wrapper/遮罩
participant Dialog as Dialog 组件
participant Content as Content/内容
User->>Wrapper: mouseDown
Wrapper->>Dialog: onWrapperMouseDownCapture (mouseDownOnMaskRef = true)
User->>Wrapper: mouseUp
Wrapper->>Dialog: onWrapperMouseUpCapture (mouseUpOnMaskRef = true)
User->>Wrapper: click
Wrapper->>Dialog: onWrapperClick (检查 target 为 wrapper && mouseDownOnMaskRef && mouseUpOnMaskRef)
alt 满足关闭条件
Dialog->>Dialog: onInternalClose()
Dialog->>User: 调用 onClose
else 不满足（拖拽等）
Dialog->>Dialog: 不触发关闭
end
Estimated code review effort🎯 3 (中等) | ⏱️ ~20 分钟 建议审查者
诗歌
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Someone is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello @zkt2002, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical usability bug in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively resolves an issue where dragging from the dialog's content to its mask would unintentionally close the dialog. The new approach, which tracks mousedown and mouseup events in the capture phase, is a robust and clean solution. It correctly ensures that the dialog only closes when a complete click action occurs on the mask. The removal of the previous setTimeout-based logic simplifies the component, and the addition of comprehensive unit tests for various drag scenarios is a great enhancement to ensure the fix is solid. I have a couple of minor suggestions to further simplify the new event handlers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/Dialog/index.tsx:
- Around line 113-114: Reset mouseDownOnMaskRef and mouseUpOnMaskRef when the
dialog opens (or on close) to avoid stale cross-session state; inside the Dialog
component add a useEffect watching the open/visible prop (the same prop used to
control mounting) and set mouseDownOnMaskRef.current = false and
mouseUpOnMaskRef.current = false whenever the dialog becomes visible (or when it
unmounts/closes) so previous mouse events do not leak into the next session.
🧹 Nitpick comments (1)
src/Dialog/index.tsx (1)
134-148: 建议添加注释说明遮罩点击追踪的逻辑这两个捕获阶段处理器实现了新的遮罩点击追踪机制,逻辑正确。但为了提高代码可维护性,建议添加注释说明:
- 为什么使用捕获阶段(
Capture)- 为什么检查
e.target === wrapperRef.current而不是使用e.currentTarget- 这种机制如何防止拖拽操作触发关闭
参考注释示例
+ // Use capture phase to track mouse events on the mask element before they bubble. + // Check e.target (the actual clicked element) to ensure the event occurred + // directly on the wrapper/mask, not on any child elements. function onModulesMouseDownCapture(e: React.MouseEvent) { if (e.target === wrapperRef.current) { mouseDownOnMaskRef.current = true; } else { mouseDownOnMaskRef.current = false; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Dialog/index.tsxtests/index.spec.tsxtests/mask-closable.spec.tsx
🔇 Additional comments (6)
src/Dialog/index.tsx (2)
123-128: 逻辑正确,有效修复了拖拽触发关闭的问题新的条件判断确保只有当完整的点击序列(mouseDown + mouseUp + click)都发生在遮罩上时才触发关闭,这正确解决了从内容拖拽到遮罩导致意外关闭的问题。实现思路清晰合理。
198-199: 捕获处理器集成正确捕获事件处理器已正确附加到包装器元素上,与
onClick处理器配合工作以实现新的遮罩点击追踪机制。tests/index.spec.tsx (1)
176-179: 测试更新正确反映了新的交互模型测试已正确更新为使用完整的鼠标交互序列(mouseDown → mouseUp → click),与新的遮罩点击追踪实现保持一致。
tests/mask-closable.spec.tsx (3)
15-34: 遮罩点击关闭测试实现正确测试正确验证了在遮罩上执行完整点击序列时对话框会关闭的行为。防御性的空值检查(第 28 行)也很好。
36-60: 关键测试用例:正确验证拖拽场景此测试用例验证了 PR 要解决的核心问题 - 从内容拖拽到遮罩时不应触发关闭。测试逻辑准确模拟了用户拖拽操作(如滑动滑块或选择文本),确保对话框不会意外关闭。
62-90: 反向拖拽场景测试完整测试覆盖了从遮罩拖拽到内容的场景,确保这种情况也不会触发关闭。第 83-86 行的注释体现了对真实浏览器行为的深入思考,有助于理解测试意图。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #543 +/- ##
==========================================
+ Coverage 98.42% 98.94% +0.51%
==========================================
Files 8 8
Lines 191 190 -1
Branches 67 70 +3
==========================================
Hits 188 188
+ Misses 3 2 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the dialog would unintentionally close when users drag from content to mask (e.g., when using a slider or selecting text). The fix replaces a timeout-based click tracking mechanism with a more precise event capture approach that tracks where mouseDown and mouseUp events occur.
Key Changes:
- Introduced capture-phase event handlers to accurately track mouseDown and mouseUp locations on the wrapper element
- Removed the previous timeout-based content click tracking mechanism
- Added comprehensive test coverage for the drag-and-drop scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/Dialog/index.tsx | Replaced timeout-based content click tracking with capture-phase mouseDown/mouseUp tracking on the wrapper element; removed cleanup effect for the old timeout mechanism |
| tests/index.spec.tsx | Updated the "mask to close" test to fire mouseDown and mouseUp events before click to match the new behavior |
| tests/mask-closable.spec.tsx | Added comprehensive test suite covering mask closure scenarios: clicking on mask, dragging from content to mask, and dragging from mask to content |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| className={clsx(`${prefixCls}-wrap`, wrapClassName, modalClassNames?.wrapper)} | ||
| ref={wrapperRef} | ||
| onClick={onWrapperClick} | ||
| onMouseDownCapture={onWrapperMouseDownCapture} |
There was a problem hiding this comment.
不要用 capture，会导致很多非预期的冲突。可以考虑在 mouseDown 的时候记录一下是否点击在 mask，click 的时候对比一下 target 就行了
2 participants
mousedownandmouseupon the wrapperSummary by CodeRabbit
发布说明
Bug 修复
测试
✏️ Tip: You can customize this high-level summary in your review settings.