Closed Bug 748381 Opened 12 years ago Closed 12 years ago

Downloads indicator should persist in the toolbar (not auto-hide)

Categories

(Firefox :: Downloads Panel, defect)

Product:
Firefox
Component:
Downloads Panel
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 17

People

(Reporter: fryn, Assigned: Paolo)

References

Details

Attachments

(1 file, 2 obsolete files)

Update the behavior and show the button on Nightly migration
12.72 KB, patch
asaf
: review+
Details | Diff | Splinter Review 
#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.
Frank, use the existing #downloads-button if possible so that this does not break themes and addons.
(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.
(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.
(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.
(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 ?
(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.
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.
(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.
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?
The feature needs polish, we are finalizing the interaction details right now.
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)
Now the indicator must be moved to the toolbar explicitly to activate the panel.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #630683 - Flags: review?(mak77)
Summary: Downloads indicator should be persist in the toolbar → Downloads indicator should persist in the toolbar (not auto-hide)
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.
(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.
(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.
(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.
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!
Attachment #630683 - Attachment is obsolete: true
Attachment #630683 - Flags: review?(mak77)
Attachment #646850 - Flags: review?(mak77)
Attachment #646850 - Flags: review?(mak77) → review?(mano)
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.
Attachment #646850 - Flags: review?(mano) → feedback+
(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!
Attachment #646850 - Attachment is obsolete: true
Attachment #649596 - Flags: review?(mano)
(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 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.
Attachment #649596 - Flags: review?(mano) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a86ecc9f22ad
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/mozilla-central/rev/a86ecc9f22ad
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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 !
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: