fix: mouseDown on the hidden element should not trigger close #1178
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @zombieJ, 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 resolves a bug in the Highlights
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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1178 +/- ##
=======================================
Coverage 99.41% 99.41%
=======================================
Files 31 31
Lines 1200 1202 +2
Branches 428 427 -1
=======================================
+ Hits 1193 1195 +2
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
概览此PR修复了Select组件在点击箭头图标时下拉菜单立即关闭的问题。通过在SelectInput中为事件标记 变更详情
代码审查工作量估计🎯 2 (简单) | ⏱️ ~12 分钟
可能相关的PR
诗歌
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/SelectInput/index.tsx (1)
175-179: 考虑更精确地限定标记范围当前实现对 SelectInput 的所有 mouseDown 事件设置了
_ignore_global_close标记，但根据关联 issue (#56002)，问题仅出现在点击后缀图标（箭头）时。将此标记应用于所有点击可能会掩盖其他应该触发关闭的场景。建议仅在点击后缀/图标区域时设置此标记：
const onInternalMouseDown: SelectInputProps['onMouseDown'] = useEvent((event) => { if (!disabled) { - // https://github.com/ant-design/ant-design/issues/56002 - // Tell `useSelectTriggerControl` to ignore this event - // When icon is dynamic render, the parentNode will miss - // so we need to mark the event directly - (event.nativeEvent as any)._ignore_global_close = true; const inputDOM = getDOM(inputRef.current); if (inputDOM && event.target !== inputDOM && !inputDOM.contains(event.target as Node)) { + // https://github.com/ant-design/ant-design/issues/56002 + // Tell `useSelectTriggerControl` to ignore this event + // When icon is dynamic render, the parentNode will miss + // so we need to mark the event directly + (event.nativeEvent as any)._ignore_global_close = true; event.preventDefault(); }这样可以将标记限制在非输入区域的点击（如后缀图标），更符合修复的原始意图。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SelectInput/index.tsx(1 hunks)src/hooks/useSelectTriggerControl.ts(1 hunks)tests/Select.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Select.test.tsx (1)
tests/utils/common.ts (1)
expectOpen(4-20)
🔇 Additional comments (2)
src/hooks/useSelectTriggerControl.ts (1)
22-25: 实现看起来正确全局关闭的防护逻辑正确地检查了
_ignore_global_close标记。这与 SelectInput 中的设置相匹配，可以防止在点击动态渲染元素时意外关闭。可选的类型安全改进建议：
如果将来需要重构，可以考虑定义一个类型扩展来避免as any：interface ExtendedMouseEvent extends MouseEvent { _ignore_global_close?: boolean; }不过对于当前实现，使用
as any是可以接受的快速解决方案。tests/Select.test.tsx (1)
392-430: 测试覆盖了核心场景此测试有效验证了动态后缀图标变化时下拉框不会关闭的场景，直接针对 issue #56002 中描述的问题。实现正确使用了相同的
mouseDownEvent来模拟事件从元素冒泡到 window。可选的额外测试场景：
如果要进一步加强覆盖，可以考虑添加以下场景的测试：
- 验证在非动态后缀（后缀不变化）的情况下点击也不会关闭
- 验证点击其他区域（如选项列表外部）仍然会正常关闭
不过对于当前的修复，这个测试已经足够。
![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 addresses the reported issue where a mouseDown event on a dynamically rendered suffix icon incorrectly closed the select dropdown. The solution involves marking the native event object to signal the global close handler to ignore it. While this approach resolves the immediate problem, directly modifying native event objects with any type casting can introduce fragility and reduce type safety. Consider alternative React-specific mechanisms for communicating this intent, such as using a context or a ref, for a more robust and maintainable solution in the future.
| // Tell `useSelectTriggerControl` to ignore this event | ||
| // When icon is dynamic render, the parentNode will miss | ||
| // so we need to mark the event directly | ||
| (event.nativeEvent as any)._ignore_global_close = true; |
There was a problem hiding this comment.
Directly modifying the native event object and casting it to any can lead to type safety issues and potential conflicts with other libraries that might also manipulate event objects. While this approach fixes the immediate bug, it relies on undocumented behavior and can be fragile. Consider using a more idiomatic React pattern, such as a context provider or a ref, to pass this _ignore_global_close flag to the useSelectTriggerControl hook.
| if ( | ||
| open && | ||
| // Marked by SelectInput mouseDown event | ||
| !(event as any)._ignore_global_close && |
There was a problem hiding this comment.
Accessing a custom property _ignore_global_close on the event object with (event as any) reduces type safety and couples this hook tightly to the implementation details of SelectInput. If the structure of the native event or the SelectInput component changes, this could break unexpectedly. A more robust solution would involve a clear, type-safe communication channel between the components, such as a shared state or a dedicated prop.
fix ant-design/ant-design#56002
Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.