1747609 - "Manage extension" does not work in overflow menu

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[gwarser]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:97.0) Gecko/20100101 Firefox/97.0

Steps to reproduce:

• fresh Fx
• install uBlock Origin
• pin icon to overflow menu
• click overflow menu
• right click uBO icon
• select "Manage extension"

Actual results:

nothing happens

Expected results:

uBO about:addons management page should open

Regression:

21:32.30 INFO: Running autoland build built on 2021-12-10 10:41:41.311000, revision 8e5e8646
21:39.93 INFO: Launching /tmp/tmphd5kloes/firefox/firefox
21:39.93 INFO: Application command: /tmp/tmphd5kloes/firefox/firefox --allow-downgrade -profile /tmp/tmpf4zar8w1.mozrunner
21:39.93 INFO: application_buildid: 20211207115238
21:39.93 INFO: application_changeset: 8e5e8646673133a26178bd2f575627d2ed236a2c
21:39.93 INFO: application_name: Firefox
21:39.93 INFO: application_repository: https://hg.mozilla.org/integration/autoland
21:39.93 INFO: application_version: 97.0a1
Was this integration build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): bad
23:50.28 INFO: Narrowed integration regression window from [6068f735, d847e5e3] (3 builds) to [6068f735, 8e5e8646] (2 builds) (~1 steps left)
23:50.28 INFO: No more integration revisions, bisection finished.
23:50.28 INFO: Last good revision: 6068f735f2247e6877eef093757cbb0fa59910e0
23:50.28 INFO: First bad revision: 8e5e8646673133a26178bd2f575627d2ed236a2c
23:50.28 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6068f735f2247e6877eef093757cbb0fa59910e0&tochange=8e5e8646673133a26178bd2f575627d2ed236a2c

Comment 1

[gwarser]

Is "Component: Downloads Panel" as in bug 1714846 right?

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Widget: Gtk' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Probably there's a better component than this.

Comment 4

[:Gijs (he/him)]

Reporter: Are there any errors in the browser console when this fails?

Emilio: given the patch in bug 1714846 didn't actually fix bug 1742797, should we back it out? Is it doing anything good at this point? Because the game of whack-a-mole with all the menus that depended on click->command dispatch logic that broke when the popup closes before the command gets dispatched doesn't seem like much fun. :-\

I guess as an alternative workaround we could make the display: none stuff take effect from a setTimeout(0), though in that case we'd obviously want to re-check whether the menu is still closed after the timeout - thoughts?

Comment 5

[gwarser]

No errors in console.

Add-on specific entry is duplicated every time I click on it. Duplicates disappear after I switch to different window(app?).

Comment 6

[gwarser]

Attachment: duplicates.png

Comment 7

[gwarser]

Looks like only two entries work: Unpin from Overflow Menu and Remove from toolbar. Clicking any entry causes "Block element..." duplication.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to :Gijs (back Jan 4, 2022; he/him) from comment #4)

Reporter: Are there any errors in the browser console when this fails?

Emilio: given the patch in bug 1714846 didn't actually fix bug 1742797, should we back it out? Is it doing anything good at this point? Because the game of whack-a-mole with all the menus that depended on click->command dispatch logic that broke when the popup closes before the command gets dispatched doesn't seem like much fun. :-\

I still think it's worth it. We've been hit with similar stuff before where an element was animating inside the panel (old WebRTC indicator is an example of this).

(In reply to :Gijs (back Jan 4, 2022; he/him) from comment #4)

I guess as an alternative workaround we could make the display: none stuff take effect from a setTimeout(0), though in that case we'd obviously want to re-check whether the menu is still closed after the timeout - thoughts?

So presumably what's going on is that nsMenuCommand relies on the menuframe being around here right? I think we should dispatch the command event even if we've closed already.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

So I took a look at this. There are multiple issues.

The main one is that the command event is dispatched async here, so a bunch of random stuff may have happened since it was posted till it runs (including closing the menu by other code).

The relevant hidePopup() call comes from here in this case, and happens off the click event.

Anyways there's the issue of the command event not getting fired if there's no frame (here), which is sort of trivial to fix, but then there's the issue of the extensions code relying on the .triggerNode here, which after hiding the panel is null.

So it's not clear to me why we post the command events to the main thread to begin with, while firing popuphidden events and such synchronously, that seems like a massive footgun. I wonder if we can just make them fire synchronously... Let's see how terrible that looks:

https://treeherder.mozilla.org/jobs?repo=try&revision=30a305b461ab07db5b2572990c451a2fc4ed26e6

Comment 10

[Emilio Cobos Álvarez (:emilio)]

This is an slightly more complete patch, hopefully: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e777c2104a8a80c9578c28f274126dd16670a62e

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Ok, but looking into this a bit further, why do we even want to auto-hide the popup in the case we're clicking in the context menu? Seems like it's probably unintended to begin with. For once, it would break the macOS "blinking".

Right now it doesn't because we use native context menus in macOS, and those don't trigger click events...

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1747609 - Don't auto-hide panel on click inside a menupopup. r=Gijs

This matches the current macOS behavior with native context menus.

The issue is that closing the panels causes the destruction of the
menupopup while the activate event hasn't run yet, see comment 9.

Executing the command event synchronously like we do for <button>s is
probably worth it to some extent, though note that on macOS with
non-native menus it'd still need to be async because of the
blinking-before-activation that menuitems have. Gnarly :-(

Of course we always use native menus for context menus so it's not a big
deal, but it still affects the content <select> dropdown, I believe.

So this is the less-invasive change.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Thanks for the review Gijs, will fix the patch. Lmk if you think landing something like https://treeherder.mozilla.org/jobs?repo=try&revision=e777c2104a8a80c9578c28f274126dd16670a62e seems worth it / reasonable. It seems green or very close to it.

Comment 14

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)

Thanks for the review Gijs, will fix the patch. Lmk if you think landing something like https://treeherder.mozilla.org/jobs?repo=try&revision=e777c2104a8a80c9578c28f274126dd16670a62e seems worth it / reasonable. It seems green or very close to it.

I think that would be heplful yes - I think maybe next week we could get a second opinion from Neil, to check if he remembers any context around why this is async in the first place?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/309f9ab93b03
Don't auto-hide panel on click inside a menupopup. r=Gijs

Comment 16

[Neil Deakin]

I think that would be heplful yes - I think maybe next week we could get a second opinion from Neil, to check if he remembers any context around why this is async in the first place?

This is so that the menu hides and the accessibility events fire before the command event fires.

The patch here seems to change this such that the accessibility events that notify that the popup is closed fire after whatever the command does, but I don't think that is possible.

When this code was originally written, a menu that had a command event that opened a modal dialog would leave the popup visible without the dual-step popuphiding. Does this patch affect this?

Comment 17

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing failures on browser_popup-record-capture-view.js. CLOSED TREE

Backout link: https://hg.mozilla.org/integration/autoland/rev/215ef14f31927f04e41a0ce23748146d4bd5ffc7

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=309f9ab93b03487aa1ba7e9a8c4af86546f7d42e&selectedTaskRun=bofPuHFFQS-Fsax_tx31Rw.0

Link to failure log :
https://treeherder.mozilla.org/logviewer?job_id=362647020&repo=autoland&lineNumber=2802
https://treeherder.mozilla.org/logviewer?job_id=362646743&repo=autoland&lineNumber=6597

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d5981a9dc3e
Don't auto-hide panel on click inside a menupopup. r=Gijs

Comment 19

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/0d5981a9dc3e

Comment 20

[Emilio Cobos Álvarez (:emilio)]

(In reply to Neil Deakin from comment #16)

The patch here seems to change this such that the accessibility events that notify that the popup is closed fire after whatever the command does, but I don't think that is possible.

It is true that the a11y events do fire with the patch above after activating the menu on the tests, but that's because menu items are activated async and the event loop in tests doesn't spin between the mousemove over the item and the click. But the menu does hide (with state ePopupInvisible) before firing the command event.

When this code was originally written, a menu that had a command event that opened a modal dialog would leave the popup visible without the dual-step popuphiding. Does this patch affect this?

I don't think it would, because we're not removing the dual-step popuphiding. We just run the command event sooner, that's all. Given that, would pursuing landing that patch make sense to you?

Comment 21

[Neil Deakin]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #20)

It is true that the a11y events do fire with the patch above after activating the menu on the tests, but that's because menu items are activated async and the event loop in tests doesn't spin between the mousemove over the item and the click. But the menu does hide (with state ePopupInvisible) before firing the command event.

OK, I think that probably is ok, but you might want to confirm with someone who can test or verify the accessibility impact.

Comment 22

[Monica Chiorean]

Reproduced issue on Win10 using Build 97.0a1(20220121104506) and steps from description.
Verified as fixed on Win 10 / Ubuntu 20.4 / Mac 10.13 using builds: 97.0b6 (20220120185732) and 98.0a1 (20220121104506).