primitives icon indicating copy to clipboard operation
primitives copied to clipboard

[Select][Mobile] Select became opened while scrolling page

Open afrokick opened this issue 2 years ago • 17 comments

Bug report

Current Behavior

When a mobile user scroll the page touching Select, Select became opened. But the user just want to scroll page down. Not open Select.

Expected behavior

Select should not be opened when user scroll the page event if touch was started on Select.

Reproducible example

CodeSandbox Template

Suggested solution

As mentioned in https://github.com/radix-ui/primitives/pull/1082#issuecomment-1230197898 we may use onPointerUp/onClick instead of onPointerDown in https://github.com/radix-ui/primitives/blob/main/packages/react/select/src/Select.tsx#L244

Additional context

Sorry for scaling...

https://user-images.githubusercontent.com/3282725/187749713-210f089f-e238-4773-a28e-8561ff20474b.mp4

Your environment

Software Name(s) Version
Radix Package(s) @radix-ui/react-select 1.0.0
Browser Chrome 105, Safari 15.6 Chrome was emulated via Devices, Safari was checked on iPhone

afrokick avatar Aug 31 '22 18:08 afrokick

We stumbled upon this too in our current project. Why is it marked as an enhancement? It feels more like a bug. It goes against the philosophy of doing it like in macOS and makes it very easy to accidentally trigger. The example page (https://www.radix-ui.com/docs/primitives/components/select) exhibits the same behaviour.

yxeri avatar Sep 22 '22 13:09 yxeri

I have a workaround/fix of this issue, which I will make into a PR soon if possibile.

The idea here is to make it so that you can use onClick on the trigger instead of the onPointerDown event it uses by default. To do this you will have to navigate to the node-modules/@radix-ui/react-slider/dist folder and open the index.modules.js file. In that file you want to add this to the SelectTrigger portion of the code:

`/* -------------------------------------------------------------------------------------------------

  • SelectTrigger

  • -----------------------------------------------------------------------------------------------/ const $cc7e05a45900e73f$var$TRIGGER_NAME = 'SelectTrigger'; const $cc7e05a45900e73f$export$3ac1e88a1c0b9f1 = /#PURE*/ $01b9c$forwardRef((props, forwardedRef)=>{ const { __scopeSelect: __scopeSelect , disabled: disabled = false , 'aria-labelledby': ariaLabelledby , ...triggerProps } = props; const context = $cc7e05a45900e73f$var$useSelectContext($cc7e05a45900e73f$var$TRIGGER_NAME, __scopeSelect); const composedRefs = $01b9c$useComposedRefs(forwardedRef, context.onTriggerChange); const getItems = $cc7e05a45900e73f$var$useCollection(__scopeSelect); const labelId = $01b9c$useLabelContext(context.trigger); const labelledBy = ariaLabelledby || labelId; const [searchRef, handleTypeaheadSearch, resetTypeahead] = $cc7e05a45900e73f$var$useTypeaheadSearch((search)=>{ const enabledItems = getItems().filter((item)=>!item.disabled ); const currentItem = enabledItems.find((item)=>item.value === context.value ); const nextItem = $cc7e05a45900e73f$var$findNextItem(enabledItems, search, currentItem); if (nextItem !== undefined) context.onValueChange(nextItem.value); }); const handleOpen = ()=>{ if (!disabled) { context.onOpenChange(true); // reset typeahead when we open resetTypeahead(); } };

    return /#PURE/ $01b9c$createElement($01b9c$Primitive.button, $01b9c$babelruntimehelpersesmextends({ type: "button", role: "combobox", "aria-controls": context.contentId, "aria-expanded": context.open, "aria-autocomplete": "none", "aria-labelledby": labelledBy, dir: context.dir, "data-state": context.open ? 'open' : 'closed', disabled: disabled, "data-disabled": disabled ? '' : undefined, "data-placeholder": context.value === undefined ? '' : undefined }, triggerProps, { ref: composedRefs, onPointerDown: $01b9c$composeEventHandlers(triggerProps.onPointerDown, (event)=>{ // prevent implicit pointer capture // https://www.w3.org/TR/pointerevents3/#implicit-pointer-capture event.target.releasePointerCapture(event.pointerId); // 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) }; // prevent trigger from stealing focus from the active item after opening. event.preventDefault(); } }), onKeyDown: $01b9c$composeEventHandlers(triggerProps.onKeyDown, (event)=>{ const isTypingAhead = searchRef.current !== ''; const isModifierKey = event.ctrlKey || event.altKey || event.metaKey; if (!isModifierKey && event.key.length === 1) handleTypeaheadSearch(event.key); if (isTypingAhead && event.key === ' ') return; if ($cc7e05a45900e73f$var$OPEN_KEYS.includes(event.key)) { handleOpen(); event.preventDefault(); } }), onClick: $01b9c$composeEventHandlers(triggerProps.onClick, (event)=>{ handleOpen(); }) })); });`

Once you have added this to your code, you want to tell the trigger to preventDefault onPointerDown if you are on mobile, which will then allow for onClick to work (onClick will trigger the select unless explicitly tapped).

Using patch-package you are able to use this fix in your own project.

jlputter avatar Oct 26 '22 09:10 jlputter

@jlputter, whilst this might resolve the issue on the surface, it will break other behaviours (like the ability to select a value in one click lifecycle (down, move, up).

…which I will make into a PR soon if possibile.

I am mostly letting you know as I don't want you to waste some of your personal time working on a PR for a fix I know we would reject.

Hope that makes sense!

✌️

benoitgrelard avatar Oct 26 '22 10:10 benoitgrelard

@benoitgrelard absolutely. If I were to make a PR it would be a fix to the onPointerDown, not by just adding an OnClick. I do think, however, that having the possibility for a simple onClick would not be bad either.

jlputter avatar Oct 26 '22 10:10 jlputter

@benoitgrelard

it will break other behaviours (like the ability to select a value in one click lifecycle (down, move, up).

I understand but I do think that it's way better to force the user to click two times in order to select a value than having this blocking scroll issue on mobile.

mclngl avatar Oct 26 '22 13:10 mclngl

Yes the issue needs to be fixed for sure. In the meantime you are welcome to modify the component to your needs.

benoitgrelard avatar Oct 26 '22 14:10 benoitgrelard

Any updates for this issue, guys?

HoHoangThai1807 avatar Nov 24 '22 08:11 HoHoangThai1807

I did some digging on how react-aria does it. They have an event abstraction

https://github.com/adobe/react-spectrum/blob/b35d5c02fe900badccd0cf1a8f23bb593419f238/packages/%40react-aria/menu/src/useMenuTrigger.ts#L106-L118

  let pressProps =  {
    onPressStart(e) {
      // For consistency with native, open the menu on mouse/key down, but touch up.
      if (e.pointerType !== 'touch' && e.pointerType !== 'keyboard' && !isDisabled) {
        // If opened with a screen reader, auto focus the first item.
        // Otherwise, the menu itself will be focused.
        state.toggle(e.pointerType === 'virtual' ? 'first' : null);
      }
    },
    onPress(e) {
      if (e.pointerType === 'touch' && !isDisabled) {
        state.toggle();
      }
    }
  };

These non-native events are plumbed through useButton https://react-spectrum.adobe.com/react-aria/useButton.html

It was not trivial to implement this to Radix UI Select. I was able to prevent opening the select when touch scrolling in https://github.com/radix-ui/primitives/commit/552650931a57b3316dd488bbe1543da80d9ba6d6

However, trying to open the select on touch devices was something I couldn't get to work reliably. I don't know when I would have the time to take a stab at this the next time, but hopefully someone else wants to pick this up in the meanwhile.

myrjola avatar Nov 30 '22 14:11 myrjola

Bump.. This issue still exists..

ghostlexly avatar Sep 12 '23 09:09 ghostlexly

Any update on this issue? Does it have an easy fix?

miguelmalo12 avatar Sep 15 '23 01:09 miguelmalo12

I used patch package to trigger on onClick and comment out onPointerDown:

diff --git a/node_modules/@radix-ui/react-select/dist/index.mjs b/node_modules/@radix-ui/react-select/dist/index.mjs
index 53fb4de..aaa1d08 100644
--- a/node_modules/@radix-ui/react-select/dist/index.mjs
+++ b/node_modules/@radix-ui/react-select/dist/index.mjs
@@ -194,23 +194,24 @@ const $cc7e05a45900e73f$export$3ac1e88a1c0b9f1 = /*#__PURE__*/ $01b9c$forwardRef
             // because we are preventing default in `onPointerDown` so effectively
             // this only runs for a label "click"
             event.currentTarget.focus();
+            handleOpen()
         }),
-        onPointerDown: $01b9c$composeEventHandlers(triggerProps.onPointerDown, (event)=>{
-            // prevent implicit pointer capture
-            // https://www.w3.org/TR/pointerevents3/#implicit-pointer-capture
-            const target = event.target;
-            if (target.hasPointerCapture(event.pointerId)) target.releasePointerCapture(event.pointerId);
-             // 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)
-                }; // prevent trigger from stealing focus from the active item after opening.
-                event.preventDefault();
-            }
-        }),
+        // onPointerDown: $01b9c$composeEventHandlers(triggerProps.onPointerDown, (event)=>{
+        //     // prevent implicit pointer capture
+        //     // https://www.w3.org/TR/pointerevents3/#implicit-pointer-capture
+        //     const target = event.target;
+        //     if (target.hasPointerCapture(event.pointerId)) target.releasePointerCapture(event.pointerId);
+        //      // 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)
+        //         }; // prevent trigger from stealing focus from the active item after opening.
+        //         event.preventDefault();
+        //     }
+        // }),
         onKeyDown: $01b9c$composeEventHandlers(triggerProps.onKeyDown, (event)=>{
             const isTypingAhead = searchRef.current !== '';
             const isModifierKey = event.ctrlKey || event.altKey || event.metaKey;

Not sure about the downsides, but seems to be good

itsramiel avatar Sep 23 '23 07:09 itsramiel

Any updates on this? Or any workarounds? The issue is still there

samkevin1 avatar Jan 30 '24 12:01 samkevin1

Hi. Is there any plan on fixing this? We would really like to have some solutions on preventing drop down opening during the scroll/touch Thanks!

hauseyo avatar Feb 17 '24 20:02 hauseyo

Workaround

You can fix this by overwriting onPointerDown to {(e) => e.preventDefault()} on the <DropdownMenu.Trigger /> & adding an onClick event & onOpenChange.

Before:

return (
  <DropdownMenu.Root>
    <DropdownMenu.Trigger>
      {/* trigger content */}
    </DropdownMenu.Trigger>
    {/* other content */}
  </DropdownMenu.Root>
);

After:

const [isOpen, setIsOpen] = useState(false);

return (
  <DropdownMenu.Root open={isOpen} onOpenChange={setIsOpen}>
    <DropdownMenu.Trigger
      onPointerDown={(e) => e.preventDefault()}
      onClick={() => setIsOpen((prev) => !prev)}
    >
      {/* trigger content */}
    </DropdownMenu.Trigger>
    {/* other content */}
  </DropdownMenu.Root>
);

This makes the Dropdown work onClick and scrolling events do not trigger it. Try out the video example ↗

https://github.com/radix-ui/primitives/assets/35841182/617fe834-fd3d-440b-96d3-9f9e3679a1b7

Here's a trimmed down version of the Dropdown component from the video:

export const Dropdown = ({ children, text, id, ...props }: Props) => {
  const [isOpen, setIsOpen] = useState(false);

  return (
    <DropdownMenuPrimitive.Root open={isOpen} onOpenChange={setIsOpen}>
      <DropdownMenuPrimitive.Trigger asChild>
        <button
          className={}
          onPointerDown={(e) => e.preventDefault()}
          onClick={() => setIsOpen((prev) => !prev)}
          id={id}
          {...props}
        >
          {text}
        </button>
      </DropdownMenuPrimitive.Trigger>
      <DropdownMenuPrimitive.Portal>
        <DropdownMenuPrimitive.Content
          {...props}
          className={}
          sideOffset={8}
          align="end"
        >
          {children}
        </DropdownMenuPrimitive.Content>
      </DropdownMenuPrimitive.Portal>
    </DropdownMenuPrimitive.Root>
  );
};

tomcru avatar Feb 20 '24 00:02 tomcru

In case you're coming from shadcn/ui, here is my workaround

  1. Wrap the <Select /> component by a context provider
const SelectContext = React.createContext<
  React.Dispatch<React.SetStateAction<boolean>>
>(() => {});

const Select: React.FC<SelectPrimitive.SelectProps> = (props) => {
  const [isOpen, setIsOpen] = React.useState(false);
  return (
    <SelectContext.Provider value={setIsOpen}>
      <SelectPrimitive.Root open={isOpen} onOpenChange={setIsOpen} {...props} />
    </SelectContext.Provider>
  );
};
  1. Override the default behavior of <SelectTrigger />
const SelectTrigger = ... => {
  const setIsOpen = React.useContext(SelectContext);
  return (
    <SelectPrimitive.Trigger
      onPointerDown={(e) => {
        if (e.pointerType === "touch") e.preventDefault(); // disable the default behavior in mobile
      }}
      onPointerUp={(e) => {
        if (e.pointerType === "touch") {
          setIsOpen((prevState) => !prevState); // use onPointerUp to simulate onClick in mobile
        }
      }}
      ...
    >
      ...
    </SelectPrimitive.Trigger>
  );
});

rayyamhk avatar May 14 '24 17:05 rayyamhk

Difference between Aug 31, 2022 and May 25, 2024: 1 years 8 months 25 days or 20 months 25 days or 90 weeks 3 days or 633 calendar days

justAnArthur avatar May 25 '24 08:05 justAnArthur

In case you're coming from shadcn/ui, here is my workaround

  1. Wrap the <Select /> component by a context provider
const SelectContext = React.createContext<
  React.Dispatch<React.SetStateAction<boolean>>
>(() => {});

const Select: React.FC<SelectPrimitive.SelectProps> = (props) => {
  const [isOpen, setIsOpen] = React.useState(false);
  return (
    <SelectContext.Provider value={setIsOpen}>
      <SelectPrimitive.Root open={isOpen} onOpenChange={setIsOpen} {...props} />
    </SelectContext.Provider>
  );
};
  1. Override the default behavior of <SelectTrigger />
const SelectTrigger = ... => {
  const setIsOpen = React.useContext(SelectContext);
  return (
    <SelectPrimitive.Trigger
      onPointerDown={(e) => {
        if (e.pointerType === "touch") e.preventDefault(); // disable the default behavior in mobile
      }}
      onPointerUp={(e) => {
        if (e.pointerType === "touch") {
          setIsOpen((prevState) => !prevState); // use onPointerUp to simulate onClick in mobile
        }
      }}
      ...
    >
      ...
    </SelectPrimitive.Trigger>
  );
});

nailed it

antoniomarcelob avatar Jun 29 '24 00:06 antoniomarcelob