afrokick commented 1 year ago

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

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

yxeri commented 1 year ago

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.

jlputter commented 1 year ago

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.

benoitgrelard commented 1 year ago

@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!

✌️

jlputter commented 1 year ago

@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.

mclngl commented 1 year ago

@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.

benoitgrelard commented 1 year ago

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

HoHoangThai1807 commented 1 year ago

Any updates for this issue, guys?

myrjola commented 1 year ago

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.

ghostlexly commented 9 months ago

Bump.. This issue still exists..

miguelmalo12 commented 9 months ago

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

itsramiel commented 9 months ago

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

samkevin1 commented 5 months ago

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

hauseyo commented 4 months ago

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!

tomcru commented 4 months ago

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>
  );
};

rayyamhk commented 1 month ago

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

Wrap the <Select /> component by a context provider


const SelectContext = React.createContext<
React.Dispatch<React.SetStateAction<boolean>>
>(() => {});

const Select: React.FC = (props) => { const [isOpen, setIsOpen] = React.useState(false); return (

); }; ``` 2. Override the default behavior of `` ``` const SelectTrigger = ... => { const setIsOpen = React.useContext(SelectContext); return ( { 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 } }} ... > ... ); }); ```

justAnArthur commented 1 month ago

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

antoniomarcelob commented 6 days ago

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

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>
  );
};

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

radix-ui / primitives

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