Closed Bug 1739348 Opened 4 years ago Closed 3 years ago

When a filetype is set to "always ask" and the user makes a save/open choice in the dialog, we should not also open the downloads panel

Categories

(Firefox :: Downloads Panel, defect, P3)

Product:
Firefox
Component:
Downloads Panel
Platform:
Desktop
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
104 Branch
Tracking Flags:
Tracking Status
firefox-esr102 --- verified
firefox96 --- disabled
firefox97 --- disabled
firefox98 --- wontfix
firefox99 --- wontfix
firefox104 --- verified

People

(Reporter: Gijs, Assigned: aminomancer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fidefe-mr11-downloads])

Attachments

(1 file)

Bug 1739348 - Don't open downloads panel after download dialogs. r=Gijs
48 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-esr102+
Details | Review

As in summary. There's no need to add another prompt / interruption in this case - the motivation for showing the panel is to allow quickly opening the file, but in this case the user has already had an opportunity to set that up.

Severity: -- → S3
Priority: -- → P3
See Also: → 1740705
Whiteboard: [fidefe-mr11-downloads]
Assignee: nobody → mtigley
Status: NEW → ASSIGNED

Unassigning myself since I'll be leaving on PTO at the end of the week and don't think I'll have time to get to this. Will pick it up after the break if it hasn't already been picked up.

Assignee: mtigley → nobody
Status: ASSIGNED → NEW
Blocks: 1744297
No longer blocks: 1733587
Assignee: nobody → mtigley
Status: NEW → ASSIGNED
Assignee: mtigley → nobody
Status: ASSIGNED → NEW

Firefox 98 shipping soon, updating flags.

status-firefox96: affected → disabled
status-firefox97: --- → disabled
status-firefox98: --- → wontfix
status-firefox99: --- → affected
See Also: → 1758791

particularly if i have chosen open I do not need the panel appearing while the application is starting/focussing, the visual noise is a distraction
(mainly commenting to subscribe to progress of this bug). Thanks for recognizing this issue.

This is a medium sized patch to legacy download construction. It takes
advantage of the new property added in Bug 1762033 to prevent the
downloads panel from being automatically shown when a download is added
after an interaction with the unknown content type dialog or the file
picker dialog. I chose to not do the same for failed transfers since I
thought it might serve some use, but that might be wrong. I don't know
if there's a way to test the dialog that appears when you download an
executable without going through the same path I adjusted with the
patch. It seems like it's covered but I could be wrong. Also add a test
to cover these changes from the bottom up. Thanks and apologies for my
sloppy C++, though I'm sure I'll learn a lot more from the review 😅

Assignee: nobody → shmediaproductions
Status: NEW → ASSIGNED
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/076c2a81804a Don't open downloads panel after download dialogs. r=NeilDeakin

I think all except the first one are fixed by just adding [optional] to the parameter in nsITransfer.init so I'll upload that in a sec after testing.

The first one I'm having trouble reproducing, I think it might involve this focus check since it's only failing on the second subtest, which would correspond to this.panelHasShownBefore being set for the first time on a new job. Kinda makes the behavior less predictable I guess. But it still shouldn't fail unless the window's not active, there are unfinished downloads somehow, or the alwaysOpenPanel pref is somehow disabled. I'll try updating the test to control for all of those possibilities just in case.

Edit: I think I resolved it so please try landing again. If anyone knows a safe, cross-platform method to activate the test window (or to trick the focus service into thinking the window is active), I would feel better putting that into the test. This isn't really related to this bug but I can improve the test in a separate patch. I'm pretty sure that's the only way the subtest can fail, but it can't be evaded since that focus check is the same logic the subtest is supposed to cover.

Flags: needinfo?(shmediaproductions)
Depends on: 1762033

Edit: I think I resolved it so please try landing again. If anyone knows a safe, cross-platform method to activate the test window (or to trick the focus service into thinking the window is active), I would feel better putting that into the test. This isn't really related to this bug but I can improve the test in a separate patch.

There isn't any other window created at that point in the test so it should already be focused (the test harness ensures this). There's a later subtest in browser_downloads_panel_opens.js that handles inactive windows

(In reply to Neil Deakin from comment #8)

Edit: I think I resolved it so please try landing again. If anyone knows a safe, cross-platform method to activate the test window (or to trick the focus service into thinking the window is active), I would feel better putting that into the test. This isn't really related to this bug but I can improve the test in a separate patch.

There isn't any other window created at that point in the test so it should already be focused (the test harness ensures this). There's a later subtest in browser_downloads_panel_opens.js that handles inactive windows

Hmm, well it doesn't really make sense to me tbh, but I don't know how else to explain it other than that check, since everything else it's checking is practically static. But idk how this subtest could possibly be affected by the legacy download changes anyway, unless the subtest I added is silently failing in a way that bears on the customize mode subtest. Well I'll try the same task again and see how that goes

Edit: If I'm not mistaken, bug 1752066 has the same failure 4 months ago. I think you can land it, I'll try dealing with this test in that bug

Hi Neil, I think whatever is going on does involve a problem with window activity. I added some extra checks to the test, to see what might be failing in _notifyDownloadEvent:

  let afterCustomizationPromise = BrowserTestUtils.waitForEvent(
    gNavToolbox,
    "aftercustomization"
  );
  gCustomizeMode.exit();
  await afterCustomizationPromise;

  await TestUtils.waitForCondition(
    () => DownloadsPanel._state == DownloadsPanel.kStateHidden,
    "The panel should no longer be trying to open"
  );
  info(
    "Downloads in panel: " +
      DownloadsCommon.summarizeDownloads(
        DownloadsCommon.getData(window)?._downloads
      ).numDownloading
  );
  ok(
    BrowserWindowTracker.getTopWindow() == Services.focus.activeWindow,
    "Top window should be inactive"
  );
  ok(window.gAlwaysOpenPanel, "alwaysOpenPanel should be true");
  DownloadsCommon.getData(window)._notifyDownloadEvent("start");
  is(
    DownloadsPanel.isPanelShowing,
    true,
    "Panel state should indicate a preparation to be opened"
  );

and ran the job again, and this seems to be the issue:

[task 2022-05-18T19:55:26.043Z] 19:55:26     INFO - Entering test bound test_customizemode_doesnt_wreck_things
[task 2022-05-18T19:55:26.043Z] 19:55:26     INFO - try to open the panel (will not work, in customize mode)
[task 2022-05-18T19:55:26.044Z] 19:55:26     INFO - TEST-PASS | browser/components/downloads/test/browser/browser_downloads_panel_opens.js | Should not start opening the panel. - 
[task 2022-05-18T19:55:26.045Z] 19:55:26     INFO - Downloads in panel: 0
[task 2022-05-18T19:55:26.045Z] 19:55:26     INFO - Buffered messages finished
[task 2022-05-18T19:55:26.046Z] 19:55:26     INFO - TEST-UNEXPECTED-FAIL | browser/components/downloads/test/browser/browser_downloads_panel_opens.js | Top window should be inactive - 
[task 2022-05-18T19:55:26.046Z] 19:55:26     INFO - Stack trace:
[task 2022-05-18T19:55:26.046Z] 19:55:26     INFO - chrome://mochikit/content/browser-test.js:test_ok:1394
[task 2022-05-18T19:55:26.046Z] 19:55:26     INFO - chrome://mochitests/content/browser/browser/components/downloads/test/browser/browser_downloads_panel_opens.js:test_customizemode_doesnt_wreck_things:149
[task 2022-05-18T19:55:26.048Z] 19:55:26     INFO - TEST-PASS | browser/components/downloads/test/browser/browser_downloads_panel_opens.js | alwaysOpenPanel should be true - 
[task 2022-05-18T19:55:26.050Z] 19:55:26     INFO - Not taking screenshot here: see the one that was previously logged
[task 2022-05-18T19:55:26.051Z] 19:55:26     INFO - TEST-UNEXPECTED-FAIL | browser/components/downloads/test/browser/browser_downloads_panel_opens.js | Panel state should indicate a preparation to be opened - Got false, expected true
[task 2022-05-18T19:55:26.051Z] 19:55:26     INFO - Stack trace:
[task 2022-05-18T19:55:26.052Z] 19:55:26     INFO - chrome://mochikit/content/browser-test.js:test_is:1422
[task 2022-05-18T19:55:26.053Z] 19:55:26     INFO - chrome://mochitests/content/browser/browser/components/downloads/test/browser/browser_downloads_panel_opens.js:test_customizemode_doesnt_wreck_things:155
[task 2022-05-18T19:56:10.251Z] 19:56:10     INFO - Not taking screenshot here: see the one that was previously logged
[task 2022-05-18T19:56:10.252Z] 19:56:10     INFO - TEST-UNEXPECTED-FAIL | browser/components/downloads/test/browser/browser_downloads_panel_opens.js | Test timed out -

The check BrowserWindowTracker.getTopWindow() == Services.focus.activeWindow is failing, which seems to explain why _notifyDownloadEvent is not changing the state of the downloads panel. If I have understood correctly, that explains the whole failure. But I don't know why the top window would not be active.

It seems to not take a screenshot at the time of this failure because it already took one earlier (it tries to open the panel in customize mode -> expected console.error -> screenshot). So whether there's another window, or the window is just somehow not focused, or it's a problem with the focus manager, I don't know how to inquire much further

It's possible that something strange is happening when switching to/from customization mode where the window is being lowered. Perhaps the test harness window is focused, or perhaps nothing is.

Since this particular test isn't testing the focus behaviour you could always just call SimpleTest.promiseFocus on the window to try to force the window to be focused before continuing.

(In reply to Neil Deakin from comment #11)

It's possible that something strange is happening when switching to/from customization mode where the window is being lowered. Perhaps the test harness window is focused, or perhaps nothing is.

Since this particular test isn't testing the focus behaviour you could always just call SimpleTest.promiseFocus on the window to try to force the window to be focused before continuing.

Yeah, fair enough. I wasn't able to figure out the underlying cause, but after adding your suggestion, it's finally green. I also added some additional tests to make sure the behavior works correctly with the file picker dialog on "Save As" and with no dialogs, but I haven't modified the actual code (except to rebase it). Thanks!

Pushed by neil@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5e08d0841b10 Don't open downloads panel after download dialogs. r=NeilDeakin

Backed out for causing browser-chrome failures on browser_pdfjs_download_button.js, browser_tempfilename.js

Backout link

Push with failures

Failure log 1 // Failure log 2

Flags: needinfo?(shmediaproductions)

Thanks, I think I sorted out the failure. I'll check it again.
Edit: Here's the second job.

Flags: needinfo?(shmediaproductions)

(In reply to Shane Hughes [:aminomancer] from comment #15)

Thanks, I think I sorted out the failure. I'll check it again.
Edit: Here's the second job.

This looks happy - is the new version of the patch on phabricator and should we try to reland it?

Flags: needinfo?(shmediaproductions)

(In reply to :Gijs (he/him) from comment #16)

(In reply to Shane Hughes [:aminomancer] from comment #15)

Thanks, I think I sorted out the failure. I'll check it again.
Edit: Here's the second job.

This looks happy - is the new version of the patch on phabricator and should we try to reland it?

Yes, tests are updated. I don't know how it handles conflict resolution when you push to mozilla-central. I can rebase it again if that's an issue

Flags: needinfo?(shmediaproductions)
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b7ac1dc2e29e Don't open downloads panel after download dialogs. r=NeilDeakin
Regressions: 1773749
Flags: needinfo?(shmediaproductions) → needinfo?(gijskruitbosch+bugs)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:aminomancer, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Flags: needinfo?(shmediaproductions)
Flags: needinfo?(enndeakin)

Sorry for being slow here. I think I've worked out what the issue is.

Flags: needinfo?(shmediaproductions)
Flags: needinfo?(enndeakin)

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

What I think is happening is that a previous tasks triggers opening the downloads panel. But the code that does the opening can delay the actual opening of the popup, here and here.

So the test fails when a test that is supposed to open the popup starts opening it, and then a later test that expects the popup not to open notices the open popup and goes "hey, that was not supposed to happen".

At least locally, with the fixup patch that I pushed to try the test now passes --verify against a debug build. If it looks good on try I'll fold it in and reland.

Some of the confusion around how the downloads panel opens is described in bug 1747465. Hopefully we'll be able to simplify some of this code there, but we don't need to try to tackle it here, this patch is big enough already.

I fixed the original issue but linux opt TV is still sad. I don't really know why and I'm tempted to just merge in my changes and disable the test on linux TV. I don't think it's worth holding this up another release and soft freeze starts Thursday, so I think I'll do that...

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/67882bbbb8ca Don't open downloads panel after download dialogs. r=NeilDeakin

(In reply to :Gijs (he/him) from comment #23)

I fixed the original issue but linux opt TV is still sad. I don't really know why and I'm tempted to just merge in my changes and disable the test on linux TV. I don't think it's worth holding this up another release and soft freeze starts Thursday, so I think I'll do that...

Looks good, thanks. Hey, what does TV mean? I've been kinda mystified by some of the try job abbreviations, maybe there's a list somewhere?

https://hg.mozilla.org/mozilla-central/rev/67882bbbb8ca

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox104: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 104 Branch
See Also: → 1780187
Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Shane Hughes [:aminomancer] from comment #25)

(In reply to :Gijs (he/him) from comment #23)

I fixed the original issue but linux opt TV is still sad. I don't really know why and I'm tempted to just merge in my changes and disable the test on linux TV. I don't think it's worth holding this up another release and soft freeze starts Thursday, so I think I'll do that...

Looks good, thanks. Hey, what does TV mean? I've been kinda mystified by some of the try job abbreviations, maybe there's a list somewhere?

Oops, sorry this got a bit lost in my bugmail. TV means "test-verify". In general you can hover over jobs on treeherder and it'll show an expanded definition of the task. In this case that'll show "test-linux-1804-64-qr/opt-test-verify" - so it's a linux 64-bit webrender opt job running test verify, ie it's running the tests added/modified in the push using "verify mode", which is the same as the --verify switch on your local mach test or mach mochitest commands.

Gotcha, thanks. Not sure how I missed that 😅

When the user would click a "download" button/link for a specific file type that has been set to "Always ask", then chooses the option to "Open" or "Save", the downloads panel would open up. This happens in Release v103.0.2.

After the fix, if the user clicks a "download" button/link for a specific file type that he has set to "Always ask", then chooses the option to "Open"
or "Save", the downloads panel no longer opens, only does the expected action ("save" and show the download animation OR "open" and load the file in the chosen application.

This behavior has been verified using this test page for the following types: MP4, FLV, MKV, 3GP, JPG, PNG, GIF, MP3, XLS, PDF. Tested platforms are: Windows 10, Ubuntu 22, Mac OS 11. Beta v104.0 verified fixed.

Status: RESOLVED → VERIFIED
status-firefox104: fixed → verified
Flags: qe-verify+

What do you think about taking this on ESR102? It grafts cleanly (alongside a couple other bugs that I'm leaving the same basic comment in) and it might be nice to get some of these download fixes landed there before ESR91 users get migrated over to ESR102 in a couple weeks.

Flags: needinfo?(shughes)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #30)

What do you think about taking this on ESR102? It grafts cleanly (alongside a couple other bugs that I'm leaving the same basic comment in) and it might be nice to get some of these download fixes landed there before ESR91 users get migrated over to ESR102 in a couple weeks.

Sure, sounds good! I haven't worked on ESR before, is there anything I need to do?

Flags: needinfo?(shughes)

Nominate the patch for esr102 approval (same process as beta uplifts).

Comment on attachment 9274785 [details]
Bug 1739348 - Don't open downloads panel after download dialogs. r=Gijs

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: We made several enhancements to the downloads panel experience that eliminate redundant interruptions. This one prevents the panel from opening when a download starts if the download was started by the user manually choosing what to do with the download (Open or Save) in a dialog. The purpose of opening the panel automatically is to give the user an opportunity to perform these actions, but in this case the user already performed one of them.
  • User impact if declined: Users migrating from ESR91 to ESR102 will notice that the downloads panel now opens automatically when a download starts. If they have configured Firefox to ask them what to do with downloads, they may be irritated by Firefox effectively showing them double the number of prompts: first, the Open/Save dialog; and second, the downloads panel.
  • Fix Landed on Version: 104
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The non-test changes include adding a flag to track whether a dialog was opened, and passing that flag to the download constructor. So this particular patch is relatively minimal.
Attachment #9274785 - Flags: approval-mozilla-esr102?

(In reply to Shane Hughes [:aminomancer] from comment #33)

For it to work, we'll also need to uplift bug 1762033. Bug 1759231 is related but optional.

Both of those landed in 101, no?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #34)

(In reply to Shane Hughes [:aminomancer] from comment #33)

For it to work, we'll also need to uplift bug 1762033. Bug 1759231 is related but optional.

Both of those landed in 101, no?

You're right, my mistake. It's all a blur lol

Comment on attachment 9274785 [details]
Bug 1739348 - Don't open downloads panel after download dialogs. r=Gijs

Approved for 102.3esr.

Attachment #9274785 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+
Blocks: 1789290

This issue was also reproduced in ESR v102.2.0esr and the fix was verified in candidate ESR v102.3.0esr on Windows 10, Mac OS 11 and Ubuntu 22.

status-firefox-esr102: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: