Closed Bug 1707204 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Menus, defect, P1)

ARM64
macOS
defect

Tracking

()

VERIFIED FIXED
90 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox88 --- unaffected
firefox89 --- verified
firefox90 --- verified

People

(Reporter: gcp, Assigned: Gijs)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [proton-context-menus] [proton-uplift])

Attachments

(1 file, 1 obsolete file)

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.

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.

Blocks: 1700679
Flags: needinfo?(mstange.moz)
No longer blocks: 1700679
Keywords: regression
Regressed by: 1700679
Has Regression Range: --- → yes

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.

Flags: needinfo?(mstange.moz)
Blocks: 34572

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.

Whiteboard: [proton-context-menus]

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.

Assignee: nobody → gijskruitbosch+bugs

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.

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.

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

Attachment #9218473 - Attachment is obsolete: false
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/aa40fbe24dd6 Remove FX_MIGRATIONS_BOOKMARKS_ROOTS telemetry probe now that its expired. r=emalysz

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.

Attachment #9218473 - Attachment is obsolete: true
Backout by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d8925e650afb Backed out changeset aa40fbe24dd6 for landing with the wrong bug number
Pushed by gijskruitbosch@gmail.com: 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
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
Priority: -- → P1

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
Attachment #9218514 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Whiteboard: [proton-context-menus] → [proton-context-menus] [proton-uplift]

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.

Summary: "Show in Finder" context menu in Downloads is nonfunctional. → "Show in Finder" and other context menu items in the Downloads panel that operate on a single download are nonfunctional.
QA Whiteboard: [qa-triaged]

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).

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Attachment #9218514 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: