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.

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.

in litmus for 2 cases: 1) with "show the DM when downloading files" checked 2) with "show the DM when downloading files" unchecked

(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

Shouldn't be that hard to fix, but hardly a new bug afaict, so not blocking.

I can't reproduce this in 2.0, certainly seems like a new bug to me.

ok, interesting. Still not a blocker, its definitely an odd edge case.

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

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.

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

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

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

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

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

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

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?

note: this may get fixed by bug 413093

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.

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.

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.

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

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.

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.

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

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.

Best bet is to add an optional aReason argument - let's not add any methods.

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.

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.

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!

Commented out the ok line with TODO comment.

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

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

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.

Just testing the one thing for now - someone else can come in with more extensive tests later.

(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?

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

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

Incorporates test update, review comments.

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.

for checkin

Checking in xpcom/tests/unit/test_nsIMutableArray.js; initial revision: 1.1

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

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

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

documentation updated

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.

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

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.

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

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