radix-ui / primitives

Radix Primitives is an open-source UI component library for building high-quality, accessible design systems and web apps. Maintained by @workos.
https://radix-ui.com/primitives
MIT License
15.49k stars 785 forks source link

[DropdownMenu] `DropdownMenu.Trigger` reacts to scrolling on mobile devices #1912

Open TmLev opened 1 year ago

TmLev commented 1 year ago

Bug report

Current Behavior

Scrolling on mobile devices may trigger DropdownMenu.Trigger even when not intended. In my case, I have a table where rows have DropdownMenu.Trigger as a last column. Scrolling with right thumb almost always triggers some row's dropdown menu.

https://user-images.githubusercontent.com/22966523/215288503-eb96c5c3-1b14-4ad3-b877-3956c3c092c1.mov

Expected behavior

Scrolling shouldn't act as a regular tap.

Reproducible example

https://www.radix-ui.com/docs/primitives/components/dropdown-menu#trigger

Toggle responsive mode.

Suggested solution

This may be hard, I don't know. Track movement and open menu only when touch cursor doesn't move far enough?..

Additional context

Nothing.

Your environment

Software Name(s) Version
Radix Package(s) @radix-ui/react-dropdown-menu 2.0.2 (latest)
React n/a 18.2
Browser Chrome Any recent one
Assistive tech
Node n/a
npm/yarn
Operating System Android and macOS 13 / 12
Fensterbank commented 10 months ago

I'm facing the same issue in my app. The behavior seems to be caused by using onPointerDown event. https://github.com/radix-ui/primitives/blob/c31c97274ff357aea99afe6c01c1c8c58b6356e0/packages/react/dropdown-menu/src/DropdownMenu.tsx#L117

This event fires always when the trigger is touched. Wouldn't it better to use onClick, which also fires on mobile devices, but wouldn't fire when the user is only scrolling?

Fensterbank commented 10 months ago

@TmLev In case you're still blocked by this issue, I created a local patch which solves the issue in our product.

patches/@radix-ui+react-dropdown-menu+2.0.6.patch

diff --git a/node_modules/@radix-ui/react-dropdown-menu/dist/index.js b/node_modules/@radix-ui/react-dropdown-menu/dist/index.js
index c05924d..205468b 100644
--- a/node_modules/@radix-ui/react-dropdown-menu/dist/index.js
+++ b/node_modules/@radix-ui/react-dropdown-menu/dist/index.js
@@ -118,7 +118,7 @@ const $d1bf075a6b218014$export$d2469213b3befba9 = /*#__PURE__*/ $7dQ7Q$react.for
         disabled: disabled
     }, triggerProps, {
         ref: $7dQ7Q$radixuireactcomposerefs.composeRefs(forwardedRef, context.triggerRef),
-        onPointerDown: $7dQ7Q$radixuiprimitive.composeEventHandlers(props.onPointerDown, (event)=>{
+        onClick: $7dQ7Q$radixuiprimitive.composeEventHandlers(props.onClick, (event)=>{
             // 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 (!disabled && event.button === 0 && event.ctrlKey === false) {
diff --git a/node_modules/@radix-ui/react-dropdown-menu/dist/index.mjs b/node_modules/@radix-ui/react-dropdown-menu/dist/index.mjs
index aa181ce..d7d8eb0 100644
--- a/node_modules/@radix-ui/react-dropdown-menu/dist/index.mjs
+++ b/node_modules/@radix-ui/react-dropdown-menu/dist/index.mjs
@@ -78,7 +78,7 @@ const $d08ef79370b62062$export$d2469213b3befba9 = /*#__PURE__*/ $9kmUS$forwardRe
         disabled: disabled
     }, triggerProps, {
         ref: $9kmUS$composeRefs(forwardedRef, context.triggerRef),
-        onPointerDown: $9kmUS$composeEventHandlers(props.onPointerDown, (event)=>{
+        onClick: $9kmUS$composeEventHandlers(props.onClick, (event)=>{
             // 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 (!disabled && event.button === 0 && event.ctrlKey === false) {

You can apply this patch using patch-package as your postinstall script.

Kulimantang commented 10 months ago

Had the same problem and now fixed it by preventing the event on the onPointerDown event (thanks to your comment @Fensterbank ), so far it seems to solve the issue.

const [open, setOpen] = useState(false);
//...    
<DropdownMenu open={open} onOpenChange={(open) => setOpen(open)}>
      <DropdownMenuTrigger
        asChild
        onPointerDown={(e) => e.preventDefault()}
        onClick={() => setOpen(!open)}
      >
          <Button>click me</Button>
      </DropdownMenuTrigger>
//...
</DropdownMenu>
Fensterbank commented 9 months ago

Follow up: I did notice after that change, that if I use the mouse to click on the trigger to close the open dropdown, it directly re-opens. Probably this happens because any blur effect closing the dropdown happens before the onClick event.

Therefore the solution of @Kulimantang is the better way due to more flexibility. In our case, in the mobile view, the dropdown content is full size (so you can't click the trigger to close it), so this is a good approach that works in both scenarios:


const isTouchDevice = 'ontouchstart' in window
const [open, setOpen] = useState(false);
//...    
<DropdownMenu open={open} onOpenChange={(open) => setOpen(open)}>
      <DropdownMenuTrigger
        asChild
        {...(touchDevice
          ? {
              onPointerDown: (e) => e.preventDefault(),
              onClick: () => setOpen(!open)
            }
          : undefined)}
      >
          <Button>click me</Button>
      </DropdownMenuTrigger>
//...
</DropdownMenu>
jackblackCH commented 7 months ago

Thanks for the solutions! This should be fixed by Radix itself! Any updates? Would a PR be welcome?

xvvvyz commented 1 month ago

I just want to add that another side effect of using onPointerDown is if you want the dropdown content to appear on top of the trigger, whichever dropdown button is under your cursor when the dropdown opens gets clicked immediately.