"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)
Tracking
()
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)
(deleted),
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details |
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•4 years ago
|
||
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.
Updated•4 years ago
|
Updated•4 years ago
|
Comment 2•4 years ago
|
||
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.
Assignee | ||
Comment 3•4 years ago
|
||
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-1240I'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.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 4•4 years ago
|
||
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 | ||
Updated•4 years ago
|
Comment hidden (typo) |
Comment hidden (typo) |
Assignee | ||
Comment 7•4 years ago
|
||
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.
Assignee | ||
Comment 8•4 years ago
|
||
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•4 years ago
|
||
Set release status flags based on info from the regressing bug 1700679
Updated•4 years ago
|
Updated•4 years ago
|
Comment 10•4 years ago
|
||
Comment 11•4 years ago
|
||
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•4 years ago
|
||
Comment 13•4 years ago
|
||
Comment 14•4 years ago
|
||
bugherder |
Updated•4 years ago
|
Assignee | ||
Comment 15•4 years ago
|
||
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
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 16•4 years ago
|
||
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.
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Comment 17•4 years ago
|
||
bugherder uplift |
Comment 18•4 years ago
|
||
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).
Updated•4 years ago
|
Description
•