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

[Popup] Light dismissal triggers for popup when used for context menu #426

Closed
flackr opened this issue Dec 2, 2021 · 6 comments
Closed
Labels
popover The Popover API

Comments

@flackr
Copy link

flackr commented Dec 2, 2021

One of the use cases popups are well suited for is for context menus. From the light dismissal behaviours there may be a couple dismissal cases missing which I'm hoping we can add:

  • Dismiss on a click / touch anywhere outside of the popup, regardless of whether it causes a focus change.
  • Dismiss on dragstart starting outside of the popup. This is specifically for mobile platforms where the native contextmenu is triggered on long press but dismissed on dragstart to allow disambiguation between the two actions.

If we have light dismissal in these two cases I think that a popup used for a custom context menus should just work, e.g. <div oncontextmenu="popup.show()"> or perhaps we could even support <div contextmenu="popup-id">.

@gregwhitworth
Copy link
Member

Good insights? What is your goal for next steps for these insights? It seems that you're beginning to lean towards a new event or content attribute? If so; where/how are you wanting to incubate these as these won't easily transfer to popup itself IMO.

@gregwhitworth gregwhitworth added the popover The Popover API label Jan 19, 2022
@mfreed7
Copy link
Collaborator

mfreed7 commented Mar 24, 2022

  • Dismiss on a click / touch anywhere outside of the popup, regardless of whether it causes a focus change.

So this one, I think, is (attempted to be) captured by the existing explainer. This section says that "click outside" should be a light dismiss trigger for popup and hint. Clearly, the description of "click outside" could use more of a concrete definition. But your text above is exactly what I was envisioning.

  • Dismiss on dragstart starting outside of the popup. This is specifically for mobile platforms where the native contextmenu is triggered on long press but dismissed on dragstart to allow disambiguation between the two actions.

This is an interesting one. This issue talks about whether scrolling should be a light dismiss trigger, and my reading is that the general direction seems to be "no". Your use case of a context menu replacement is interesting though. This seems to be platform dependent - at least on my Mac in Chrome, when there's an active (native browser) context menu, scrolling is completely blocked. So no dismiss-on-scroll, but also, no-scroll.

@mfreed7
Copy link
Collaborator

mfreed7 commented May 13, 2022

I recently updated the explainer to more-concretely flesh out what "light dismiss" means. I believe that based on that definition, and assuming #529 results in no changes, both of your conditions should be supported as-is. Basically, a mousedown on anything outside the DOM tree of the popup (including the ancestral popup logic) constitutes a light dismiss signal. Let me know if you think there's anything missing from that definition. Otherwise, I think we can close this issue.

@flackr
Copy link
Author

flackr commented May 14, 2022

Sorry I should have been more specific. When I was talking about dragstart this is the html drag and drop api dragstart event. The slightly tricky bit is that the thing that will be "dragged" is the trigger for the contextmenu popup.

E.g. imagine you have <div draggable="true" oncontextmenu="popup.show()">. That div is the thing that will be dragged. This is outside of the popup element itself though so maybe this will still work.

That said, the current design is that the dragstart will happen first, but when you move your finger suffiicently far we plan to dispatch a new type of event: w3c/uievents#309 so as long as this new event being dispatched to the triggering element counts as a light dismiss we'll be good.

@mfreed7
Copy link
Collaborator

mfreed7 commented May 26, 2022

Right, I understood that we're talking about dragstart from the drag and drop API, but I was missing the details of the scenario. I think I finally understand! Correct me if I'm wrong:

  1. You have a <div> with an oncontextmenu handler that shows a popup (either native or developer-provided).
  2. The platform, on mobile, wants to fire the contextmenu event on a long press, but if the user then starts dragging, it wants to "cancel" the context menu (somehow) and instead fire a dragstart event.
  3. If the popup from #1 is provided by the Popup API, we need a way to make sure the "cancel" from #2 happens.

Based on the above, you therefore think a dragstart event needs to be added to the current mousedown trigger for light dismiss.

I believe your logic is sound, and I don't see any issues with adding such a trigger. One peripheral thing is that a popup triggered by an invoking attribute (e.g. triggerpopup) is triggered on a click event, but in your examples above, no click should ever be fired. So that logic should be unaffected.

I think this makes sense. If the above sounds right, I'm inclined to just add this to the explainer and prototype as an implementation detail, and avoid spending OpenUI meeting time for a resolution. Make sense?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 27, 2022
See the discussion at [1] for more context, but this handles the
mobile contextmenu case.

[1] openui/open-ui#426 (comment)

Bug: 1307772
Change-Id: I32e5440c3560cf93b054ff32eb87a79c021cca15
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 27, 2022
See the discussion at [1] for more context, but this handles the
mobile contextmenu case.

[1] openui/open-ui#426 (comment)

Bug: 1307772
Change-Id: I32e5440c3560cf93b054ff32eb87a79c021cca15
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 1, 2022
See the discussion at [1] for more context, but this handles the
mobile contextmenu case.

[1] openui/open-ui#426 (comment)

Bug: 1307772
Change-Id: I32e5440c3560cf93b054ff32eb87a79c021cca15
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 3, 2022
See the discussion at [1] for more context, but this handles the
mobile contextmenu case.

[1] openui/open-ui#426 (comment)

Bug: 1307772
Change-Id: I32e5440c3560cf93b054ff32eb87a79c021cca15
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 3, 2022
See the discussion at [1] for more context, but this handles the
mobile contextmenu case.

[1] openui/open-ui#426 (comment)

Bug: 1307772
Change-Id: I32e5440c3560cf93b054ff32eb87a79c021cca15
@mfreed7
Copy link
Collaborator

mfreed7 commented Jun 21, 2022

So w3c/uievents#309 discusses adding a new contextmenuclose or similar event, which will be fired when the UA wants to close a context menu due to the (typically touch-) user initiating a drag from a long-press. Once that's in the platform, we'll want to make it also be a light dismiss trigger for popup. But until then, based on some offline discussion, it seems that we shouldn't make dragstart a light dismiss trigger. For one, it always comes before contextmenu, so it doesn't work for the problematic use case. I'm going to close this issue - we'll open a fresh one when contextmenuclose is a thing.

@mfreed7 mfreed7 closed this as completed Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
popover The Popover API
Projects
None yet
Development

No branches or pull requests

3 participants