1707204 - "Show in Finder" and other context menu items in the Downloads panel that operate on a single download are nonfunctional.

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[Gian-Carlo Pascutto [:gcp]]

macOS Big Sur 11.2.3, M1 Air

Download a file.

Expand the download menu in the toolbar. Right-click the file, "Show in Finder".

Expected: Finder opens, similar to clicking the magnifying glass icon.
Actual: No effect.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

I used mozregression and tracked this down to bug 1700679. It might be nice to re-run mozregression with native context menus enabled to find what change within the feature is causing this.

Comment 2

[Markus Stange [:mstange]]

focusedElement is null here: https://searchfox.org/mozilla-central/rev/9f76a47f4aa935b49754c5608a1c8e72ee358c46/browser/components/downloads/content/downloads.js#1232-1240

I'm not quite sure why we need to check focus when handling the command.

Comment 3

[:Gijs (he/him)]

We previously broke this on Windows in bug 1695383, and unbroke it when we changed the implementation of the Win10 menu styling to not have big shadow boxes around their edges, which were causing mouse events to get lost. See in particular bug 1695383 comment 5:

This is broken because the downloads panel keeps track of selection based on mouse hover state, and the new [Windows 10 custom-style] context menu's big box shadow area is eating mouse events, which means that when the context menu opens, the hovered item in the downloads panel is no longer hovered, which means it is no longer selected, which means that when executing any of the commands in the menu, they don't work because the downloads panel does not know which item is selected.

Fixing bug 1692084 would likely fix this. We could also fix it orthogonally by changing the downloads view to "fixate" selection on the context menu'd item when opening the context menu, and then "release" the selection back to the mouse hover / keyboard behaviour when the context menu closes. In fact, this last thing is already what the code is trying to do, but it's not working -- Marco and me aren't sure off-hand precisely why the selection is being lost...

(In reply to Markus Stange [:mstange] from comment #2)

focusedElement is null here: https://searchfox.org/mozilla-central/rev/9f76a47f4aa935b49754c5608a1c8e72ee358c46/browser/components/downloads/content/downloads.js#1232-1240

I'm not quite sure why we need to check focus when handling the command.

The downloads panel does some weird stuff to track the "active" element for the context menu, and act on that. I think it's to do with the use of command elements and those not necessarily being invoked from the context menu (so can't rely on triggerNode). It doesn't help that a bunch of this code is shared with about:downloads and the downloads view in the library.

Comment 4

[:Gijs (he/him)]

Shilpa/Romain: given this is basically completely busted with the new macOS menus (see also bug 1707652), I'm assuming this will be P1 or whatever we call that, and I'll start work on this. I still have some context from bug 1695383 so hopefully that will make figuring this out quicker.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1707204 - Remove FX_MIGRATIONS_BOOKMARKS_ROOTS telemetry probe now that its expired.

Comment 6

[Phabricator Automation]

Comment on attachment 9218473 [details]
Bug 1707204 - Remove FX_MIGRATIONS_BOOKMARKS_ROOTS telemetry probe now that its expired.

Revision D113415 was moved to bug 1706836. Setting attachment 9218473 [details] to obsolete.

Comment 7

[:Gijs (he/him)]

The issue is that with native menus we get a mouseout when the menu opens, but we didn't with the non-native menus. The mouseout triggers https://searchfox.org/mozilla-central/rev/37edd2782e67e716dd07a85016da07b4d6275e5d/browser/components/downloads/content/downloads.js#930-937 , which clears the selected item in the richlistbox that is used in the panel, which means the box doesn't know anymore which item is selected, which is why everything else goes pearshaped after that. Looking into fixing this now.

Comment 8

[:Gijs (he/him)]

Attachment: Bug 1707204 - fix download panel item mouse event handlers so the context menu is considered open before popupshown has fired, r?mak

As a bonus, this is net code removal.

I spent a long time on a test, but ultimately it seems that in an automated test the
conditions in real use don't appear (ie the mouseout event doesn't happen) unless
manually fired from the test, which seems like it isn't worth it.

Comment 9

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

Set release status flags based on info from the regressing bug 1700679

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa40fbe24dd6
Remove FX_MIGRATIONS_BOOKMARKS_ROOTS telemetry probe now that its expired. r=emalysz

Comment 11

[Phabricator Automation]

Comment on attachment 9218473 [details]
Bug 1707204 - Remove FX_MIGRATIONS_BOOKMARKS_ROOTS telemetry probe now that its expired.

Revision D113415 was moved to bug 1706836. Setting attachment 9218473 [details] to obsolete.

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8925e650afb
Backed out changeset aa40fbe24dd6 for landing with the wrong bug number

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a279dcb23ef
fix download panel item mouse event handlers so the context menu is considered open before popupshown has fired, r=jaws

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/8a279dcb23ef

Comment 15

[:Gijs (he/him)]

Comment on attachment 9218514 [details]
Bug 1707204 - fix download panel item mouse event handlers so the context menu is considered open before popupshown has fired, r?mak

Beta/Release Uplift Approval Request

• User impact if declined: Required for MR1 / Proton, if bug 1700679 is uplifted. Uplifting this prior to bug 1700679 would be fine (ie there's no hard dependency).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0. Note: to reproduce the bug, widget.macos.native-context-menus needs to be set to true. This is not yet the case on beta, but will happen once bug 1700679 is uplifted.
• List of other uplifts needed: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simplifies some JS logic in the downloads code that tracks whether a context menu is open, has automated tests (that didn't catch the original bug on macOS, but should catch breaking changes on other platforms).
• String changes made/needed: Nope

Comment 16

[:Gijs (he/him)]

This won't reproduce on beta except when the native macOS menus are enabled. Doing that by default is bug 1700679 which already has a pending uplift request. Going to set beta as affected to ensure this shows up in uplift queries.

Comment 17

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/807e5ec45aa9

Comment 18

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue using old Nightly from 2021-04-23, verified that "Show in Finder" and other items inside download panel are working as expected using 89.0b7 and latest Nightly 90.0a1 on macOS 11.2 and macOS 11.2.3 (M1).