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

Introduce contextmenuclose or similar event. #309

Open
flackr opened this issue Aug 26, 2021 · 11 comments · Fixed by #320 · May be fixed by #322
Open

Introduce contextmenuclose or similar event. #309

flackr opened this issue Aug 26, 2021 · 11 comments · Fixed by #320 · May be fixed by #322

Comments

@flackr
Copy link
Contributor

flackr commented Aug 26, 2021

There is a contextmenu event fired when the user right clicks or long presses (with touch) to show a context menu (Note #279 proposes adding this event to UI Events). There is however no notification about the closing of that menu. Developers typically call preventDefault() on the contextmenu event to create a custom menu, however they have to implement custom logic to try to detect under what circumstances it should close. There are many scenarios under which a contextmenu should be closed, that would be easier to handle given a single event to listen for:

  • Escape key pressed
  • When focusing any other window
  • Clicking outside of the browser window
  • When interacting with touch it is common to initiate a contextmenu after a long press. However, if the item being pressed is also draggable than a subsequent drag of the finger can be used to initiate a drag which implicitly cancels the contextmenu.

I propose adding a contextmenuclose (or contextmenucancel or contextmenudismiss) event which will be dispatched under the conditions when the platform would close a native contextmenu.

@hober
Copy link
Member

hober commented Aug 26, 2021

I wonder if it might make more sense to introduce a generic dismiss event that we could also use for e.g. <dialog> or custom modals (see domenic/close-watcher or similar efforts, such as Indie UI from back in the day).

(Mentioning WICG/close-watcher#1 so that this gets linked over there)

@mustaqahmed
Copy link
Member

Yes, a generic event seems logical here. Thanks.

While we don't expect modals inside modals in general, we can have context-menu from inside an active <dialog>! Wondering what's the best way to handle this. Maybe the order of dismiss events will implicitly determine what is being dismissed? Like the first event will be for the "topmost" modal and so on? Alternately, the dismiss event can carry some "dismiss target" information.

Thoughts?

@flackr
Copy link
Contributor Author

flackr commented Oct 5, 2021

I like the idea of trying to make this a generic event, and this works well for most of the ways that context menus can be dismissed, though it gets particularly tricky for dismissing a contextmenu on a drag, since I think dragging should be allowed within a dialog without dismissing it. I suppose one way to explain this particular quirk would to say if a dragstart targets an element that is presumably outside of the dialog element (i.e. the element the contextmenu is anchored on), it should implicitly dismiss the dialog.

@flackr
Copy link
Contributor Author

flackr commented Oct 18, 2021

@hober I think I may have not correctly grok'd your idea initially. To restate, we would add a dismiss event which would be fired when dialog or custom modals are closed, and could also be fired in these cases where a contextmenu should be dismissed? The firing of this dismiss event would not cause anything to close unless the developer handled that event by doing so.

Given this, a developer implementing a custom contextmenu would listen to the dismiss event and close their custom contextmenu UI in response. They would also need to make sure that pressing escape dismisses their contextmenu UI and not another dialog or popup element showing on the page. They would also need to make sure that they close their contextmenu UI if another element is focused or if focus of the window is lost.

My main concern is where the developer would see the dismiss event dispatched from. Presumably it would be the element which had the contextmenu event. However, if so, if that element was also a dialog element how would they listen for a dismiss of the contextmenu and not the dialog, or vice versa? My presumption is that contextmenu dismissal would have a different target but I'm not sure what that would be if you right clicked on the root of a dialog.

e.g.

<dialog> <!-- right click in here --> </dialog>

@mustaqahmed Another awkwardness (probably unavoidable) is that the browser wouldn't know if a custom contextmenu has been dismissed within the application so the developer may see a spurious dismiss / contextmenuclose event sometime later long after their custom contextmenu was dismissed internally within the application.

Would it make sense to start a PR for the contextmenuclose event and discuss / comment there with issues / ideas for how to deal with these issues.

@mustaqahmed
Copy link
Member

mustaqahmed commented Oct 19, 2021

I just sent out a draft PR (#322) that makes the new event non-cancelable and always follow a contextmenu event regardless of whether the latter was cancelled or not.

Please add your suggestions/comments there.

EDIT: Updated the PR link to point to the latest change.

@mustaqahmed
Copy link
Member

Yikes, the PR got merged prematurely as I was typing above!

@mustaqahmed mustaqahmed reopened this Oct 19, 2021
@mustaqahmed mustaqahmed linked a pull request Oct 19, 2021 that will close this issue
5 tasks
@mustaqahmed
Copy link
Member

Please add your suggestions/comments in the new PR: #322.

@flackr
Copy link
Contributor Author

flackr commented Jun 7, 2022

@mfreed7 I wanted to point your attention to this issue in the context of popup dismissal openui/open-ui#426. In particular, what would a dismiss event look like? Would it be dispatched whenever a popup was dismissed?

It sounds like if dismiss is just a way for developers to observe that a popup / dialog was dismissed, then having such an event doesn't need to block openui/open-ui#426, though it would mean that developers could only implement context menus properly by using popup elements as they wouldn't have the events if their application only consisted of e.g. a canvas.

@mfreed7
Copy link

mfreed7 commented Jun 9, 2022

Thanks for the ping. For the popup API, there are two events proposed, show and hide. The hide event is fired whenever the popup is hidden for any reason. It is not cancelable.

For this issue, I think you'd want a new/fresh event like contextmenudismiss, because it needs to be fired on the element that was the originator of the context menu, not the context menu itself.

Let me know if I understood your question correctly. If so, openui/open-ui#426 would be resolved by making the new contextmenudismiss event be a light dismiss trigger for popups. I think that part makes total sense.

@mustaqahmed
Copy link
Member

We seem to have concluded that a separate contextmenudismiss event is logical here which would be orthogonal to popup events, right? If so, please take a look at our proposed PR #322.

@mfreed7 how can we mark the new event as a light dismiss trigger as per your comment above?

@mfreed7
Copy link

mfreed7 commented May 12, 2023

We seem to have concluded that a separate contextmenudismiss event is logical here which would be orthogonal to popup events, right? If so, please take a look at our proposed PR #322.

@mfreed7 how can we mark the new event as a light dismiss trigger as per your comment above?

So the Popover spec has landed, and I think you'd just need to modify the "light dismiss" algorithm here:

https://html.spec.whatwg.org/multipage/popover.html#light-dismiss-open-popovers

by watching for the appropriate new event. Note that since the comments above, one change was that pointer-up is the light dismiss trigger, and there is logic to handle the case where the user is trying to highlight text starting within the popover. (That's the "popover pointerdown target" stuff.) I'm not sure how that'll interact with the contextmenudismiss event - perhaps it won't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants
@hober @flackr @mustaqahmed @mfreed7 and others