Fix popovers shown during contextmenu event [chromium/src : main]

0 views
Skip to first unread message

Joey Arhar (Gerrit)

unread,
Mar 24, 2025, 7:44:43 PMMar 24
to Mustaq Ahmed, AyeAye, Chromium LUCI CQ, blink-re...@chromium.org, jmedle...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org
Attention needed from Mustaq Ahmed

Joey Arhar added 1 comment

Commit Message
File-level comment, Patchset 2 (Latest):
Joey Arhar . unresolved

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?

Open in Gerrit

Related details

Attention is currently required from:
  • Mustaq Ahmed
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Id7c0514cdb117d9a14352ac48cc80e6aa051d681
Gerrit-Change-Number: 6371980
Gerrit-PatchSet: 2
Gerrit-Owner: Joey Arhar <jar...@chromium.org>
Gerrit-Reviewer: Joey Arhar <jar...@chromium.org>
Gerrit-Reviewer: Mustaq Ahmed <mus...@chromium.org>
Gerrit-Attention: Mustaq Ahmed <mus...@chromium.org>
Gerrit-Comment-Date: Mon, 24 Mar 2025 23:44:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Mustaq Ahmed (Gerrit)

unread,
Apr 1, 2025, 1:01:07 PMApr 1
to AyeAye, Chromium LUCI CQ, blink-re...@chromium.org, jmedle...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org
Attention needed from Joey Arhar

Mustaq Ahmed added 3 comments

Patchset-level comments
Mustaq Ahmed . unresolved

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.

File third_party/blink/renderer/core/dom/document.h
Line 1681, Patchset 2 (Latest): // The popover light dismiss canceled flag is set to true when some code
Mustaq Ahmed . unresolved

I guess this comment belongs to the next function below, right?

File third_party/blink/renderer/core/html/html_element.cc
Line 1641, Patchset 2 (Latest): GetDocument().CancelPopoverLightDismiss();
Mustaq Ahmed . unresolved

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!

Open in Gerrit

Related details

Attention is currently required from:
  • Joey Arhar
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Id7c0514cdb117d9a14352ac48cc80e6aa051d681
Gerrit-Change-Number: 6371980
Gerrit-PatchSet: 2
Gerrit-Owner: Joey Arhar <jar...@chromium.org>
Gerrit-Reviewer: Joey Arhar <jar...@chromium.org>
Gerrit-Reviewer: Mustaq Ahmed <mus...@chromium.org>
Gerrit-Attention: Joey Arhar <jar...@chromium.org>
Gerrit-Comment-Date: Tue, 01 Apr 2025 17:00:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Mustaq Ahmed (Gerrit)

unread,
Apr 1, 2025, 2:09:04 PMApr 1
to AyeAye, Chromium LUCI CQ, blink-re...@chromium.org, jmedle...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org
Attention needed from Joey Arhar

Mustaq Ahmed added 1 comment

Commit Message
Joey Arhar . unresolved

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?

Mustaq Ahmed

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.

Open in Gerrit

Related details

Attention is currently required from:
  • Joey Arhar
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Id7c0514cdb117d9a14352ac48cc80e6aa051d681
Gerrit-Change-Number: 6371980
Gerrit-PatchSet: 2
Gerrit-Owner: Joey Arhar <jar...@chromium.org>
Gerrit-Reviewer: Joey Arhar <jar...@chromium.org>
Gerrit-Reviewer: Mustaq Ahmed <mus...@chromium.org>
Gerrit-Attention: Joey Arhar <jar...@chromium.org>
Gerrit-Comment-Date: Tue, 01 Apr 2025 18:08:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joey Arhar <jar...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Joey Arhar (Gerrit)

unread,
Apr 3, 2025, 4:25:44 PMApr 3
to Mustaq Ahmed, AyeAye, Chromium LUCI CQ, blink-re...@chromium.org, jmedle...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org
Attention needed from Mustaq Ahmed

Joey Arhar added 1 comment

Commit Message
Joey Arhar . unresolved

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?

Mustaq Ahmed

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.

Joey Arhar

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

Open in Gerrit

Related details

Attention is currently required from:
  • Mustaq Ahmed
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Id7c0514cdb117d9a14352ac48cc80e6aa051d681
Gerrit-Change-Number: 6371980
Gerrit-PatchSet: 2
Gerrit-Owner: Joey Arhar <jar...@chromium.org>
Gerrit-Reviewer: Joey Arhar <jar...@chromium.org>
Gerrit-Reviewer: Mustaq Ahmed <mus...@chromium.org>
Gerrit-Attention: Mustaq Ahmed <mus...@chromium.org>
Gerrit-Comment-Date: Thu, 03 Apr 2025 20:25:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Mustaq Ahmed <mus...@chromium.org>
Comment-In-Reply-To: Joey Arhar <jar...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Joey Arhar (Gerrit)

unread,
Sep 17, 2025, 12:54:37 PM (6 days ago) Sep 17
to Mustaq Ahmed, AyeAye, Chromium LUCI CQ, blink-re...@chromium.org, jmedle...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, blink-rev...@chromium.org

Joey Arhar abandoned this change

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: abandon
satisfied_requirement
unsatisfied_requirement
open
diffy
Reply all
Reply to author
Forward
0 new messages