[menu] menu emits double click events when closeOnSelect: false

[anilanar]

🐛 Bug report

Menu emits double click events when closeOnSelect: false, because of a technical issue, due to the following code snippet:

        onPointerUp(event) {
          const evt = getNativeEvent(event)
          if (!evt.isPrimary || disabled) return
          event.currentTarget.click()
        },

I guess this is just an oversight. There are multiple problems in this approach:

1. If the goal is to prevent non-left clicks, isPrimary doesn't do that according to MDN. Can use evt.button to decide if it's a left click or not.
2. This event handler does nothing to prevent the other onClick handler from being triggered regardless of isPrimary. I'd have expected stopPropogation or preventDefault or return false or such.
3. Neither stopPropogation nor preventDefault nor return false will prevent the other onClick handler from running. pointerup event apparently cannot cancel the upcoming click event.
4. Due to event.currentTarget.click(), click events happen twice, so onClick handlers run twice. For checkboxes, that means they are toggled twice.
5. This bug is only observable when closeOnSelect: false because when closeOnSelect: true, the menu is closed on the first click so it prevents the second click from happening.
6. It seems click event's button prop doesn't actually reflect the button that is pressed. It's always 0 even if it's a different button.

My proposal(s)

Option 1. Drop onClick handlers and rely on onPointerUp only, if it's so important to only respect left clicks.

Option 2. Drop onPointerUp handler and rely only on onClick handlers, if it's not very important to handle middle/right clicks.

Option 3. Try hard to prevent non-left clicks. Integrate onPointerUp into the state machine, in onPointerUp event check whether if evt.button === 0 and emit "LEFT_CLICK_UP" and in onClick, emit CLICK and do the proper state transition only if LEFT_CLICK_UP is followed up by a CLICK, but this still sounds naive. There are myriad of edge cases like user holding left button pressed and release it on another element etc. Why bother? Let people click with whatever button they want.

I'd pick "Option 2" for now so that the bug that completely breaks checkboxes with closeOnSelect: false is fixed, and create a followup github issue to prevent non-left clicks (which I think will not be trivial to achieve) in future.

💥 Steps to reproduce

1. Add closeOnSelect: false to menu-options.ts in next-ts example project.
2. Try clicking checkboxes.
3. (BONUS) Add debugger in onPointerUp and in both onClick handlers in menu.connect.ts.

Expected behavior

Checkboxes toggle.

Actual behavior

Checkboxes don't toggle.

Codesandbox

I suggest you see with your own eyes, with a debugger.

But here you go: https://codesandbox.io/s/gallant-ellis-cq0wyr?file=/src/App2.js

🌍 System information

Software	Version(s)
Zag Version	0.1.11
Browser	All
Operating System	All

[anilanar]

We published a temporary fork with this problem fixed: https://www.npmjs.com/package/@userlike/zag-menu

[segunadebayo]

Thanks for the detailed report @anilanar.

I just pushed a fix for this. We'll release an update shortly.

If the issue persists after upgrading, I'll re-open it.

[frankborden]

I am still seeing this regardless of closeOnSelect. Here is a sandbox of the Menu docs basic example with a log statement added https://codesandbox.io/s/wonderful-pine-y8olsp?file=/src/App.js

[anubra266]

@frankborden There's no issue there, note how it only runs twice when you click particularly on a list item and not anywhere else in the content container, that's because the click event bubbles. And BTW what's your usecase for running a function when content is clicked? If you want to run a method when a menu item is selected, then use the onSelect method in context, if you explicitly want to run a method when an item is clicked, then you should use mergeProps to add your onClick handler.

[frankborden]

I see, you are right. onSelect is the way to do what I want without issue. I did not see it in the docs, but I see it in some examples like: https://github.com/chakra-ui/zag/blob/1e9306c0cab3174fd0e4d2926a773f18280eea55/examples/next-ts/pages/context-menu.tsx

Thank you for the explanation.

[anilanar]

@anubra266 What about the reproduction in original post? That reproduction with zag deps updated to latest version is still reproducible. Basically checkbox menu items cannot be toggled while closeOnSelect: false due to double click events. It's not event bubbling, the code is retriggering element.click() itself in onPointerUp.