Last Comment Bug 748381 - Downloads indicator should persist in the toolbar (not auto-hide)
: Downloads indicator should persist in the toolbar (not auto-hide)
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Downloads Panel (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 17
Assigned To: :Paolo Amadini [Away]
:
Mentors:
Depends on:
Blocks: DownloadsPanel 746591 746751 746772 747808
  Show dependency treegraph
 
Reported: 2012-04-24 08:32 PDT by Frank Yan (:fryn)
Modified: 2013-11-12 00:57 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Update the behavior of the Downloads indicator. (7.76 KB, patch)
2012-06-06 13:03 PDT, :Paolo Amadini [Away]
no flags Details | Diff | Review
Update the behavior and show the button on Nightly migration (12.24 KB, patch)
2012-07-28 07:52 PDT, :Paolo Amadini [Away]
asaf: feedback+
Details | Diff | Review
Update the behavior and show the button on Nightly migration (12.72 KB, patch)
2012-08-07 04:14 PDT, :Paolo Amadini [Away]
asaf: review+
Details | Diff | Review

Description Frank Yan (:fryn) 2012-04-24 08:32:34 PDT
#downloads-indicator should be add to #nav-bar's defaultset between #search-container and #home-button, and it shouldn't be collapsed when there are no downloads in its list.

Limi, Shorlander, and I discussed this and agreed on this design, but we failed to communicate it in time for the initial release.

I'd be happy to write the patch for it, if someone could point me to all the bits that need to be modified.
Comment 1 Siddhartha Dugar [:sdrocking] 2012-04-24 08:40:36 PDT
Frank, use the existing #downloads-button if possible so that this does not break themes and addons.
Comment 2 :Paolo Amadini [Away] 2012-04-24 09:03:35 PDT
(In reply to Frank Yan (:fryn) from comment #0)
> #downloads-indicator should be add to #nav-bar's defaultset between
> #search-container and #home-button

Yes, probably we'll do that at release time. The current behavior is a
compromise to ensure that enabling and disabling the feature does not leave
any traces (bug 726444 comment 23). See also the status page here:

https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager/Status

> and it shouldn't be collapsed when there are no downloads in its list.

This is something we should discuss, there are pros and cons to both approaches.
Comment 3 Guillaume C. [:ge3k0s] 2012-04-24 10:34:05 PDT
(In reply to Frank Yan (:fryn) from comment #0)
> #downloads-indicator should be add to #nav-bar's defaultset between
> #search-container and #home-button, and it shouldn't be collapsed when there
> are no downloads in its list.
> 
> Limi, Shorlander, and I discussed this and agreed on this design, but we
> failed to communicate it in time for the initial release.

I agree with this. There will be even more space with home as an app tab.
Comment 4 Frank Yan (:fryn) 2012-04-24 10:37:46 PDT
(In reply to Guillaume C. [:ge3k0s] from comment #3)
> I agree with this. There will be even more space with home as an app tab.

Home is not going to be an app tab.
Comment 5 Guillaume C. [:ge3k0s] 2012-04-24 11:29:18 PDT
(In reply to Frank Yan (:fryn) from comment #4)
> (In reply to Guillaume C. [:ge3k0s] from comment #3)
> > I agree with this. There will be even more space with home as an app tab.
> 
> Home is not going to be an app tab.

Why that ? On every single Australis mockup it's an app tab and it was planned for phase 2. If it isn't the case, UX team should reconsider to have the home button by default on the nav toolbar : it isn't very useful in that form and it could be replaced by the download button then. 

What is gonna be phase 2 then ? Shouldn't it be more interesting to merge the new tab and the home tab ?
Comment 6 Frank Yan (:fryn) 2012-04-24 11:32:47 PDT
(In reply to Guillaume C. [:ge3k0s] from comment #5)
> Shouldn't it be more interesting to merge
> the new tab and the home tab ?

There is no such thing as the home tab.
We're gonna merge the content of about:home into about:newtab and redirect about:home to about:newtab.
Comment 7 Guillaume C. [:ge3k0s] 2012-04-24 11:52:17 PDT
Thanks for the answer and sorry for the off topic. But if it is so the home button should clearly become optional to have (as Chrome currently does). In fact the home tab idea wasn't that great.
Comment 8 Frank Yan (:fryn) 2012-04-24 11:56:14 PDT
(In reply to Guillaume C. [:ge3k0s] from comment #7)
> But if it is so the home
> button should clearly become optional to have (as Chrome currently does).

It is optional.
Please stop spamming this bug. You're not being helpful.
Comment 9 Gingerbread Man 2012-04-26 10:39:28 PDT
May I ask for some clarification? I can't figure out if I'm experiencing a bug, a feature that will be polished at a later date, or intended behavior.

By default, there's no icon. When you press Ctrl+J, #downloads-indicator magically appears out of nowhere. Click on the page or on another UI element, and it disappears.

In the customize palette, there's another icon, #downloads-button. When this is placed on the toolbar, #downloads-indicator becomes the preceding sibling (while still not being visible). This icon is always visible, whether or not you have any downloads.

* In the end, will we be able to place the downloads icon in any position on the toolbar?
* In the end, will the icon hide itself when there are no active or new finished downloads?
Comment 10 Marco Bonardo [::mak] 2012-04-26 11:06:21 PDT
The feature needs polish, we are finalizing the interaction details right now.
Comment 11 Willy_ Foo_Foo 2012-04-27 07:49:36 PDT
Would love to be able to decide if the button is permanent or automatic
Auto hide just makes it worse to use the home button & bookmark button(if used in default place)
also would be able to easily disable the panel in whole
if want to use download window instead of the panel)
Comment 12 :Paolo Amadini [Away] 2012-06-06 13:03:13 PDT
Created attachment 630683 [details] [diff] [review]
Update the behavior of the Downloads indicator.

Now the indicator must be moved to the toolbar explicitly to activate the panel.
Comment 13 Marco Bonardo [::mak] 2012-07-25 14:57:49 PDT
Comment on attachment 630683 [details] [diff] [review]
Update the behavior of the Downloads indicator.

Review of attachment 630683 [details] [diff] [review]:
-----------------------------------------------------------------

So, the fact users will have to explicitly enable the panel is sort of an issue for now, in the sense we want people to test it but here we are removing it and they will have to figure out why and how to add it back...
It won't be a problem once we can change the currentSet, but I feel like now we should keep initializePlaceholder and temporarily use it to add the button for the session.
Comment 14 :Paolo Amadini [Away] 2012-07-27 09:29:42 PDT
(In reply to Marco Bonardo [:mak] (Away 28 Jul - 12 Aug) from comment #13)
> So, the fact users will have to explicitly enable the panel is sort of an
> issue for now, in the sense we want people to test it but here we are
> removing it and they will have to figure out why and how to add it back...

I agree.

> It won't be a problem once we can change the currentSet, but I feel like now
> we should keep initializePlaceholder and temporarily use it to add the
> button for the session.

Since this would break customization behavior (that is also something we want to
test now because it's supposedly final), I propose that we change the currentSet
if the profile is loaded in Nightly (with the feature enabled). People who use
Nightly regularly and didn't want the feature already had time to disable it.
People who don't use Nightly are not affected.
Comment 15 Marco Bonardo [::mak] 2012-07-27 09:40:32 PDT
(In reply to Paolo Amadini [:paolo] from comment #14)
> Since this would break customization behavior (that is also something we
> want to
> test now because it's supposedly final), I propose that we change the
> currentSet
> if the profile is loaded in Nightly (with the feature enabled). People who
> use
> Nightly regularly and didn't want the feature already had time to disable it.
> People who don't use Nightly are not affected.

The problem with that is that people may disable the unfinished feature, due to current bugs, and never get the polished version cause we already did the UI migration. It's sort of a minor issue though, since I expect nightly testers to be able to manage their customization (hopefully).
The major problem though is that it complicates the Aurora merge, cause we don't want to do the migration until we are sure the feature is shipping.
Comment 16 :Paolo Amadini [Away] 2012-07-27 09:48:59 PDT
(In reply to Marco Bonardo [:mak] (Away 28 Jul - 12 Aug) from comment #15)
> The major problem though is that it complicates the Aurora merge, cause we
> don't want to do the migration until we are sure the feature is shipping.

We kind of figured out how to handle the preference switch in time, by landing in
the few days between the merge and the first build is done, helping tracking with
checkin-needed. In the worst case, if the first build has the feature enabled by
mistake, we can undo the customization in Aurora with specific code.
Comment 17 :Paolo Amadini [Away] 2012-07-28 07:52:10 PDT
Created attachment 646850 [details] [diff] [review]
Update the behavior and show the button on Nightly migration

This is the best compromise I could think of, to address the testing needs
outlined in comment 13. It doesn't guarantee 100% button migration coverage,
i.e. some users might still need to add the button manually, but also doesn't
affect the startup time of a blank Nightly profile.

A better approach, functionally, would have been to leave the "defaultset" in
browser.js without the button, and write initialization code that added the
button in the toolbar's currentset during startup, the first time the feature
was enabled (and only the first time, tracking with a separate preference, to
allow further customization and button removal in the usual way).

However, this would have caused a startup time impact, not in Aurora (because
the feature is disabled there) but in Nightly and for new profiles only, that
is actually what we're measuring continuously.

Using an ordinary migration task that's conditional on having the feature
enabled, instead, doesn't affect startup time in new profiles. However, it
executes the migration only if the profile is updated using Nightly (more
precisely, if you migrate while the feature is disabled, the button won't
appear the next time you use the profile on a build with the feature enabled
by default).

I wanted to be exhaustive, to better show the alternatives I already
considered. If you think there's another way that gives the final wanted
customization behavior, just let me know!
Comment 18 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-08-05 13:14:09 PDT
Comment on attachment 646850 [details] [diff] [review]
Update the behavior and show the button on Nightly migration

DIV_SEN_callback() {
>       if (this._notificationTimeout) {
>         clearTimeout(this._notificationTimeout);
>       }
> 
>+      // After loading the overlay, place the indicator in its final position.
>+      DownloadsButton.updatePosition();

Why that late?

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js
>@@ -1172,17 +1172,17 @@ BrowserGlue.prototype = {
>     var notifyBox = browser.getNotificationBox();
>     var notification = notifyBox.appendNotification(text, title, null,
>                                                     notifyBox.PRIORITY_CRITICAL_MEDIUM,
>                                                     buttons);
>     notification.persistence = -1; // Until user closes it
>   },
> 
>   _migrateUI: function BG__migrateUI() {
>-    const UI_VERSION = 6;
>+    const UI_VERSION = 7;
>     const BROWSER_DOCURL = "chrome://browser/content/browser.xul#";
>     let currentUIVersion = 0;
>     try {
>       currentUIVersion = Services.prefs.getIntPref("browser.migration.version");
>     } catch(ex) {}
>     if (currentUIVersion >= UI_VERSION)
>       return;
> 
>@@ -1270,16 +1270,46 @@ BrowserGlue.prototype = {
>       // convert tabsontop attribute to pref
>       let toolboxResource = this._rdf.GetResource(BROWSER_DOCURL + "navigator-toolbox");
>       let tabsOnTopResource = this._rdf.GetResource("tabsontop");
>       let tabsOnTopAttribute = this._getPersist(toolboxResource, tabsOnTopResource);
>       if (tabsOnTopAttribute)
>         Services.prefs.setBoolPref("browser.tabs.onTop", tabsOnTopAttribute == "true");
>     }
> 
>+    // This migration step is executed only if the Downloads Panel feature is
>+    // enabled.  This usually happens when the profile is updated in Nightly.

"usually"? What are the other cases?

>+    if (currentUIVersion < 7 &&
>+        !Services.prefs.getBoolPref("browser.download.useToolkitUI")) {
>+      // This code adds the customizable downloads buttons.
>+      let currentsetResource = this._rdf.GetResource("currentset");
>+      let toolbarResource = this._rdf.GetResource(BROWSER_DOCURL + "nav-bar");
>+      let currentset = this._getPersist(toolbarResource, currentsetResource);
>+
...
>+ ... currentset.replace(

It would be really really nice to introduce some helpers here (scope in _migrateUI) to handle this stuff. A lot of code (hard enough to follow) is copied here. I guess there was a good reason to use regexps here rather than split-and-join. Ugh.

But only if you feel like... :) This can go in a follow up.

>+      // Need to migrate only if toolbar is customized and the element is not found.

Hrm, this is somewhat confusing.  Maybe "Migration is necessary only necessary if the navigation/main toolbar was changed not to include the downloads button". Feel free to improve that. My English isn't that good!

>+      if (currentset &&
>+          currentset.indexOf("downloads-button") == -1) {

So, to my biggest concern probably, what happens if the button was moved to some other toolbar?

>+        // The element is added after the search bar, before the home button, or
>+        // as a last resort just before the window controls.
>+        if (currentset.indexOf("search-container") != -1) {
>+          currentset = currentset.replace(/(^|,)search-container($|,)/,
>+                                          "$1search-container,downloads-button$2")
>+        }
>+        else if (currentset.indexOf("home-button") != -1) {
>+          currentset = currentset.replace(/(^|,)home-button($|,)/,
>+                                          "$1downloads-button,home-button$2")
>+        }
>+        else if (currentset.indexOf("window-controls") != -1) {
>+          currentset = currentset.replace(/(^|,)window-controls($|,)/,
>+                                          "$1downloads-button,window-controls$2")
>+        }

It would make sense just append it if the window-controls aren't there, just in case.
Comment 19 :Paolo Amadini [Away] 2012-08-07 04:14:42 PDT
Created attachment 649596 [details] [diff] [review]
Update the behavior and show the button on Nightly migration

(In reply to Mano from comment #18)
> >+      // After loading the overlay, place the indicator in its final position.
> >+      DownloadsButton.updatePosition();
> 
> Why that late?

I don't understand the question, why do you say late? updatePosition works on
the indicator element that we just loaded from the overlay.

> >+      if (currentset &&
> >+          currentset.indexOf("downloads-button") == -1) {
> 
> So, to my biggest concern probably, what happens if the button was moved to
> some other toolbar?

I'd say, the same thing that happened to the home and bookmarks button.

Probably it depends on the specific toolbar (menu bar may be special), but
generally the button will just move to the navigation bar.

> >+        else if (currentset.indexOf("window-controls") != -1) {
> >+          currentset = currentset.replace(/(^|,)window-controls($|,)/,
> >+                                          "$1downloads-button,window-controls$2")
> >+        }
> 
> It would make sense just append it if the window-controls aren't there, just
> in case.

Well, window-controls can't be removed. So the check for their existence is
unnecessary, thanks for pointing that out!
Comment 20 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-08-07 04:49:35 PDT
(In reply to Paolo Amadini [:paolo] from comment #19)
> Created attachment 649596 [details] [diff] [review]
> Update the behavior and show the button on Nightly migration
> 
> (In reply to Mano from comment #18)
> > >+      // After loading the overlay, place the indicator in its final position.
> > >+      DownloadsButton.updatePosition();
> > 
> > Why that late?
> 
> I don't understand the question, why do you say late? updatePosition works on
> the indicator element that we just loaded from the overlay.
> 

You're right. I read it wrong.

> > >+      if (currentset &&
> > >+          currentset.indexOf("downloads-button") == -1) {
> > 
> > So, to my biggest concern probably, what happens if the button was moved to
> > some other toolbar?
> 
> I'd say, the same thing that happened to the home and bookmarks button.
> 
> Probably it depends on the specific toolbar (menu bar may be special), but
> generally the button will just move to the navigation bar.

This is broken, and may result in unexpected bugs. However, you're right that it applies to all migration cases there. Mind to file a follow up on that?

> 
> > >+        else if (currentset.indexOf("window-controls") != -1) {
> > >+          currentset = currentset.replace(/(^|,)window-controls($|,)/,
> > >+                                          "$1downloads-button,window-controls$2")
> > >+        }
> > 
> > It would make sense just append it if the window-controls aren't there, just
> > in case.
> 
> Well, window-controls can't be removed. So the check for their existence is
> unnecessary, thanks for pointing that out!

Yup!
Comment 21 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-08-07 04:57:09 PDT
Comment on attachment 649596 [details] [diff] [review]
Update the behavior and show the button on Nightly migration

The very last nits:

> +    // toolbar, because the toolbar can be displayed later.

may

>+      // After loading the overlay, place the indicator in its final position.
>+      DownloadsButton.updatePosition();

Now that the overlay is loaded

>+      if (currentset &&
>+          currentset.indexOf("downloads-button") == -1) {
>+        // The element is added after the search bar, before the home button, or
>+        // as a last resort just before the non-customizable window controls.

either...or...As a last resort

r=mano.
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-08-25 19:29:13 PDT
https://hg.mozilla.org/mozilla-central/rev/a86ecc9f22ad
Comment 24 Jim Jeffery not reading bug-mail 1/2/11 2012-08-28 06:23:42 PDT
Was it intended that this bug change the color of the mini-progress meter on the download button from green to blue on win7 ?

The arrow panel is green, and the button indicator is 'blue' - looks odd !
Comment 25 Simona B [:simonab](PTO) 2012-11-12 09:04:11 PST
Verified on the latest Nightly that the Downloads button is located as default and is persistent on the Navigation bar (doesn't auto-hide).

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.7:
Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/19.0 Firefox/19.0        
Build ID: 20121111030749
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19.0 Firefox/19.0        
Build ID: 20121112030753
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:19.0) Gecko/19.0 Firefox/19.0        Build ID: 20121112030753

Note You need to log in before you can comment on or make changes to this bug.