-
Notifications
You must be signed in to change notification settings - Fork 800
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
[Select] Fix touch devices bugs #2939
Changes from all commits
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 |
---|---|---|
@@ -0,0 +1,5 @@ | ||
releases: | ||
"@radix-ui/react-select": patch | ||
|
||
declined: | ||
- primitives |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -220,6 +220,7 @@ const SelectTrigger = React.forwardRef<SelectTriggerElement, SelectTriggerProps> | |
const isDisabled = context.disabled || disabled; | ||
const composedRefs = useComposedRefs(forwardedRef, context.onTriggerChange); | ||
const getItems = useCollection(__scopeSelect); | ||
const pointerTypeRef = React.useRef<React.PointerEvent['pointerType']>('touch'); | ||
|
||
const [searchRef, handleTypeaheadSearch, resetTypeahead] = useTypeaheadSearch((search) => { | ||
const enabledItems = getItems().filter((item) => !item.disabled); | ||
|
@@ -230,12 +231,19 @@ const SelectTrigger = React.forwardRef<SelectTriggerElement, SelectTriggerProps> | |
} | ||
}); | ||
|
||
const handleOpen = () => { | ||
const handleOpen = (pointerEvent?: React.MouseEvent | React.PointerEvent) => { | ||
if (!isDisabled) { | ||
context.onOpenChange(true); | ||
// reset typeahead when we open | ||
resetTypeahead(); | ||
} | ||
|
||
if (pointerEvent) { | ||
context.triggerPointerDownPosRef.current = { | ||
x: Math.round(pointerEvent.pageX), | ||
y: Math.round(pointerEvent.pageY), | ||
}; | ||
} | ||
}; | ||
|
||
return ( | ||
|
@@ -262,8 +270,15 @@ const SelectTrigger = React.forwardRef<SelectTriggerElement, SelectTriggerProps> | |
// because we are preventing default in `onPointerDown` so effectively | ||
// this only runs for a label "click" | ||
event.currentTarget.focus(); | ||
|
||
// Open on click when using a touch or pen device | ||
if (pointerTypeRef.current !== 'mouse') { | ||
handleOpen(event); | ||
} | ||
})} | ||
onPointerDown={composeEventHandlers(triggerProps.onPointerDown, (event) => { | ||
pointerTypeRef.current = event.pointerType; | ||
|
||
// prevent implicit pointer capture | ||
// https://www.w3.org/TR/pointerevents3/#implicit-pointer-capture | ||
const target = event.target as HTMLElement; | ||
|
@@ -272,13 +287,10 @@ const SelectTrigger = React.forwardRef<SelectTriggerElement, SelectTriggerProps> | |
} | ||
|
||
// 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) { | ||
handleOpen(); | ||
context.triggerPointerDownPosRef.current = { | ||
x: Math.round(event.pageX), | ||
y: Math.round(event.pageY), | ||
}; | ||
// 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); | ||
Comment on lines
+290
to
+293
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. Retain handle open callback on pointerdown for mouse devices |
||
// prevent trigger from stealing focus from the active item after opening. | ||
event.preventDefault(); | ||
} | ||
|
@@ -1197,6 +1209,7 @@ const SelectItem = React.forwardRef<SelectItemElement, SelectItemProps>( | |
contentContext.itemRefCallback?.(node, value, disabled) | ||
); | ||
const textId = useId(); | ||
const pointerTypeRef = React.useRef<React.PointerEvent['pointerType']>('touch'); | ||
|
||
const handleSelect = () => { | ||
if (!disabled) { | ||
|
@@ -1242,11 +1255,24 @@ const SelectItem = React.forwardRef<SelectItemElement, SelectItemProps>( | |
ref={composedRefs} | ||
onFocus={composeEventHandlers(itemProps.onFocus, () => setIsFocused(true))} | ||
onBlur={composeEventHandlers(itemProps.onBlur, () => setIsFocused(false))} | ||
onPointerUp={composeEventHandlers(itemProps.onPointerUp, handleSelect)} | ||
onClick={composeEventHandlers(itemProps.onClick, () => { | ||
// Open on click when using a touch or pen device | ||
if (pointerTypeRef.current !== 'mouse') handleSelect(); | ||
})} | ||
onPointerUp={composeEventHandlers(itemProps.onPointerUp, () => { | ||
// Using a mouse you should be able to do pointer down, move through | ||
// the list, and release the pointer over the item to select it. | ||
if (pointerTypeRef.current === 'mouse') handleSelect(); | ||
})} | ||
Comment on lines
+1258
to
+1266
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. In case of #1658, what appears to happen when you click an item on a touch device is that touch and pointer events are fired on the Select Item correctly, but then also a click event is emulated, but by that time pointerup has fired and the select is closed, so the simulated click lands on the item behind Moving the handler that closes the select to onclick avoids that issue, but we keep pointerup for mouse devices to retain the ability to glide through the items without releasing the pointer (which has never worked on iOS anyway). |
||
onPointerDown={composeEventHandlers(itemProps.onPointerDown, (event) => { | ||
pointerTypeRef.current = event.pointerType; | ||
})} | ||
onPointerMove={composeEventHandlers(itemProps.onPointerMove, (event) => { | ||
// Remember pointer type when sliding over to this item from another one | ||
pointerTypeRef.current = event.pointerType; | ||
if (disabled) { | ||
contentContext.onItemLeave?.(); | ||
} else { | ||
} else if (pointerTypeRef.current === 'mouse') { | ||
// even though safari doesn't support this option, it's acceptable | ||
// as it only means it might scroll a few pixels when using the pointer. | ||
event.currentTarget.focus({ preventScroll: true }); | ||
Comment on lines
+1275
to
1278
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. Check for a mouse pointer is added because on iOS gliding through the items doesn't quite work the same way—focus gets stuck on the first item that received focus on pointer move and won't budge |
||
|
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.
Open on click for non-mouse devices
For posterity, about pen devices in this PR:
pointerType: 'pen'
, but behaves exactly like touch (e.g. opens the Select on pointerdown when intending to scroll the page)pointerType: 'pen'
, but behaves exactly like mouse (which don't need pointerdown to scroll, so the current behaviour isn't a problem in this case).Since we can't disambiguate the two, I'm being a bit more defensive here and assuming
pen
is equivalent totouch