The empty download view looks weird with just a white background. There should be an indicator (the best would be icon+text) explaining that the list is empty ("No downloads in the list").

If someone can point me to some code, I'll be happy to provide a patch :-)

Nice to have, but if we can't do it in time, won't block the feature. The code is in /browser/components/downloads/content/allDownloadsViewOverlay.xul and its .js file

I've also noticed the fact the view doesn't allow contextual menu with right click. It feels a bit weird. Should I file a bug about this behaviour ?

(In reply to Guillaume C. [:ge3k0s] from comment #3) > I've also noticed the fact the view doesn't allow contextual menu with right > click. It feels a bit weird. Should I file a bug about this behaviour ? you mean when it's empty? in the empty space? or on download items?

(In reply to Marco Bonardo [:mak] (intermittently avail. until 3 Jan) from comment #4) > (In reply to Guillaume C. [:ge3k0s] from comment #3) > > I've also noticed the fact the view doesn't allow contextual menu with right > > click. It feels a bit weird. Should I file a bug about this behaviour ? > > you mean when it's empty? in the empty space? or on download items? When it's empty and in empty space.

Paolo offered to work on this, and I think it's a soft-blocker for the specific case where we open the view in a tab in a PB window, then about:downloads shows a totally empty content that is not really nice.

(In reply to Marco Bonardo [:mak] from comment #6) > Paolo offered to work on this, and I think it's a soft-blocker for the > specific case where we open the view in a tab in a PB window, then > about:downloads shows a totally empty content that is not really nice. Yes it clearly would be better, but IMO the in-content library work should become main priority after the panel work is complete to improve consistency.

Initial patch, without styling. I'm also waiting to write the code that actually determines whether the list is empty, since this may overlap with bug 825846.

Mano suggested that we do this only in the in-content overlay... and honestly looking at the complexity of this patch I'm starting thinking may make more sense. The fact is nobody ever complained for empty lists in the Library, likely cause it also has other controls, while the in-content ui is barely minimal and looks odd.

btw, if you want to land the string, rs=me

(In reply to Marco Bonardo [:mak] from comment #9) > Mano suggested that we do this only in the in-content overlay... and > honestly looking at the complexity of this patch I'm starting thinking may > make more sense. > The fact is nobody ever complained for empty lists in the Library, likely > cause it also has other controls, while the in-content ui is barely minimal > and looks odd. There is however a difference between an empty white pane and the others that have a bar on top to sort item by categories and a information pane at the bottom. The complete blank background looks a bit weird.

(In reply to Marco Bonardo [:mak] from comment #9) > Mano suggested that we do this only in the in-content overlay... and > honestly looking at the complexity of this patch I'm starting thinking may > make more sense. The complexity will be more or less the same, the only difference being not changing the overlay's entry point (that I don't think is a big deal), in exchange for needing a new callback from the view to the in-content page to let the page display the alternate empty view with the label. I can do either, let me know what you prefer. Marco, are you suggesting landing the unused string before the Aurora merge?

On behalf of Marco - yes, pleas lang the strings.

Comment on attachment 698421 [details] [diff] [review] String only We may want to improve the wording later, but it shouldn't affect localizers.

Personally, I think you should add this is as a feature to richlistbox (simiar to the emptytext feature we have for text fields). I would expect a much simpler patch.

(In reply to Mano from comment #16) > Personally, I think you should add this is as a feature to richlistbox > (simiar to the emptytext feature we have for text fields). I would expect a > much simpler patch. Good suggestion, will look into this. I'll just have to figure out how to handle richlistitems that are hidden and then unhidden.

A simple mutation observer should do the trick. I just realized that this string won't fit the Library anyway, because the list is also empty when the search term does not match any download. So either we add another string, "No results", or we don't apply this feature to the Library.

Or maybe we should make it "There are no downloads to show".

I think that a specific message like "Could not find any matching downloads." (similar to the one in the Add-ons Manager) may be clearer. In the end, in a future iteration, we will support search for the in-content view as well.

Comment on attachment 698439 [details] [diff] [review] No results string "No matching downloads could be found" would be better, I think. Anyway, we may want to revise the wording later here as well, but this should be good enough for localizers.

(In reply to Mano from comment #22) > Anyway, we may want to revise the wording later here as well, but this > should be good enough for localizers. Yeah, I kept the same wording as the Add-ons Manager, but we may change it later. https://hg.mozilla.org/integration/mozilla-inbound/rev/4727f845406a

https://hg.mozilla.org/mozilla-central/rev/ddedd11ef0e4 https://hg.mozilla.org/mozilla-central/rev/4727f845406a

Comment on attachment 698265 [details] [diff] [review] Without styling I like Mano's idea to handle the thing in the rlb (as an optional feature like greytext), though we wouldn't be able to re-use these strings in such case. Btw, since Mano started looking at the patch and approaches I think it's fine if he takes care of the review.

(In reply to Marco Bonardo [:mak] from comment #25) > though we wouldn't be able to re-use these strings in such disregard this, I was not thinking clearly at that time, we can re-use the string, so I definitely agree the emptytext (or whatever may be called) attribute in the RLB would be nice.

Should i review the current patch, or wait for the richlistbox alternative?

I'd wait news about the RLB alternative, since that approach looks much better.

It was a good idea to wait for the other performance patches and tests, because they actually affect the solution for showing the empty text. After investigating how to implement this as a richlistbox feature, it turns out that in our case we are better off making a specific patch, in particular to prevent regressing the performance gains we obtained. In fact, to implement this properly in the richlistbox (considering empty a view where all items have the "hidden" attribute), without re-scanning the entire list of children every time one of them is removed, the simple way is to make the richlistitem track its hidden state. But, we've actually detached the richlistitem binding to improve performance, so this wouldn't work. In theory we could work around using a MutationObserver in the richlistbox, but I personally don't like the idea of writing a platform feature in a less efficient way to solve a very particular case, that we can otherwise address with local changes. This initial patch works correctly, and I'm attaching it to get initial feedback, though it will be rebased on top of a couple of other patches that are being worked on to fix some errors when the view starts up.

Gently kicking this out of the blockers list. If we'll have a reviewed patch any time soon (let's say before January 16), and it looks safe regarding regressions, we'll ask approval on it the usual way. Though, we won't wait for this to call the feature "ready". Sad decision, but compared to the other blockers this is just a polish issue that is particularly bad in a very specific case: Show all download, without any download, while in private browsing mode. Clearly, whatever thing happens, this stays as a priority polish for Firefox 21.

Over IRC I suggested this hack to Paolo (it only applies to the in-content view, which is what we really care of here). 1) Because it's the in-content view, which has no search box, we can rely on the fact that there are no hidden items, i.e. we can use the :empty selector to detect the empty richlistbox case. 2) Along with the empty selector, we can use this selector: https://developer.mozilla.org/en-US/docs/CSS/Adjacent_sibling_selectors to "toggle" the empty label. Hacky as it is, this should result in a very, very small and safe patch. Later on we can come up with a better solution that would also work for the library, and maybe, just maybe, for future use cases of richlistbox. For Firefox 20, however, this should do the trick.

Simple version as per comment 31.

Comment on attachment 701598 [details] [diff] [review] Simple version for the in-content view only Review of attachment 701598 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall. I'm canceling the review request only due to my xul-structure concern (see below). I might be wrong about that though. ::: browser/components/downloads/content/contentAreaDownloadsView.css @@ +6,5 @@ > + display: none; > +} > + > +#downloadsRichListBox:empty + #downloadsListEmptyDescription { > + display: block; -moz-box ::: browser/components/downloads/content/contentAreaDownloadsView.xul @@ +35,5 @@ > #endif > </keyset> > > + <stack flex="1"> > + <richlistbox id="downloadsRichListBox"/> set flex="1" (for readability. I know the overlay provides this). @@ +37,5 @@ > > + <stack flex="1"> > + <richlistbox id="downloadsRichListBox"/> > + <description id="downloadsListEmptyDescription" > + value="&downloadsListEmpty.label;"/> Please test that this does not result in strange layout if the window is too small. I wonder if we should wrap the description element in a box (also with flex="1"), and style that box.

(In reply to Mano from comment #33) > > + <stack flex="1"> > > + <richlistbox id="downloadsRichListBox"/> > > + <description id="downloadsListEmptyDescription" > > + value="&downloadsListEmpty.label;"/> > > Please test that this does not result in strange layout if the window is too > small. It looks like there's nothing special to note in this case. > I wonder if we should wrap the description element in a box (also with > flex="1"), and style that box. I've no preference. Currently, the description element has the same dimensions of the stack element, similarly to what would happen to a box element. I fixed the other two nits, let me know if you want me to attach a new patch.

(In reply to Paolo Amadini [:paolo] from comment #34) > (In reply to Mano from comment #33) > > > + <stack flex="1"> > > > + <richlistbox id="downloadsRichListBox"/> > > > + <description id="downloadsListEmptyDescription" > > > + value="&downloadsListEmpty.label;"/> > > > > Please test that this does not result in strange layout if the window is too > > small. > > It looks like there's nothing special to note in this case. > OK. > > I wonder if we should wrap the description element in a box (also with > > flex="1"), and style that box. > > I've no preference. Currently, the description element has the same > dimensions > of the stack element, similarly to what would happen to a box element. Never mind then. It'd just make layout complex. So, attach the new patch and I'll mark it.

With "display: -moz-box".

Comment on attachment 701604 [details] [diff] [review] Simple version for the in-content view only I'm glag we could come up with a simple hack here. This is good to go, and safe enough for Aurora as well. Please file a follow up for a proper fix that would also work for the Library. I'm still in favor of the richlistbox approach.

Comment on attachment 701604 [details] [diff] [review] Simple version for the in-content view only http://hg.mozilla.org/integration/mozilla-inbound/rev/80c6a630b0ba

I filed Bug 830156.

Comment on attachment 701604 [details] [diff] [review] Simple version for the in-content view only [Approval Request Comment] Bug caused by (feature/regressing bug #): Downloads Panel feature (in content view for private browsing) User impact if declined: notable unpolished ui Testing completed (on m-c, etc.): tbd. Risk to taking this patch (and alternatives if risky): limited to the feature (in particular, limited to the in-content view. The library view isn't affected). String or UUID changes made by this patch: none

Comment on attachment 701604 [details] [diff] [review] Simple version for the in-content view only New feature, no string changes, approving for Aurora 20.

Verified as fixed on Firefox 20 beta 1 - "There are no downloads" is shown in about:downloads. Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.8: Build ID: 20130220104816 Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0 Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0