-
-
Notifications
You must be signed in to change notification settings - Fork 502
fix(Select): improve touch/pointer handling in select #2329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v2
Are you sure you want to change the base?
Changes from all commits
ccd03d7
14326be
dd3e97a
5f07e30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,10 @@ const props = withDefaults(defineProps<SelectTriggerProps>(), { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const rootContext = injectSelectRootContext() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { forwardRef, currentElement: triggerElement } = useForwardExpose() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Track pointer type to differentiate between mouse and touch/pen | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // eslint-disable-next-line prefer-const | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let pointerTypeRef: PointerEvent['pointerType'] = 'touch' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const isDisabled = computed(() => rootContext.disabled?.value || props.disabled) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| rootContext.contentId ||= useId(undefined, 'reka-select-content') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -32,19 +36,19 @@ onMounted(() => { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { getItems } = useCollection() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { search, handleTypeaheadSearch, resetTypeahead } = useTypeahead() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function handleOpen() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function handleOpen(pointerEvent?: MouseEvent | PointerEvent) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!isDisabled.value) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| rootContext.onOpenChange(true) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // reset typeahead when we open | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resetTypeahead() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function handlePointerOpen(event: PointerEvent) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handleOpen() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| rootContext.triggerPointerDownPosRef.value = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| x: Math.round(event.pageX), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| y: Math.round(event.pageY), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (pointerEvent) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| rootContext.triggerPointerDownPosRef.value = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| x: Math.round(pointerEvent.pageX), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| y: Math.round(pointerEvent.pageY), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </script> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -70,21 +74,23 @@ function handlePointerOpen(event: PointerEvent) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :as-child="asChild" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| :as="as" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
76
to
+88
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type mismatch: The 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @pointerdown=" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (event: PointerEvent) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Prevent opening on touch down. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // https://github.com/unovue/reka-ui/issues/804 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (event.pointerType === 'touch') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return event.preventDefault(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pointerTypeRef = event.pointerType; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // prevent implicit pointer capture | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // https://www.w3.org/TR/pointerevents3/#implicit-pointer-capture | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -94,30 +100,21 @@ function handlePointerOpen(event: PointerEvent) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // only call handler if it's the left button (mousedown gets triggered by all mouse buttons) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // but not when the control key is pressed (avoiding MacOS right click) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (event.button === 0 && event.ctrlKey === false) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handlePointerOpen(event) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // but not when the control key is pressed (avoiding MacOS right click); also not for touch | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // devices because that would open the menu on scroll. (pen devices behave as touch on iOS). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (event.button === 0 && event.ctrlKey === false && event.pointerType === 'mouse') { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handleOpen(event); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // prevent trigger from stealing focus from the active item after opening. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| event.preventDefault(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @pointerup.prevent=" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (event: PointerEvent) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Only open on pointer up when using touch devices | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // https://github.com/unovue/reka-ui/issues/804 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (event.pointerType === 'touch') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handlePointerOpen(event) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @keydown=" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (event) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (event: KeyboardEvent) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const isTypingAhead = search !== ''; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const isModifierKey = event.ctrlKey || event.altKey || event.metaKey; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!isModifierKey && event.key.length === 1) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (isTypingAhead && event.key === ' ') return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handleTypeaheadSearch(event.key, getItems()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!isModifierKey && event.key.length === 1) handleTypeaheadSearch(event.key, getItems() ?? []); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (isTypingAhead && event.key === ' ') return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (OPEN_KEYS.includes(event.key)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handleOpen(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| event.preventDefault(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsafe type cast from
MouseEventtoPointerEvent.The
clickevent provides aMouseEvent, which is cast toPointerEvent. While this works at runtime becausehandleSelectCustomEventonly accesses common properties, the cast is technically incorrect and could cause issues if the handler later accessesPointerEvent-specific properties likepointerIdorpressure.Suggested fix: Update function signature to accept both types
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