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
演练重构了 Dialog 组件的关闭交互逻辑，从基于内容元素的点击追踪改为基于遮罩的点击追踪。通过新增 变更
序列图sequenceDiagram
participant User
participant Wrapper
participant Dialog
participant Mask
participant Content
rect rgba(100, 150, 200, 0.5)
Note over User,Content: 正常关闭流程（点击遮罩）
User->>Wrapper: mouseDown on mask
activate Wrapper
Wrapper->>Dialog: onWrapperMouseDown()
Dialog->>Dialog: mouseDownOnMaskRef.current = true
deactivate Wrapper
User->>Wrapper: click on mask
activate Wrapper
Wrapper->>Dialog: onWrapperClick()
alt mouseDownOnMaskRef.current == true
Dialog->>Dialog: trigger onInternalClose()
else
Dialog->>Dialog: no action
end
Dialog->>Dialog: reset mouseDownOnMaskRef.current = false
deactivate Wrapper
end
rect rgba(200, 100, 100, 0.5)
Note over User,Content: 拖拽防护流程（从内容拖至遮罩）
User->>Content: mouseDown on content
activate Content
Content->>Dialog: mouseDownOnMaskRef = false
deactivate Content
User->>Mask: mouseUp on mask
activate Mask
Mask->>Dialog: onWrapperClick()
alt mouseDownOnMaskRef.current == false
Dialog->>Dialog: no action (drag detected)
end
deactivate Mask
end
预估审查工作量🎯 3 (中等) | ⏱️ ~25 分钟 建议审查人
诗
🚥 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)
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.91% +0.48%
==========================================
Files 8 8
Lines 191 185 -6
Branches 67 70 +3
==========================================
- Hits 188 183 -5
+ 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.
- Use bubbling `onMouseDown` to track if the click originated from the mask. - Only trigger close in `onClick` if both mousedown and click targets are the wrapper. - Reset tracking refs when dialog becomes visible to prevent state pollution. - Remove capture listeners to avoid potential event conflicts.
|
能合并了嘛大佬 @zombieJ |
|
@gemini-code-assist Pls help to CR again. |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue where dragging from the dialog content to the mask would unintentionally trigger the dialog's close event. The solution involves tracking the mousedown event on the wrapper to ensure that the onClose callback is only invoked if both the mousedown and subsequent mouseup/click events originate from the mask itself. The previous contentClickRef logic has been removed, simplifying the event handling. Additionally, comprehensive unit tests have been added in a new file, tests/mask-closable.spec.tsx, to specifically cover the mask close behavior, including scenarios where dragging from content to mask should not trigger a close. This is a well-implemented and thoroughly tested fix.
…同时将相关测试从独立文件合并到主测试文件中以简化结构。
2 participants
mousedownandmouseupon the wrapperSummary by CodeRabbit