fix(Select): improve touch/pointer handling in select #2329
Conversation
commit: |
Screen_Recording_20251210_013143_Chrome.mp4Desktop
Screen.Recording.2025-12-10.at.1.45.21.AM.mov |
|
Please also help me again update the test files, cause I lowkey don't like tests 😁 |
zernonia
force-pushed
the
fix-select-touch
branch
from
f4d7039 to
5f07e30
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes refactor the Select component's pointer and touch interaction handling across multiple files by introducing device-type awareness (mouse vs. touch/pen). The SelectContent component is simplified by removing reactive helpers and adding a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SelectTrigger
participant SelectItem
participant SelectContent
participant FocusScope
Note over User,FocusScope: Mouse Interaction Flow
User->>SelectTrigger: pointerdown (mouse)
SelectTrigger->>SelectTrigger: set pointerTypeRef = 'mouse'<br/>call handleOpen
SelectTrigger->>SelectContent: open menu
User->>SelectItem: pointermove (mouse)
SelectItem->>SelectItem: focus item
User->>SelectItem: pointerup (mouse)
SelectItem->>SelectItem: trigger selection sync
SelectTrigger->>SelectContent: close menu
Note over User,FocusScope: Touch Interaction Flow
User->>SelectTrigger: pointerdown (touch)
SelectTrigger->>SelectTrigger: set pointerTypeRef = 'touch'<br/>skip open on pointerdown
User->>SelectTrigger: click (touch)
SelectTrigger->>SelectContent: open menu
SelectContent->>FocusScope: bind trapped=true
User->>SelectItem: click (touch)
SelectItem->>SelectItem: trigger selection sync
SelectTrigger->>SelectContent: close menu
SelectContent->>FocusScope: bind trapped=false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 Fix all issues with AI agents
In @packages/core/src/Select/SelectItem.vue:
- Around line 174-179: The click handler is unsafely casting MouseEvent to
PointerEvent; update the signature of handleSelectCustomEvent to accept (event:
MouseEvent | PointerEvent) and remove the cast in the @click handler, then audit
all calls to handleSelectCustomEvent to ensure any PointerEvent-only properties
are accessed behind type guards (e.g., narrow by checking 'pointerId' in the
event) so both event types are handled safely while keeping pointerTypeRef logic
intact.
In @packages/core/src/Select/SelectTrigger.vue:
- Around line 76-88: The click handler in SelectTrigger.vue is incorrectly typed
as PointerEvent; change the event parameter type to MouseEvent in the @click
handler and update any related casts to HTMLElement accordingly (refer to the
@click handler, pointerTypeRef and handleOpen identifiers), and make the
analogous change in SelectItem.vue where an event is cast (replace PointerEvent
with MouseEvent for that cast) so the runtime types match the declared types.
In @packages/core/src/Select/story/SelectForm.story.vue:
- Around line 60-61: The SelectContent element has a typo in its state selector:
it uses data-[state=close] which never matches the component's data-state (which
is 'open' or 'closed'); update the class on the SelectContent JSX/Template (the
element named SelectContent in this diff) to replace data-[state=close]:... with
data-[state=closed]:... so the
animate-[slide-in_150ms_cubic-bezier(0.16,1,0.3,1)] rule applies when
SelectContent's state is closed.
🧹 Nitpick comments (1)
packages/core/src/Select/story/SelectDemo.story.vue (1)
114-123: Consider removing unused CSS transition styles.The
.v-enter-active,.v-leave-active,.v-enter-from, and.v-leave-toclasses were used with the<Transition>wrapper that has been removed. These styles are now dead code unless there's a plan to reintroduce transitions.Remove unused styles
</template> - -<style scoped> -.v-enter-active, -.v-leave-active { - transition: opacity .3s ease; -} - -.v-enter-from, -.v-leave-to { - opacity: 0; -} -</style>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/src/Select/SelectContent.vuepackages/core/src/Select/SelectContentImpl.vuepackages/core/src/Select/SelectItem.vuepackages/core/src/Select/SelectTrigger.vuepackages/core/src/Select/story/SelectDemo.story.vuepackages/core/src/Select/story/SelectForm.story.vue
🔇 Additional comments (11)
packages/core/src/Select/SelectTrigger.vue (3)
26-28: LGTM on pointer type tracking initialization.The
pointerTypeRefinitialization to'touch'is a safe default that ensures non-mouse behavior until a pointer event explicitly sets the type. The eslint disable is appropriate since the variable must be reassigned.
103-109: Good implementation of device-aware pointer handling.The logic correctly differentiates between mouse and touch/pen devices:
- Mouse: opens on
pointerdownwith left button, no ctrl key- Touch/Pen: defers to
clickhandler to avoid opening on scrollThis aligns with radix-ui's approach for handling touch devices appropriately.
116-117: Defensive null check ongetItems()is appropriate.The
?? []fallback ensureshandleTypeaheadSearchwon't fail ifgetItems()returns null/undefined.packages/core/src/Select/SelectItem.vue (4)
70-72: Consistent pointer type tracking across components.The initialization pattern matches
SelectTrigger.vue, ensuring consistent behavior. Defaulting to'touch'is safe as it prevents mouse-specific behavior until confirmed.
73-91: Good refactor: async handlers converted to synchronous.Removing
async/awaitandnextTickfrom these handlers reduces unnecessary overhead. The early return onev.defaultPreventedis correctly preserved.
180-190: Device-aware selection logic is well-implemented.The split behavior is correct:
pointerupfor mouse enables drag-to-select (pointer down on trigger, drag to item, release to select)clickfor touch/pen prevents accidental selection during scrollThe
pointerdownhandler correctly captures the pointer type before other events fire.
93-108: Good refinement: Focus only on mouse pointer move.Restricting focus behavior to mouse interactions prevents scroll interference on touch devices. The comment correctly notes Safari's limitation with
preventScroll.packages/core/src/Select/SelectContentImpl.vue (2)
140-160: Well-implemented touch-aware pointer handling.The touch device flow correctly:
- Tracks pointer type on
pointermoveto catch sliding interactions- Exits early for touch on
pointerupwithout checking movement threshold- Cleans up the
pointermovelistener and resetstriggerPointerDownPosRefThis prevents the drag-distance threshold check from interfering with touch selection, which is handled via
clickinSelectItem.vue.
282-282: Focus trap correctly bound to open state.Binding
:trapped="rootContext.open.value"ensures focus is only trapped while the select is open, preventing focus issues when closed.packages/core/src/Select/story/SelectDemo.story.vue (1)
55-106: Good demo update showing the refactored Select component.The restructured demo now demonstrates:
- Both
item-alignedandpopperpositions- Multiple groups (Fruits and Vegetables)
- Scroll buttons, separators, and item indicators
- Disabled item handling
This provides better coverage for testing the touch/pointer improvements.
packages/core/src/Select/SelectContent.vue (1)
43-62: Clean refactor of presence/rendering logic.The updated pattern is cleaner:
PresencewithforceMountprop always renders its slotpresentprop passed to Presence is computed fromforceMount || rootContext.open.value- Slot receives
{ present }from theusePresencehook, which tracks animation stateSelectContentImplrenders only whenpresentTeleporthandles the closed state to preserve slot content in a fragmentThis enables better integration with Vue animation libraries while maintaining proper content lifecycle.
| @click="(event: MouseEvent) => { | ||
| // Open on click when using a touch or pen device | ||
| if (pointerTypeRef !== 'mouse') { | ||
| handleSelectCustomEvent(event as unknown as PointerEvent) | ||
| } | ||
| }" |
There was a problem hiding this comment.
Unsafe type cast from MouseEvent to PointerEvent.
The click event provides a MouseEvent, which is cast to PointerEvent. While this works at runtime because handleSelectCustomEvent only accesses common properties, the cast is technically incorrect and could cause issues if the handler later accesses PointerEvent-specific properties like pointerId or pressure.
Suggested fix: Update function signature to accept both types
-function handleSelectCustomEvent(ev: PointerEvent | KeyboardEvent) {
+function handleSelectCustomEvent(ev: PointerEvent | MouseEvent | KeyboardEvent) {
if (ev.defaultPrevented)
return
const eventDetail = { originalEvent: ev, value: props.value as T }
handleAndDispatchCustomEvent(SELECT_SELECT, handleSelect, eventDetail)
}Then remove the cast:
@click="(event: MouseEvent) => {
// Open on click when using a touch or pen device
if (pointerTypeRef !== 'mouse') {
- handleSelectCustomEvent(event as unknown as PointerEvent)
+ handleSelectCustomEvent(event)
}
}"🤖 Prompt for AI Agents
In @packages/core/src/Select/SelectItem.vue around lines 174 - 179, The click
handler is unsafely casting MouseEvent to PointerEvent; update the signature of
handleSelectCustomEvent to accept (event: MouseEvent | PointerEvent) and remove
the cast in the @click handler, then audit all calls to handleSelectCustomEvent
to ensure any PointerEvent-only properties are accessed behind type guards
(e.g., narrow by checking 'pointerId' in the event) so both event types are
handled safely while keeping pointerTypeRef logic intact.
| @click=" | ||
| (event: MouseEvent) => { | ||
| (event: PointerEvent) => { | ||
| // Whilst browsers generally have no issue focusing the trigger when clicking | ||
| // on a label, Safari seems to struggle with the fact that there's no `onClick`. | ||
| // We force `focus` in this case. Note: this doesn't create any other side-effect | ||
| // because we are preventing default in `onPointerDown` so effectively | ||
| // this only runs for a label 'click' | ||
| (event?.currentTarget as HTMLElement)?.focus(); | ||
| // Open on click when using a touch or pen device | ||
| if (pointerTypeRef !== 'mouse') { | ||
| handleOpen(event); | ||
| } |
There was a problem hiding this comment.
Type mismatch: click event is MouseEvent, not PointerEvent.
The @click handler receives a MouseEvent, but it's typed as PointerEvent. While both have pageX/pageY properties, the type annotation is incorrect. Additionally, the cast at line 177 in SelectItem.vue has a similar pattern.
Suggested fix
@click="
- (event: PointerEvent) => {
+ (event: MouseEvent) => {
// Whilst browsers generally have no issue focusing the trigger when clicking
// on a label, Safari seems to struggle with the fact that there's no `onClick`.
// We force `focus` in this case. Note: this doesn't create any other side-effect
// because we are preventing default in `onPointerDown` so effectively
// this only runs for a label 'click'
(event?.currentTarget as HTMLElement)?.focus();
// Open on click when using a touch or pen device
if (pointerTypeRef !== 'mouse') {
handleOpen(event);
}
}
"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @click=" | |
| (event: MouseEvent) => { | |
| (event: PointerEvent) => { | |
| // Whilst browsers generally have no issue focusing the trigger when clicking | |
| // on a label, Safari seems to struggle with the fact that there's no `onClick`. | |
| // We force `focus` in this case. Note: this doesn't create any other side-effect | |
| // because we are preventing default in `onPointerDown` so effectively | |
| // this only runs for a label 'click' | |
| (event?.currentTarget as HTMLElement)?.focus(); | |
| // Open on click when using a touch or pen device | |
| if (pointerTypeRef !== 'mouse') { | |
| handleOpen(event); | |
| } | |
| @click=" | |
| (event: MouseEvent) => { | |
| // Whilst browsers generally have no issue focusing the trigger when clicking | |
| // on a label, Safari seems to struggle with the fact that there's no `onClick`. | |
| // We force `focus` in this case. Note: this doesn't create any other side-effect | |
| // because we are preventing default in `onPointerDown` so effectively | |
| // this only runs for a label 'click' | |
| (event?.currentTarget as HTMLElement)?.focus(); | |
| // Open on click when using a touch or pen device | |
| if (pointerTypeRef !== 'mouse') { | |
| handleOpen(event); | |
| } |
🤖 Prompt for AI Agents
In @packages/core/src/Select/SelectTrigger.vue around lines 76 - 88, The click
handler in SelectTrigger.vue is incorrectly typed as PointerEvent; change the
event parameter type to MouseEvent in the @click handler and update any related
casts to HTMLElement accordingly (refer to the @click handler, pointerTypeRef
and handleOpen identifiers), and make the analogous change in SelectItem.vue
where an event is cast (replace PointerEvent with MouseEvent for that cast) so
the runtime types match the declared types.
| <SelectContent | ||
| class="min-w-[160px] bg-white overflow-hidden rounded shadow-[0px_10px_38px_-10px_rgba(22,_23,_24,_0.35),_0px_10px_20px_-15px_rgba(22,_23,_24,_0.2)] will-change-[opacity,transform] data-[side=top]:animate-slideDownAndFade data-[side=right]:animate-slideLeftAndFade data-[side=bottom]:animate-slideUpAndFade data-[side=left]:animate-slideRightAndFade" | ||
| class="min-w-[160px] bg-white overflow-hidden rounded shadow-[0px_10px_38px_-10px_rgba(22,_23,_24,_0.35),_0px_10px_20px_-15px_rgba(22,_23,_24,_0.2)] will-change-[opacity,transform] data-[side=top]:animate-slideDownAndFade data-[side=right]:animate-slideLeftAndFade data-[side=bottom]:animate-slideUpAndFade data-[side=left]:animate-slideRightAndFade data-[state=close]:animate-[slide-in_150ms_cubic-bezier(0.16,1,0.3,1)] data-[state=closed]:animate-scaleOut" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check what data-state values are used in the Select component
rg -n "data-state" packages/core/src/Select/ -A 2 -B 2Repository: unovue/reka-ui
Length of output: 2290
🏁 Script executed:
# Also check SelectContentImpl.vue specifically around line 321
cat -n packages/core/src/Select/SelectContentImpl.vue | sed -n '315,330p'Repository: unovue/reka-ui
Length of output: 710
🏁 Script executed:
# Verify the exact context in SelectForm.story.vue
cat -n packages/core/src/Select/story/SelectForm.story.vue | sed -n '55,70p'Repository: unovue/reka-ui
Length of output: 1122
Fix typo: data-[state=close] should be data-[state=closed]
The data-state attribute is set to either 'open' or 'closed' (see SelectContentImpl.vue line 321). The class selector data-[state=close]:animate-[slide-in_150ms_cubic-bezier(0.16,1,0.3,1)] will never match. This should be data-[state=closed] to match the valid state value.
🤖 Prompt for AI Agents
In @packages/core/src/Select/story/SelectForm.story.vue around lines 60 - 61,
The SelectContent element has a typo in its state selector: it uses
data-[state=close] which never matches the component's data-state (which is
'open' or 'closed'); update the class on the SelectContent JSX/Template (the
element named SelectContent in this diff) to replace data-[state=close]:... with
data-[state=closed]:... so the
animate-[slide-in_150ms_cubic-bezier(0.16,1,0.3,1)] rule applies when
SelectContent's state is closed.
To follow the
radix-uiSelect component code for touch devices#2239 flicker fix is from here (Cherry-picked, Co-Authored applied, but there is no avatar)
#1472
Summary by CodeRabbit
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.