Download Manager ("Downloads") window doesn't stay open when clicking on download-complete alert notification with auto-close pref ("close when done")

VERIFIED FIXED in mozilla1.9

Status

()

Toolkit
Downloads API
P2
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: stephend, Assigned: sdwilsh)

Tracking

({addon-compat, dev-doc-complete})

Trunk
mozilla1.9
addon-compat, dev-doc-complete
Points:
---
Bug Flags:
blocking1.9 +
wanted1.9 +
in-testsuite +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

11 years ago
Summary: Download Manager ("Downloads") window doesn't stay open when clicking on alert-notification dialog with auto-close pref (my nomination for the most verbose summary!)

Steps to Reproduce:

1. Under Tools | Options | Main | Download, check "Close it when all downloads are finished"; click OK (close the window on Mac).
2. Start file download.
3. Wait for the download to complete.
4. Observe the "Downloads Complete" notification slider in the lower right of your screen.
5. Click on the "All files have finished downloading" link.

Expected Results:

The "Downloads" window should appear, be focused, and remain until minimized or closed.

Actual Results:

The "Downloads" window briefly appears, but just as quickly disappears.
(Reporter)

Comment 1

11 years ago
Build ID: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007092804 Minefield/3.0a9pre

Also, this is an outcropping from a misunderstanding in bug 395708.
autoRemoveAndClose() is called from Startup(), and closes the DM if the pref is set and it hasn't detected "user action" and if the window's opener is nonexistent or has the same URI as the current window. I don't understand this code but it seems pretty likely that it's what's causing the DM window to close as soon as you open it.
(Reporter)

Updated

11 years ago
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
in litmus for 2 cases:
1) with "show the DM when downloading files" checked
2) with "show the DM when downloading files" unchecked
Flags: in-litmus?
(Reporter)

Comment 4

11 years ago
(In reply to comment #3)
> in litmus for 2 cases:
> 1) with "show the DM when downloading files" checked

http://litmus.mozilla.org/show_test.cgi?id=4657

> 2) with "show the DM when downloading files" unchecked

http://litmus.mozilla.org/show_test.cgi?id=4710

Flags: in-litmus? → in-litmus+

Updated

11 years ago
OS: Windows Vista → All

Updated

11 years ago
Target Milestone: --- → Firefox 3 M11

Updated

11 years ago
Priority: -- → P5
Shouldn't be that hard to fix, but hardly a new bug afaict, so not blocking.
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Priority: P5 → P4
Target Milestone: Firefox 3 Mx → Firefox 3 M11
I can't reproduce this in 2.0, certainly seems like a new bug to me.
Flags: blocking-firefox3- → blocking-firefox3?
Summary: Download Manager ("Downloads") window doesn't stay open when clicking on alert-notification dialog with auto-close pref → Download Manager ("Downloads") window doesn't stay open when clicking on download-complete alert notification with auto-close pref ("close when done")
Duplicate of this bug: 405112
ok, interesting.

Still not a blocker, its definitely an odd edge case.
Flags: blocking-firefox3? → blocking-firefox3-

Comment 9

11 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112905 Minefield/3.0b2pre

I can reproduce this exactly as described in Comment #1 
Duplicate of this bug: 407905

Comment 11

11 years ago
I hope this would be fixed before general release; it's the sort of thing every (Windows?) FF user will notice if they click on the popup alert, which by default always appears. I wouldn't call it an edge case.
(Assignee)

Comment 12

11 years ago
It's an edge case because the pref isn't set this way by default.  Most users will never hit this.

Comment 13

11 years ago
browser.download.manager.showAlertOnComplete seems to be set to _true_ by default, as I didn't explicitly set it in a user.js, about:config doesn't present the name/value in boldface (indicating it'd been changed from default), and I don't see anything in the GUI prefs to toggle this.

I'm not harping -- I am trying to reconcile your (sdwilsh) statement with my conflicting observation. My browser version is:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2
(Reporter)

Comment 14

11 years ago
There are two prefs, and I believe Shawn was referring to browser.download.manager.closeWhenDone, set in http://lxr.mozilla.org/seamonkey/source/browser/app/profile/firefox.js#216 to |false|.

Comment 15

11 years ago
Thanks for the clarification. For me, browser.download.manager.closeWhenDone is also the default value, so my original post (comment #11) still obtains...
(Assignee)

Comment 16

11 years ago
(In reply to comment #15)
> Thanks for the clarification. For me, browser.download.manager.closeWhenDone is
> also the default value, so my original post (comment #11) still obtains...
Then you are experiencing a different bug - please file a new one with steps to reproduce because this bug is dealing with browser.download.manager.closeWhenDone set to true.

Comment 17

11 years ago
(In reply to comment #16)

Looks like it's an error on my part, very sorry -- I had to retrace my steps with the prefs. I apparently see the same bug as stephend, with browser.download.manager.closeWhenDone indeed set to true.
(Reporter)

Updated

11 years ago
Duplicate of this bug: 419546
(Reporter)

Updated

11 years ago
Keywords: helpwanted
Duplicate of this bug: 420021

Comment 20

11 years ago
I would just like to comment that the links name being in the title might help with dup's,  I had trouble finding it amongst all the bugs.

"All files have finished downloading"  instead of download-complete alert notification or additional to it would possibly help?
(Assignee)

Comment 21

11 years ago
note:  this may get fixed by bug 413093
(Reporter)

Updated

11 years ago
Duplicate of this bug: 420169

Comment 23

10 years ago
So I've been testing Firefox 3 beta 4 today (29/02/2008 - testday) and I confirm that this bug still exists on b4pre.

So the target milestone has to be changed as well as it (ff3b3) has long been passed without this bug being solved.
(Assignee)

Updated

10 years ago
Duplicate of this bug: 420417

Comment 25

10 years ago
This may be an "edge case", but I think it's a pretty big edge, and it's definitely a regression from 2 and was one of the first things I noticed when jumping to B3. It's a dumb little bug that has a lot of visibility potential. "Open DL Manager link doesn't!" I say dumb little bug because it's not catastrophic, not because I'm saying anyone here is dumb nor little. Not that there's anything wrong with that.
(Reporter)

Comment 26

10 years ago
I'm re-requesting blocking.  Although dup-count is low, we get a lot of feedback about this being broken every testday (and, in #qa on other occasions); I expect the same feedback come ship day, because these are the pref values we ship by default.
Flags: blocking-firefox3- → blocking-firefox3?
(Reporter)

Comment 27

10 years ago
making comment early in the am--

http://mxr.mozilla.org/seamonkey/source/browser/app/profile/firefox.js#231: pref("browser.download.manager.closeWhenDone", false);

It's actually false by default, but I think folks will discover the pref, change it to true, and be bitten (but I might be wrong, not sure if we know what most users do).

Updated

10 years ago
Assignee: nobody → sdwilsh
Flags: blocking-firefox3? → blocking-firefox3+
Priority: P4 → P2
(Assignee)

Updated

10 years ago
Whiteboard: [needs patch][swag 3hr]
Target Milestone: Firefox 3 beta3 → Firefox 3 beta5

Comment 28

10 years ago
Previous version of Firefox will open the actual file that is downloaded when clicked on the link that appears in the notification box. In Firefox 3 beta 4 when you click this link the download manager window pops up for a second and then disappear.

Not the end of the world but for the people who are used to clicking this link after every download so they don't have to open the download manager window will be so annoyed.
(Assignee)

Updated

10 years ago
Duplicate of this bug: 423108

Comment 30

10 years ago
The problem is caused by the alertclickcallback notification that triggers a dmui->Show(nsnull, 0) which has a null window, so it looks kinda like the auto-open.

I tried making the alert box give its window as the aSubject, but because we use growl on os x, we can't quite pass in the window... I think?

Alternatively, we can specially detect that Show(nsnull, 0) is being called and know that it's not an auto-opened show because nsDownloadProxy will call Show with the id of the download.
Target Milestone: Firefox 3 beta5 → Firefox 3
(Assignee)

Comment 31

10 years ago
We should probably just special case this.  I can't think of a better solution currently :/

Edward - will you have time for this?  I can't see myself having time to code and write a test for this for at least three weeks...
(Assignee)

Updated

10 years ago
Duplicate of this bug: 358738
I went through the control flow related to this bug and have documented it here: http://wiki.mozilla.org/User:Brahmana/Downloads_Complete_Alert_Control_Flow

As stated in the document there the variable gUserInteracted is not being used at all. I think we can use this variable to solve this bug as we know for sure that the user has clicked the alert link. We can set the gUserInteracted to true before the nsIDMUI->show() is called. As this is a JS variable we cannot obviously set it DM code where the show() is called. 

My solution here is to change the nsIDownloadManagerUI interface, in 2 ways:

1. Change the signature of the Show() function to accept a third boolean argument telling whether this Show() is because of an user action. So in the DM we would call it as: dmui->show(nsnull, 0, PR_TRUE);

This might break any consumers of this interface.

2. Add another wrapper for the above function to the interface, say something like: ShowPersistent() or ShowOnUserAction() which has the same signature as Show(). It will just set the gUserInteracted to true before calling Show() and after that set it back to false. So we will use this function for this alertclickcallback case and wherever else required.
Status: NEW → ASSIGNED
(Assignee)

Comment 34

10 years ago
Best bet is to add an optional aReason argument - let's not add any methods.
Keywords: late-compat

Comment 35

10 years ago
Created attachment 312044 [details] [diff] [review]
testcase v1

This test passes without setting the close-when-done pref to true. I.e., it fails per this bug when the pref is set to false.
(Assignee)

Comment 36

10 years ago
Created attachment 312049 [details] [diff] [review]
v0.1

Work in progress.

Good analysis Brahmana.  It's a bit late in the game to be adding methods I think, and using reasons in method's is pretty normal in our codebase.

I haven't had a chance to run Edward's test for this (I have class!) - but maybe I will later today.  Someone else should feel free to try it if they want.  I don't think it will pass just yet.

Still TODO:
write an xpcom unit test ensuring that null elements are appended to nsIMutableArrays (per request of bsmedberg).
make downloads.js use the reason
get a test to test already open and calling show again with user interacted reason.
(Assignee)

Comment 37

10 years ago
Comment on attachment 312044 [details] [diff] [review]
testcase v1

>+    ok(dmui.visible, "Download Manager stays open on alert click");
change that to a todo, and land
r=sdwilsh

Thanks for the test!
Attachment #312044 - Flags: review+

Comment 38

10 years ago
Created attachment 312085 [details] [diff] [review]
testcase v1.1

Commented out the ok line with TODO comment.
Attachment #312044 - Attachment is obsolete: true

Comment 39

10 years ago
Kinda testcase+.. make sure to uncomment the TODO and leave the ok ;)

Checking in toolkit/mozapps/downloads/tests/browser/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/Makefile.in,v  <--  Makefile.in
new revision: 1.21; previous revision: 1.20
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_397935.js,v
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_397935.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_397935.js,v  <--  browser_bug_397935.js
initial revision: 1.1
done
Flags: in-testsuite+
Hardware: PC → All
(Assignee)

Comment 40

10 years ago
Created attachment 312089 [details] [diff] [review]
v1.0

This passes Edward's test when locally applied.  Requesting sr on this since there is an API change.

I've found a number of bugs while looking into this as well:
1) We never actually used any of the arguments passed into us in downloads.js.  We now use one, but the others are not used...
2) gUserInteracted is not set to true anywhere - probably should do that if any command is executed.

I think both of those should be fixed in follow-ups though since they aren't terribly serious.

Patch coming soon with xpcom unit test...
Attachment #312049 - Attachment is obsolete: true
Attachment #312089 - Flags: superreview?(shaver)
Attachment #312089 - Flags: review?(edilee)
(Assignee)

Updated

10 years ago
Attachment #312089 - Attachment is patch: true
Attachment #312089 - Attachment mime type: application/octet-stream → text/plain

Comment 41

10 years ago
Comment on attachment 312089 [details] [diff] [review]
v1.0

>+++ b/toolkit/components/downloads/public/nsIDownloadManagerUI.idl
>  /**
>   * Shows the Download Manager's UI to the user.
>   *
>-  * @param aWindowContext
>+  * @param [optional] aWindowContext
>   *        The parent window context to show the UI.
>-  * @param aID
>+  * @param [optional] aID
>   *        The id of the download to be preselected upon opening.
>+  * @param [optional] aReason
>+  *        The reason to show the download manager's UI.  This defaults to
>+  *        REASON_USER_INTERACTION, and should be one of the previously listed
>+  *        constants.
>   */
nit: REASON_USER_INTERACTED not INTERACTION

>+++ b/toolkit/components/downloads/src/nsDownloadManagerUI.js
>+  show: function show(aWindowContext, aID, aReason)
>   {
Because the arguments are optional, we should make sure we assign some useful defaults. aWindowContext to null, aID to 0?, aReason to USER_INTERACTED
>     // First we see if it is already visible
>-    if (this.recentWindow) {
>-      this.recentWindow.focus();
>+    let window = this.recentWindow;
>+    if (window) {
>+      window.focus();
>+      
>+      // If we are being asked to show again, with a user interaction reason,
>+      // set the appropriate variable.
>+      if (aReason == Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED)
>+        window.gUserInteracted = true;
Either check aReason != NEW_DOWNLOAD or rely on setting the correct default if we don't get called with aReason.
Attachment #312089 - Flags: review?(edilee) → review+
(Assignee)

Comment 42

10 years ago
Created attachment 312093 [details] [diff] [review]
xpcom test v.10

Just testing the one thing for now - someone else can come in with more extensive tests later.
Attachment #312093 - Flags: superreview?(benjamin)
Attachment #312093 - Flags: review?(benjamin)

Comment 43

10 years ago
(In reply to comment #41)
> Because the arguments are optional, we should make sure we assign some useful
> defaults. aWindowContext to null, aID to 0?, aReason to USER_INTERACTED
Actually.. perhaps nevermind. xpcommagic will correctly give 0/null for these things? Unless someone else from JSland is calling nsIDownloadManagerUI.show?
(Assignee)

Comment 44

10 years ago
(In reply to comment #43)
> (In reply to comment #41)
> > Because the arguments are optional, we should make sure we assign some useful
> > defaults. aWindowContext to null, aID to 0?, aReason to USER_INTERACTED
> Actually.. perhaps nevermind. xpcommagic will correctly give 0/null for these
> things? Unless someone else from JSland is calling nsIDownloadManagerUI.show?
Yup - xpconnect magic FTW.  That's actually how optional arguments are supposed to work, and that's why also why REASON_USER_INTERACTED is set to 0.

(In reply to comment #41)
> nit: REASON_USER_INTERACTED not INTERACTION
whoops - fixed locally.  Will attach a new patch after sr.
(Assignee)

Comment 45

10 years ago
Comment on attachment 312085 [details] [diff] [review]
testcase v1.1

>+    // TODO bug 397935 ok(dmui.visible, "Download Manager stays open on alert click");
I meant the todo function call instead of ok ;)
(Assignee)

Updated

10 years ago
Whiteboard: [needs patch][swag 3hr] → [has patch][needs sr shaver]
(Assignee)

Comment 46

10 years ago
Created attachment 312112 [details] [diff] [review]
v1.1

Incorporates test update, review comments.
Attachment #312089 - Attachment is obsolete: true
Attachment #312112 - Flags: superreview?(shaver)
Attachment #312089 - Flags: superreview?(shaver)

Updated

10 years ago
Attachment #312093 - Flags: superreview?(benjamin)
Attachment #312093 - Flags: superreview+
Attachment #312093 - Flags: review?(benjamin)
Attachment #312093 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #312112 - Flags: superreview?(shaver) → superreview?(mconnor)
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs sr shaver] → [has patch][needs sr mconnor]
Comment on attachment 312112 [details] [diff] [review]
v1.1

sr=mconnor, I would probably explicitly check aReason in the impl, rather than relying on xpconnect to clean up optional input.
Attachment #312112 - Flags: superreview?(mconnor) → superreview+
(Assignee)

Comment 48

10 years ago
Created attachment 313181 [details] [diff] [review]
v1.2

for checkin
Attachment #312112 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs sr mconnor] → [has patch][has reviews][can land]
(Assignee)

Comment 49

10 years ago
Checking in xpcom/tests/unit/test_nsIMutableArray.js;
initial revision: 1.1
(Assignee)

Comment 50

10 years ago
Checking in toolkit/components/downloads/public/nsIDownloadManagerUI.idl;
new revision: 1.2; previous revision: 1.1
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.181; previous revision: 1.180
Checking in toolkit/components/downloads/src/nsDownloadManagerUI.js;
new revision: 1.8; previous revision: 1.7
Checking in toolkit/components/downloads/src/nsDownloadProxy.h;
new revision: 1.24; previous revision: 1.23
Checking in toolkit/mozapps/downloads/content/downloads.js;
new revision: 1.142; previous revision: 1.141
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_397935.js;
new revision: 1.2; previous revision: 1.1
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: helpwanted → dev-doc-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews][can land]
(Assignee)

Comment 51

10 years ago
mfinkle - not sure if you want to post about this like you have for other potentially breaking changes (although this one probably won't break anyone!)
(Assignee)

Updated

10 years ago
Duplicate of this bug: 426682
(Reporter)

Comment 53

10 years ago
Verified FIXED using:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008040305 Minefield/3.0pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008040304 Minefield/3.0pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008040304 Minefield/3.0pre
Status: RESOLVED → VERIFIED
documentation updated
Keywords: dev-doc-needed → dev-doc-complete

Comment 55

10 years ago
can confirm the bug is still in version 3.0b5
(In reply to comment #55)
> can confirm the bug is still in version 3.0b5
> 

Hi Matthias, the patch was checked-in after Beta 5 was released, so this should be fixed in the coming RC1 Release. 
(Reporter)

Updated

10 years ago
Duplicate of this bug: 431162
(Reporter)

Updated

10 years ago
Duplicate of this bug: 431518

Comment 59

10 years ago
Can you please put this bug on the "known issues" section on the firefox 3 download page please.

http://www.mozilla.com/en-US/firefox/3.0b5/releasenotes/#issues

Comment 60

10 years ago
Liam: I doubt there will be any changes to the beta 5 page at this point. The first release candidate will be out soon, and this bug has been fixed since beta 5.

Comment 61

10 years ago
is there a set date for the first RC? even better would be a link to a page display when RC1, RC2, etc.. will be released. Thanks

(Assignee)

Comment 62

10 years ago
(In reply to comment #61)
> is there a set date for the first RC? even better would be a link to a page
> display when RC1, RC2, etc.. will be released. Thanks
They will be released when they are ready - we don't know exact dates as of yet.
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.