π 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:
- 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.
- 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.
- Neither
stopPropogation
nor preventDefault
nor return false
will prevent the other onClick
handler from running. pointerup
event apparently cannot cancel the upcoming click
event.
- Due to
event.currentTarget.click()
, click events happen twice, so onClick
handlers run twice. For checkboxes, that means they are toggled twice.
- 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.
- 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
- Add
closeOnSelect: false
to menu-options.ts
in next-ts
example project.
- Try clicking checkboxes.
- (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 |