Fix popovers shown during contextmenu event [chromium/src : main]
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
Hi mustaq! This patch makes a test fail when doing touch events because pointerdown is not fired unless there is a pointerdown event listener in the document, which leads to HTMLElement::HandlePopoverLightDismiss only getting called for pointerup.
I'm guessing this is a performance optimization somewhere that I can't find. Is there another place that pointer events are dispatched that we could hook up to popover light dismiss? Or make pointerdown always get fired on touch when there is an open popover in the document?
Related details
- Mustaq Ahmed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 3 comments![]( "Open in Gerrit")
Sorry for the late review, I was OOO for a while.
The change looks logical to me. I have a newbie question on light-dismiss: what set of interaction events cause light-dismissal? I am trying to understand why `mouseup` is there in the first place.
// The popover light dismiss canceled flag is set to true when some codeI guess this comment belongs to the next function below, right?
GetDocument().CancelPopoverLightDismiss();Doesn't this mean the light-dismiss behavior is cancelled unconditionally when the feature is enabled? Apologies if I missed something obvious here, I am new to this behavior and also lost in the 200-line method!
Related details
- Joey Arhar
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 1 comment![]( "Open in Gerrit")
Hi mustaq! This patch makes a test fail when doing touch events because pointerdown is not fired unless there is a pointerdown event listener in the document, which leads to HTMLElement::HandlePopoverLightDismiss only getting called for pointerup.
I'm guessing this is a performance optimization somewhere that I can't find. Is there another place that pointer events are dispatched that we could hook up to popover light dismiss? Or make pointerdown always get fired on touch when there is an open popover in the document?
You are correct, this is an optimization from the browser side, see the refs to [PreFilterResult::kFilteredNoPageHandlers](https://source.chromium.org/chromium/chromium/src/+/main:components/input/passthrough_touch_event_queue.h;drc=87fedd04f34e479b4d5a45f957f09b79d45e3cee;l=219). In fact we are "saving" `mouseup` from this optimization for an unknown reason, and my attempt to omit this exception failed (can't locate the bug now, sorry).
My core question remains: `mouseup` marks an end of the mouse interaction so it seems counter-intuitive to dismiss at `up`. Maybe `click` and `auxclick` are more qualified dismissal events, not sure.
Related details
- Joey Arhar
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
Mustaq AhmedHi mustaq! This patch makes a test fail when doing touch events because pointerdown is not fired unless there is a pointerdown event listener in the document, which leads to HTMLElement::HandlePopoverLightDismiss only getting called for pointerup.
I'm guessing this is a performance optimization somewhere that I can't find. Is there another place that pointer events are dispatched that we could hook up to popover light dismiss? Or make pointerdown always get fired on touch when there is an open popover in the document?
You are correct, this is an optimization from the browser side, see the refs to [PreFilterResult::kFilteredNoPageHandlers](https://source.chromium.org/chromium/chromium/src/+/main:components/input/passthrough_touch_event_queue.h;drc=87fedd04f34e479b4d5a45f957f09b79d45e3cee;l=219). In fact we are "saving" `mouseup` from this optimization for an unknown reason, and my attempt to omit this exception failed (can't locate the bug now, sorry).
My core question remains: `mouseup` marks an end of the mouse interaction so it seems counter-intuitive to dismiss at `up`. Maybe `click` and `auxclick` are more qualified dismissal events, not sure.
Thanks! I agree on the click/auxclick instead of pointerdown/pointerup concept, but I ran into some issues that I emailed you, mason, and rob about.
I am taking a different approach to this patch for now by just looking at the button in the event, which I created an HTML spec PR for (i will probably abandon this patch): https://chromium-review.googlesource.com/c/chromium/src/+/6426904
Thanks for the pointer to PreFilterResult! I am still trying to hook up pointer events on touch input to fix a different bug here, and I tried a bunch of stuff but I'm still not receiving pointer events unless I add an event listener. Want to take a look there and see if I'm missing something? https://chromium-review.googlesource.com/c/chromium/src/+/6430952
Related details
- Mustaq Ahmed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |