814961 - Hide the summary in the Downloads Panel if there are no hidden downloads in progress or paused.

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Girish Sharma [:Optimizer]]

When the number of downloads do not increase the available limit, the last item says "Show all downloads" with some height, as number of downloads increases, the item changes to "+X other downloads" but the height increases significantly to match the height of other items.

I find it inconsistent with the previous state of the item, plus, visually the item looks better with a smaller height than the other items in the list.

Comment 1

[Marco Bonardo [:mak]]

I thought bug 814509 was going to fix this but looks like it won't.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Wireframe

I threw together a wireframe - is this the behaviour that we're looking for?

Comment 3

[Marco Bonardo [:mak]]

At first glance I thought to just make all of them the same height... But this is actually a visual design question for which I don't have a technical answer.

Comment 4

[Girish Sharma [:Optimizer]]

I would tend away from the idea of giving more preference to a completed download just because its start time was newer than a download which is still in progress and hide it under "other downloads" label.

The preference should be first given to the downloads in progress.

About the height of the last item, I raised this bug for the height to be consistent in both states so any height would do, while I myself prefer the smaller height.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

shorlander just sent me this wireframe:

http://cl.ly/image/1o0J1K2J3T0V

For the footers, the top-left state is when at least one item is downloading. The top-right state is when there are no downloads downloading, but at least one is paused. Bottom left is when we have at least 1 "hidden" item. Bottom right is when there are no hidden items in the list.

Transitioning from top-left or top-right to bottom-left while the panel is open (for example, if a download completes or is cancelled somehow) should use an animation to make the change in height not too jarring.

Comment 6

[Girish Sharma [:Optimizer]]

The mockups are great. There is more consistency between when downloads are happening ans when they are not. (as per this bug's summary, it is solved). But I still don't like the fact that an in-progress download is given lower priority (in terms of visibility) as compared to a completed one. For instance, my 2 gb linux download would be hidden by 1 completed kitten pic, and 2 on going songs.

Comment 7

[Marco Bonardo [:mak]]

so we have info, removing needinfo request.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Here's a little more info:

The "+X other current downloads" is confusing if there are no downloads in progress ("current" seems to suggest that some are actually in progress).

So the graphic I posted is slightly invalidated - the bottom-left case no longer exists. We simply switch to the "Show All Downloads" footer in that case.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

With this patch, we only display "+X other current downloads" if at least one download is downloading or paused. In all other cases, we just have "Show All Downloads".

On a lower level, I've moved the logic of hiding / showing the downloads summary out in favour of using CSS and an attribute instead. DownloadsSummary no longer has a visibility state - it has an active state. When active, if we're not showingProgress, we flip the DownloadsFooter to not show the summary.

We do the same thing if and when the DownloadsSummary becomes inactive.

I don't have a transition animation in this patch - I might want to do that in a follow-up bug, if we think it's really needed.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Fixing some nits I caught in self-review.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 688370 [details] [diff] [review]
Patch v2

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

::: browser/components/downloads/content/downloads.css
@@ +98,5 @@
>  
>  #downloadsSummary:not([inprogress="true"]) > vbox > #downloadsSummaryProgress,
> +#downloadsSummary:not([inprogress="true"]) > vbox > #downloadsSummaryDetails,
> +#downloadsFooter[showingsummary="true"] > #downloadsHistory,
> +#downloadsFooter:not([showingsummary="true"]) > #downloadsSummary

I think we can remove all of the ="true" since we don't set the attributes to false, we remove them (well, we should remove them, see below)

::: browser/components/downloads/content/downloads.js
@@ +1431,3 @@
>     *
> +   * @param aActive
> +   *        True if the summary should be active.

"Set to true to activate the summary."

@@ +1445,5 @@
>                       .removeView(this);
>      }
> +
> +    if (!aActive) {
> +      DownloadsFooter.showingSummary = false;

you can just move this into the previous else branch

@@ +1461,1 @@
>    },

nit: eventually you can write this as

get active() this._active,

@@ +1476,5 @@
>        this._summaryNode.removeAttribute("inprogress");
>      }
> +    // If progress isn't being shown, then we simply do not show the summary.
> +    DownloadsFooter.showingSummary = aShowingProgress;
> +    return aShowingProgress;

I think you can move the return to the previous line

@@ +1663,5 @@
> +   */
> +  set showingSummary(aValue)
> +  {
> +    if (this._footerNode) {
> +      this._footerNode.setAttribute("showingsummary", aValue);

don't set the attribute to false, remove it instead

@@ +1683,1 @@
>    }

I'm starting thinking we should replace all these many element getters with an ._element(id) getter that caches them in a WeakMap... just thinking :)

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3 (r+'d by mak)

(In reply to Marco Bonardo [:mak] from comment #11)
> Comment on attachment 688370 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 688370 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/downloads/content/downloads.css
> @@ +98,5 @@
> >  
> >  #downloadsSummary:not([inprogress="true"]) > vbox > #downloadsSummaryProgress,
> > +#downloadsSummary:not([inprogress="true"]) > vbox > #downloadsSummaryDetails,
> > +#downloadsFooter[showingsummary="true"] > #downloadsHistory,
> > +#downloadsFooter:not([showingsummary="true"]) > #downloadsSummary
> 
> I think we can remove all of the ="true" since we don't set the attributes
> to false, we remove them (well, we should remove them, see below)
> 

Done.

> ::: browser/components/downloads/content/downloads.js
> @@ +1431,3 @@
> >     *
> > +   * @param aActive
> > +   *        True if the summary should be active.
> 
> "Set to true to activate the summary."
> 

Fixed.

> @@ +1445,5 @@
> >                       .removeView(this);
> >      }
> > +
> > +    if (!aActive) {
> > +      DownloadsFooter.showingSummary = false;
> 
> you can just move this into the previous else branch
> 

Good catch - fixed.

> @@ +1461,1 @@
> >    },
> 
> nit: eventually you can write this as
> 
> get active() this._active,
> 

Eventually? Let's make it immediately. :) Fixed.

> @@ +1476,5 @@
> >        this._summaryNode.removeAttribute("inprogress");
> >      }
> > +    // If progress isn't being shown, then we simply do not show the summary.
> > +    DownloadsFooter.showingSummary = aShowingProgress;
> > +    return aShowingProgress;
> 
> I think you can move the return to the previous line
> 

Done.

> @@ +1663,5 @@
> > +   */
> > +  set showingSummary(aValue)
> > +  {
> > +    if (this._footerNode) {
> > +      this._footerNode.setAttribute("showingsummary", aValue);
> 
> don't set the attribute to false, remove it instead
> 

Done.

> @@ +1683,1 @@
> >    }
> 
> I'm starting thinking we should replace all these many element getters with
> an ._element(id) getter that caches them in a WeakMap... just thinking :)

Good idea. I've filed bug 818534.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/51cbdd0f1ba4

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/51cbdd0f1ba4

Comment 15

[Simona Badau, Desktop QA]

Verified that that there is no summary in the Downloads Panel if there are no hidden downloads in progress or paused.

Verified on Windows 7, Ubuntu 12.10 and Mac OS X 10.7.5:
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20121217 Firefox/20.0 Build ID: 20121217030850
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20121218 Firefox/20.0 Build ID: 20121218030803
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20121218 Firefox/20.0 Build ID: 20121218030803