Skip to content
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

Merged
merged 5 commits into from
Aug 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .yarn/versions/cc138cf5.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
releases:
"@radix-ui/react-select": patch

declined:
- primitives
10 changes: 8 additions & 2 deletions packages/react/select/src/Select.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -829,7 +829,9 @@ export const Cypress = () => {
</Select.Portal>
</Select.Root>
</Label>
<button type="submit">buy</button>
<button type="submit" style={{ width: 100, height: 50 }}>
buy
</button>
{data.size ? <p>You picked t-shirt size {data.size}</p> : null}
</form>

Expand Down Expand Up @@ -876,7 +878,7 @@ export const Cypress = () => {
</Select.Root>
</Label>

<button type="button" onClick={() => setModel('')}>
<button type="button" style={{ width: 100, height: 50 }} onClick={() => setModel('')}>
unset
</button>
</div>
Expand Down Expand Up @@ -1107,6 +1109,10 @@ const itemClass = css({
position: 'relative',
outline: 'none',

'&:active': {
backgroundColor: '$gray100',
},

'&[data-highlighted]': {
backgroundColor: '$black',
color: 'white',
Expand Down
46 changes: 36 additions & 10 deletions packages/react/select/src/Select.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 (
Expand All @@ -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);
}
Comment on lines +273 to +277
Copy link
Collaborator Author

@vladmoroz vladmoroz Jun 6, 2024

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:

  • Using an Apple Pencil on iOS yields pointerType: 'pen', but behaves exactly like touch (e.g. opens the Select on pointerdown when intending to scroll the page)
  • Using a Wacom tablets on macOS also yields 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 to touch

})}
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;
Expand All @@ -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
Copy link
Collaborator Author

@vladmoroz vladmoroz Jun 6, 2024

Choose a reason for hiding this comment

The 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();
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Copy link
Collaborator Author

@vladmoroz vladmoroz Jun 6, 2024

Choose a reason for hiding this comment

The 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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

Expand Down
Loading