fix(SelectTrigger): prevent supression of mousedown event#2688
fix(SelectTrigger): prevent supression of mousedown event#2688rastuhacode wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesSelect trigger click handling refactor
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
commit: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/Select/SelectTrigger.vue (1)
107-116: ⚡ Quick winAdd a regression test for window-level
mousedownpropagation.This change fixes a cross-framework integration path, but provided test context only shows open-on-pointerdown behavior. Please add/extend a test to assert a
window(including capture-phase)mousedownlistener still fires when clickingSelectTrigger.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/Select/SelectTrigger.vue` around lines 107 - 116, Extend the test suite for SelectTrigger to add a regression test that verifies a window-level mousedown listener (including capture phase) is still invoked when clicking the SelectTrigger component; specifically, render the SelectTrigger (or the parent Select component) in the test, attach a spy listener to window.addEventListener('mousedown', handler, { capture: true }) and ensure the handler is called after dispatching a click/mousedown on the SelectTrigger element, and include a case that covers the new inline mousedown handler which conditionally calls event.preventDefault() via isCtrlLeftClick so the test proves propagation to window is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/Select/SelectTrigger.vue`:
- Around line 107-116: Extend the test suite for SelectTrigger to add a
regression test that verifies a window-level mousedown listener (including
capture phase) is still invoked when clicking the SelectTrigger component;
specifically, render the SelectTrigger (or the parent Select component) in the
test, attach a spy listener to window.addEventListener('mousedown', handler, {
capture: true }) and ensure the handler is called after dispatching a
click/mousedown on the SelectTrigger element, and include a case that covers the
new inline mousedown handler which conditionally calls event.preventDefault()
via isCtrlLeftClick so the test proves propagation to window is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb61f0eb-7c2f-4c3b-b169-42c0e16dd590
📒 Files selected for processing (1)
packages/core/src/Select/SelectTrigger.vue
🔗 Linked issue
Resolves #1773
❓ Type of change
📚 Description
Issue
SelectTriggeropens the select onpointerdownand was callingevent.preventDefault()there to stop the trigger from taking focus away from the highlighted item in the listbox.Per the Pointer Events spec, calling
preventDefault()onpointerdowncancels the compatibility mouse events that follow —mousedown,mouseup, andclick.Fix
Focus prevention was moved from
pointerdowntomousedown:pointerdown— still opens the select, but no longer callspreventDefault(), so the browser emitsmousedown.mousedown— callspreventDefault()under the same conditions (left click, no Ctrl) to keep the trigger from stealing focus.Global mousedown listeners (including capture-phase ones on window) now fire before the trigger’s own mousedown handler runs.
📝 Checklist
Summary by CodeRabbit