1761265 - Do not show downloads panel for "save link/image/page as" downloads that didn't previously show the "what do you want to do with this file" dialog

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P2)

Description

[:Gijs (he/him)]

Given that a bunch of the feedback in bug 1749998 and its dupes relate to use of the image/link/page saving (context) menus/shortcuts, we should just not show the panel for users of those entrypoints, irrespective of the browser.download.alwaysOpenPanel pref.

Comment 1

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

The severity field is not set for this bug.
:mak, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Neil Deakin]

Am I correct here in assuming that the when we get to the point where we decide to open the download panel, we don't know how the download was invoked?

Comment 3

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #2)

Am I correct here in assuming that the when we get to the point where we decide to open the download panel, we don't know how the download was invoked?

Yes; I think we need the patch in bug 1762033 so that we store info on the download object; that will put us much closer to the creation of the Download object itself, though that in and of itself could still not be close enough (ie we might need additional levels of information passing)

Comment 4

[Neil Deakin]

So the only flag that seems available that indicates that this is a user initiated save is when the content policy type is TYPE_SAVEAS_DOWNLOAD. This is available in the loadinfo of the channel which we can inspect when the download starts. However sometimes this flag is overriden (such as for images).

One possibility is adding a new flag, however that needs to be propagated through various nsWebBrowserPersist methods. But perhaps all uses of nsWebBrowserPersist shouldn't show the download panel -- there seem to be quite a few callers so I'm not sure.

The question is also which cases do we want to distinguish here, that is, what situations should the panel always be shown?

Comment 5

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #4)

So the only flag that seems available that indicates that this is a user initiated save is when the content policy type is TYPE_SAVEAS_DOWNLOAD. This is available in the loadinfo of the channel which we can inspect when the download starts. However sometimes this flag is overridden (such as for images).

Interesting, how is it overridden for images?

One possibility is adding a new flag, however that needs to be propagated through various nsWebBrowserPersist methods. But perhaps all uses of nsWebBrowserPersist shouldn't show the download panel -- there seem to be quite a few callers so I'm not sure.

The question is also which cases do we want to distinguish here, that is, what situations should the panel always be shown?

I think anything where the webpage initialized a download by returning a content-type/disposition combination that we can't display and therefore download, should continue to show the panel. I also think add-ons initiating downloads should default to showing them, at least for now.

Does that help?

Comment 6

[Neil Deakin]

Interesting, how is it overridden for images?

It is modified at https://searchfox.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#311

Does that help?

Perhaps. I think I will need to make a more comprehensive list of save points.

Comment 7

[Neil Deakin]

Shane, did your needinfo here mean that you had intended to look into this bug?

Comment 8

[Shane Hughes [:aminomancer]]

(In reply to Neil Deakin from comment #7)

Shane, did your needinfo here mean that you had intended to look into this bug?

Yeah, I didn't notice this was assigned. Just wanted to remind myself to check on it since it might involve the flag from bug 1739348

Comment 9

[Neil Deakin]

I found the following entry points for webbrowserpersist:

1. when dropping a file onto the desktop
2. open an external editor from view source
3. save a json file from devtools
4. save a media file from the page info dialog
5. save a video, video frame or audio from the context menu
6. save an image or canvas from the context menu
7. save a link from the context menu
8. when Save Frame As is selected from the context menu
9. when Save Page As is selected from the context menu
10. when Save Page As is selected from the main menu
11. when the Save Page toolbar button is pressed
12. when the 'browser.altClickSave' preference is true and a link is alt + clicked
13. when dropping a link onto the download toolbar button
14. when the download button in the pdf viewer is pressed
15. when the save message is sent from the system
16. when setting a desktop background on Mac

All of these are user-initiated and I think most of these would be ok to not show the panel. Some are rare cases that users wouldn't invoke often.

Extensions should be ok too as they set the show panel flag themselves.

Comment 10

[Neil Deakin]

Attachment: Bug 1761265, don't show the downloads panel when a download was started by user action that they expect will save the file, r=mhowell

The download panel should still appear when clicking on download links or those with content-disposition: attachment

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1fb70bd6ad60
don't show the downloads panel when a download was started by user action that they expect will save the file, r=mhowell,necko-reviewers,kershaw

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/1fb70bd6ad60

Comment 13

[Ryan VanderMeulen [:RyanVM]]

What do you think about taking this on ESR102? It grafts cleanly and it might be nice to get some of these download panel fixes landed there before ESR91 users get migrated over to ESR102 in a couple weeks.

Comment 14

[Neil Deakin]

Comment on attachment 9279111 [details]
Bug 1761265, don't show the downloads panel when a download was started by user action that they expect will save the file, r=mhowell

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Download panel in cases where a user was just trying to save a file.
• User impact if declined: Download panel appears too frequently when not expected.
• Fix Landed on Version: 103
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only affects a cosmetic display of the downloads panel

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9279111 [details]
Bug 1761265, don't show the downloads panel when a download was started by user action that they expect will save the file, r=mhowell

Approved for 102.3esr.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/d5a4ffd1e666

Comment 17

[Adrian Florinescu [:aflorinescu]]

Comment 9 lists the download points for which download pannel shouldn't be shown, not a list of download points which were fixes in this bug/patch. Hence the verification also covers that status in the latest versions + the behaviour in case of download.panel.shown.

Excluding point 15 from comment 9 which got no usecase, all behaviors with download.panel.shown = true are verified with 102.3.0esr, 104.0.2, 105.0b7, 106.0a1 (2022-09-06) on Windows 10, Ubuntu 22 and Mac 10.15.

The behavior for case 4 with download.panel.shown = false seems regressing: (the download panel is not shown if there's first download ever), but we'll sort that separatelly, it's rather an edge case and not a showstopper for this uplift.

Comment 18

[Shane Hughes [:aminomancer]]

(In reply to Adrian Florinescu [:aflorinescu] from comment #17)

The behavior for case 4 with download.panel.shown = false seems regressing: (the download panel is not shown if there's first download ever), but we'll sort that separatelly, it's rather an edge case and not a showstopper for this uplift.

I think that's expected. I added a separate check so that if we explicitly pass openDownloadsListOnStart: false, we don't show the panel. Currently, the only important difference with the first download ever is that it basically ignores the pref browser.download.alwaysOpenPanel being set to false.

That was to fix 2 issues: download panel opening in windows that were not active, which subverted the patch for bug 1741076; and an empty download panel automatically opening if the first download ever was created by an extension (bug 1759231). The first download ever was effectively an exception to the behavior implemented by those patches

Comment 19

[Adrian Florinescu [:aflorinescu]]

Now that makes sense since the page info is separate, so indeed it might endup in a scenario where the main window is minimized. Thanks for taking time to clarify.