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.

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

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

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.

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.

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.

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.

so we have info, removing needinfo request.

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.

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.

Fixing some nits I caught in self-review.

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

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

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

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