Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popovers are broken when shown in a contextmenu event listener #10905

Open
jpzwarte opened this issue Jan 9, 2025 · 13 comments
Open

Popovers are broken when shown in a contextmenu event listener #10905

jpzwarte opened this issue Jan 9, 2025 · 13 comments
Labels
topic: popover The popover attribute and friends

Comments

@jpzwarte
Copy link

jpzwarte commented Jan 9, 2025

What is the issue with the HTML Standard?

https://codepen.io/jpzwarte/pen/OPLQypz?editors=1010

If you want to use a popover for a context menu, you cannot use the contextmenu event to display the popover. The problem is that the popover is immediately light dismissed.

(not sure if this belongs in whatwg/html or whatwg/dom)

@jpzwarte
Copy link
Author

jpzwarte commented Jan 9, 2025

Perhaps the light dismiss behavior should be tweaked to not close any popovers that are shown immediately before in contextmenu, mousedown, mouseup, pointerdown, pointerup, touchstart, touchend event listeners?

@mkrause
Copy link

mkrause commented Jan 10, 2025

Maybe wrap it in a setTimeout(fn, 0) so the popover gets shown after the event.

@jpzwarte
Copy link
Author

That doesn't work in Chrome, FF or Safari. Once you increase the timeout to at least 100ms, then it sometimes works. But at that point you start to have a noticeable delay.

@mkrause
Copy link

mkrause commented Jan 10, 2025

That doesn't work in Chrome, FF or Safari. Once you increase the timeout to at least 100ms, then it sometimes works. But at that point you start to have a noticeable delay.

Ah, interesting. I just tested it, the reason is because contextmenu is fired on mouse down, but on mouse up the popover will be closed.

To test this, try wrapping the showPopover() in setTimeout(fn, 0), and then start right clicking but hold the click. The popover will show. As soon as you release the click the popover is closed.

What's also interesting is it doesn't seem that the "close popovers on click/mouseup" can be canceled. At least no combination that I tried with canceling click/mouseup/pointerup resulted in canceling the popover close. So that doesn't seem like a viable option either.

popover="manual" would work of course, but then you have to reimplement light dismiss.

@mkrause
Copy link

mkrause commented Jan 10, 2025

Found a workaround by using mouseup instead and checking for right click (or CTRL+click on Mac).

document.querySelector('span')?.addEventListener('contextmenu', (event) => {
  event.preventDefault();
});


document.querySelector('span')?.addEventListener('mouseup', (event) => {
  if (event.which === 3 || event.ctrlKey) {
    document.querySelector('[popover]').showPopover();   
  }
});

No setTimeout necessary when done in mouseup. Tested it in Chrome, Firefox, and Safari and works in all of them.

@annevk annevk added the topic: popover The popover attribute and friends label Jan 16, 2025
@annevk
Copy link
Member

annevk commented Jan 16, 2025

This sounds similar to #10890 (comment).

cc @josepharhar @nt1m @mfreed7

@josepharhar
Copy link
Contributor

Yeah, we are running light dismiss kind of unconditionally whenever there is a pointerup event. Maybe we should add a flag for the case that a popover is shown in between pointerdown and pointerup and skip doing light dismiss in that case? I don't think that would help with #10890 (comment) though.

@mfreed7
Copy link
Contributor

mfreed7 commented Jan 23, 2025

Interesting bug!

Yeah, we are running light dismiss kind of unconditionally whenever there is a pointerup event. Maybe we should add a flag for the case that a popover is shown in between pointerdown and pointerup and skip doing light dismiss in that case?

Yeah, this sounds like a good solution to me. Might be tricky to keep track of which popover(s) were open, but we can probably do something. Does this sound like a reasonable solution to others?

I don't think that would help with #10890 (comment) though.

Perhaps it could, basically if we keep track of the list of open popovers during pointerdown, and compare to pointerup, and exempt any popovers that changed (either direction) from the light dismiss behavior?

@nt1m
Copy link
Member

nt1m commented Jan 23, 2025

Another solution is to check for the event button of the pointerup event? (Just to be clear, I don't have a strong preference on how to resolve this, just throwing options out there)

@mfreed7
Copy link
Contributor

mfreed7 commented Jan 23, 2025

Another solution is to check for the event button of the pointerup event? (Just to be clear, I don't have a strong preference on how to resolve this, just throwing options out there)

Sounds interesting, but help me understand. In the repro for this issue, the pointerdown and pointerup both happen on the same <span>, don't they?

@nt1m
Copy link
Member

nt1m commented Jan 24, 2025

If you right click, there should be a different event.button used right? Why should right click trigger light dismiss in the first place?

@jpzwarte
Copy link
Author

Sounds interesting, but help me understand. In the repro for this issue, the pointerdown and pointerup both happen on the same <span>, don't they?

If you mousedown over the span, move the mouse elsewhere and then release when it's not over the span, it still light dismisses.

Aside from technical questions: a common way to invoke a context menu item is to mousedown, hover over a specific menu item and the release.

@mfreed7
Copy link
Contributor

mfreed7 commented Jan 25, 2025

If you right click, there should be a different event.button used right? Why should right click trigger light dismiss in the first place?

Ahh, mouse "button", not "the <button>". Thanks. I think normal behavior for transient things is that any mouse click, regardless of the button being pressed, light-dismisses popovers. (Unless the right-click is on the popover, of course.) For example, the UA-provided context menu itself is roughly a popover, and if you right-click somewhere else, the first one closes.

If you mousedown over the span, move the mouse elsewhere and then release when it's not over the span, it still light dismisses.

You mean right-mouse-down, etc.? Yeah, mouse drags also (per spec) light dismiss the popover, unless the drag starts or ends on the popover. That's so that you can e.g. select text on the popover.

Aside from technical questions: a common way to invoke a context menu item is to mousedown, hover over a specific menu item and the release.

That is common, at least on Mac platforms. I do think we should work on a solution to this issue. I'd love to make that solution also fix #10890 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: popover The popover attribute and friends
Development

No branches or pull requests

6 participants