winstripe theme-ing for new Downloads View in Places

RESOLVED FIXED in Firefox 20

Status

()

defect
P1
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

Trunk
Firefox 20
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 9 obsolete attachments)

+++ This bug was initially created as a clone of Bug #822257 +++

The new Downloads View in places landed in bug 675902 with pinstripe theme-ing only. We need to complete the theme for winstripe as well.
The current on-hover effect feels inconsistent with the one in the panel (in fact it's a general library problem). The new blue semi transparent effect should be generalized to the whole UI.
I even think than a mockup from the UX team could be useful.
I'm assigning this to myself just in case no one else jumps in.
Assignee: nobody → mano
Keywords: helpwanted
I also think that more commands should be avalaible as buttons in this view (pause/resume for example):
(In reply to Guillaume C. [:ge3k0s] from comment #4)
> I also think that more commands should be avalaible as buttons in this view
> (pause/resume for example):

while I mostly agree with you, this request should be filed apart.
(In reply to Marco Bonardo [:mak] from comment #5)
> (In reply to Guillaume C. [:ge3k0s] from comment #4)
> > I also think that more commands should be avalaible as buttons in this view
> > (pause/resume for example):
> 
> while I mostly agree with you, this request should be filed apart.

As I've written in my previous comment UX feedback (via mockups) seems to be inevitable here.
(In reply to Guillaume C. [:ge3k0s] from comment #6)
> As I've written in my previous comment UX feedback (via mockups) seems to be
> inevitable here.

Sure, just that this is not the proper bug. This bug is about theming what is already there, not about adding more stuff. So please, file a separate bug about showing more buttons.
(In reply to Mano from comment #3)
> I'm assigning this to myself just in case no one else jumps in.

I'm jumping in. :)
Assignee: mano → mconley
Filed bug 822763, however I'm not sure about what we really want.
Posted patch WIP 1 (obsolete) — Splinter Review
Posted image Screenshot of WIP patch on Windows 7 (obsolete) —
Still a few issues here - primarily, the icons on the right are practically invisible when the download item is selected.
(In reply to Mike Conley (:mconley) from comment #11)
> Created attachment 693546 [details]
> Screenshot of WIP patch on Windows 7
> 
> Still a few issues here - primarily, the icons on the right are practically
> invisible when the download item is selected.

As I've written above I think the on-hover effect should be changed. Main issue this is a whole UI change and not just a download view one.
Posted patch Patch v1 (obsolete) — Splinter Review
Ok, I think I've got something workable here.
Attachment #693544 - Attachment is obsolete: true
Attachment #693546 - Attachment is obsolete: true
Posted image Patch v1 on Windows 7 (obsolete) —
Attachment #693604 - Flags: review?(mak77)
Comment on attachment 693604 [details] [diff] [review]
Patch v1

>+#downloadsRichListBox > richlistitem.download[selected] {
>+  background-color: rgba(148, 172, 204, 0.39);

Random hardcoded color alert. This surely isn't going to fit with various OS themes.

>+  color: inherit;
>+  outline: 1px transparent solid;
>+  outline-offset: -1px;

Why specify a width, style and offset if you don't want any visible outline?

>+richlistitem.download {
>+  height: 6em;
>+  margin: 0;

richlistitems don't have a margin by default, do they?

>+  padding: 5px 8px 5px 8px;

padding: 5px 8px;

>+  border-bottom: 1px solid hsla(220,18%,51%,.25);

ThreeDShadow?
Attachment #693604 - Flags: review-
Priority: -- → P1
(In reply to Dão Gottwald [:dao] from comment #15)
> Comment on attachment 693604 [details] [diff] [review]
> Patch v1
> 
> >+#downloadsRichListBox > richlistitem.download[selected] {
> >+  background-color: rgba(148, 172, 204, 0.39);
> 
> Random hardcoded color alert. This surely isn't going to fit with various OS
> themes.

I think Mike was matching style of the add-ons manager (http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/mozapps/extensions/extensions.css#639), cause there's plan to move the Library to a tab asap and the downloads view will indeed move to a tab earlier, in bug 815352, to support per-window-pb.
We may probably just have the same styling as the awesomebar richlistbox, and just style the tab version according to the add-ons manager.
Empty download view should also have icon+text indicating that the list is empty.
Posted patch Patch v2 (obsolete) — Splinter Review
Cool - thanks for the feedback dao.

Alright, I've taken some inspiration from the Awesomebar, and I'm using similar styling there for winstripe aero. I've relaxed my rules somewhat so as to use more OS-specific default colours.

Screenshot is coming.
Attachment #693604 - Attachment is obsolete: true
Attachment #693605 - Attachment is obsolete: true
Attachment #693604 - Flags: review?(mak77)
Comment on attachment 693910 [details] [diff] [review]
Patch v2

I'll post some screenshots from Windows XP as well.
Attachment #693910 - Flags: review?(mak77)
Note that the icons are rather hard to see on hover. We'll likely need specialized icons for the selection.
Blocks: 823095
Keywords: helpwanted
bug 815352 part 1 landed and, provided it sticks to the tree, moves some of the style files, so this likely needs an unbitrot.
Download view background should be blue-ish like the one in the other subcategories (History, Bookmarks, etc.)
Posted patch Patch v2.1 (obsolete) — Splinter Review
De-bitrotted.
Attachment #693910 - Attachment is obsolete: true
Attachment #693910 - Flags: review?(mak77)
(In reply to Guillaume C. [:ge3k0s] from comment #23)
> Download view background should be blue-ish like the one in the other
> subcategories (History, Bookmarks, etc.)

Hey ge3k0s,

So, we're thinking about going with the more native look for richlist selection for downloads. The blue background that's seen in the rest of Places is really not native for Aero.

Cheers,

-Mike
The background of the Library is light blue cause the Library is styled (by design) as a Media Collection window, that is, for example, Windows Media Player.
This view will be both in the Library and in a tab, so I'm not sure if we want to differentiate the background based on that rule, considered also the Library will move to a tab. This is a minor details btw.
For the raw details, bug 403147.
Attachment #694430 - Flags: review?(mak77)
(In reply to Mike Conley (:mconley) from comment #25)
> (In reply to Guillaume C. [:ge3k0s] from comment #23)
> > Download view background should be blue-ish like the one in the other
> > subcategories (History, Bookmarks, etc.)
> 
> Hey ge3k0s,
> 
> So, we're thinking about going with the more native look for richlist
> selection for downloads. The blue background that's seen in the rest of
> Places is really not native for Aero.
> 
> Cheers,
> 
> -Mike

Ok I understand. Thanks for the info ;-)
Comment on attachment 694430 [details] [diff] [review]
Patch v2.1

Nope, this is no good - it looks like allDownloadsViewOverlay.css was essentially copied over from downloads.css; there's a bunch of rules in there that we don't need.

I'll need to remove those.
Attachment #694430 - Flags: review?(mak77)
Posted patch Patch v2.2 (obsolete) — Splinter Review
Removes cruft from allDownloadsViewOverlay.css.
Attachment #694430 - Attachment is obsolete: true
Attachment #694511 - Flags: review?(mak77)
Comment on attachment 694511 [details] [diff] [review]
Patch v2.2

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

::: browser/themes/winstripe/downloads/allDownloadsViewOverlay-aero.css
@@ +6,5 @@
> +
> +@media (-moz-windows-default-theme) {
> +  #downloadsRichListBox > richlistitem.download {
> +    padding: 0;
> +  }

I think you should use the WINSTRIPE_AERO define trick and in allDownloadsViewOverlay.css define a padding only %ifndef WINSTRIPE_AERO
(http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/downloads/downloads-aero.css#5)
ps: remember to add the preprocessing in jar.mn

@@ +9,5 @@
> +    padding: 0;
> +  }
> +
> +  #downloadsRichListBox > richlistitem.download > .downloadTypeIcon {
> +    -moz-margin-start: 8px;

and the same for this, since these 2 rules are related.

These 2 should probably not be under -default-theme so should just work

@@ +39,5 @@
> +  }
> +
> +  #downloadsRichListBox > richlistitem.download:last-child {
> +    border-bottom: initial;
> +  }

a comment on why these are needed would be awesome...

::: browser/themes/winstripe/downloads/allDownloadsViewOverlay.css
@@ +11,5 @@
>    border-top: 1px solid transparent;
>  }
>  
> +#downloadsRichListBox > richlistitem.download:last-child {
> +  border-bottom: 1px solid transparent;

may we just define border: none in winstripe since we decided to not have any border?

I couldn't find border rules in rlb, so think here we are overriding the listbox
listitem {
  border: 1px solid transparent;
}

in the end doesn't looks like this rule is doing anything... but let me know I may have missed something

@@ +16,3 @@
>  }
>  
> +richlistitem.download {

So, here you have
#downloadsRichListBox > richlistitem.download:first-child
#downloadsRichListBox > richlistitem.download:last-child
richlistitem.download
richlistitem.download:first-child
richlistitem.download:last-child

looks like there is some redundancy... you just need
#downloadsRichListBox > richlistitem.download
#downloadsRichListBox > richlistitem.download:first-child
#downloadsRichListBox > richlistitem.download:last-child

and maybe not even the last 2 if we don't want a border

@@ +17,5 @@
>  
> +richlistitem.download {
> +  height: 6em;
> +  padding: 5px 8px;
> +  -moz-padding-end: 0;

what is this padding override for? I was not expecting the item to have padding at end...
Attachment #694511 - Flags: review?(mak77)
Posted patch Patch v2.3 (obsolete) — Splinter Review
Thanks for the audit, Marco. I find it very easy to get lost in the complexities of CSS rules, so I really appreciate the second pair of eyes!

(In reply to Marco Bonardo [:mak] (intermittently avail. until 3 Jan) from comment #31)
> Comment on attachment 694511 [details] [diff] [review]
> Patch v2.2
> 
> Review of attachment 694511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/winstripe/downloads/allDownloadsViewOverlay-aero.css
> @@ +6,5 @@
> > +
> > +@media (-moz-windows-default-theme) {
> > +  #downloadsRichListBox > richlistitem.download {
> > +    padding: 0;
> > +  }
> 
> I think you should use the WINSTRIPE_AERO define trick and in
> allDownloadsViewOverlay.css define a padding only %ifndef WINSTRIPE_AERO
> (http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/
> downloads/downloads-aero.css#5)
> ps: remember to add the preprocessing in jar.mn
> 

Done! And good idea. I think I much prefer this to overriding.

> @@ +9,5 @@
> > +    padding: 0;
> > +  }
> > +
> > +  #downloadsRichListBox > richlistitem.download > .downloadTypeIcon {
> > +    -moz-margin-start: 8px;
> 
> and the same for this, since these 2 rules are related.
> 

Done.

> @@ +39,5 @@
> > +  }
> > +
> > +  #downloadsRichListBox > richlistitem.download:last-child {
> > +    border-bottom: initial;
> > +  }
> 
> a comment on why these are needed would be awesome...

Actually, they don't appear to be needed. Thanks for noticing that - I've removed them.

> 
> ::: browser/themes/winstripe/downloads/allDownloadsViewOverlay.css
> @@ +11,5 @@
> >    border-top: 1px solid transparent;
> >  }
> >  
> > +#downloadsRichListBox > richlistitem.download:last-child {
> > +  border-bottom: 1px solid transparent;
> 
> may we just define border: none in winstripe since we decided to not have
> any border?
> 
> I couldn't find border rules in rlb, so think here we are overriding the
> listbox
> listitem {
>   border: 1px solid transparent;
> }
> 
> in the end doesn't looks like this rule is doing anything... but let me know
> I may have missed something
> 

You didn't miss a thing - these are, I think, just more leftovers of stuff brought over from the original panel stylings. They're not adding anything useful, so I've removed them.

> @@ +16,3 @@
> >  }
> >  
> > +richlistitem.download {
> 
> So, here you have
> #downloadsRichListBox > richlistitem.download:first-child
> #downloadsRichListBox > richlistitem.download:last-child
> richlistitem.download
> richlistitem.download:first-child
> richlistitem.download:last-child
> 
> looks like there is some redundancy... you just need
> #downloadsRichListBox > richlistitem.download
> #downloadsRichListBox > richlistitem.download:first-child
> #downloadsRichListBox > richlistitem.download:last-child
> 
> and maybe not even the last 2 if we don't want a border
> 

Done!

> @@ +17,5 @@
> >  
> > +richlistitem.download {
> > +  height: 6em;
> > +  padding: 5px 8px;
> > +  -moz-padding-end: 0;
> 
> what is this padding override for? I was not expecting the item to have
> padding at end...

Another leftover. Removed.
Attachment #694511 - Attachment is obsolete: true
Attachment #694843 - Flags: review?(mak77)
Comment on attachment 694843 [details] [diff] [review]
Patch v2.3

This has been bitrotted by a patch in bug 822343. Regenerating...
Attachment #694843 - Flags: review?(mak77)
Posted patch Patch v2.4 (obsolete) — Splinter Review
De-bitrotted.
Attachment #694843 - Attachment is obsolete: true
Attachment #694853 - Flags: review?(mak77)
Comment on attachment 694853 [details] [diff] [review]
Patch v2.4

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

provided we may still have polish to do later (even in Aurora), this looks sane enough to me.

::: browser/themes/winstripe/downloads/allDownloadsViewOverlay-aero.css
@@ +8,5 @@
> +
> +@media (-moz-windows-default-theme) {
> +  /*
> +  -moz-appearance: menuitem is almost right, but the hover effect is not
> +  transparent and is lighter than desired.

add comment stating this is taken from the autocomplete richlistbox styling (state the exact css file) and should be kept in sync if that one changes.
Attachment #694853 - Flags: review?(mak77) → review+
Thanks Marco! Added the comment.
Attachment #694853 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5836113e3e87
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Comment on attachment 694928 [details] [diff] [review]
Patch v2.5 (r+'d by mak)

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

::: browser/themes/winstripe/downloads/allDownloadsViewOverlay-aero.css
@@ +24,5 @@
> +      -moz-linear-gradient(to bottom, rgba(255,255,255,0.9) 3px, rgba(255,255,255,0) 3px),
> +      -moz-linear-gradient(to right, rgba(255,255,255,0.5) 3px, rgba(255,255,255,0) 3px),
> +      -moz-linear-gradient(to left, rgba(255,255,255,0.5) 3px, rgba(255,255,255,0) 3px),
> +      -moz-linear-gradient(to top, rgba(255,255,255,0.4) 3px, rgba(255,255,255,0) 3px),
> +      -moz-linear-gradient(to bottom, rgba(163,196,247,0.3), rgba(122,180,246,0.3));

We support unprefixed gradients already, btw.
likely the original gradient had not been updated yet and I didn't double check this copy/paste. We should indeed use unprefixed gradients now.
You need to log in before you can comment on or make changes to this bug.