822258 - winstripe theme-ing for new Downloads View in Places

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

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

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

Comment 1

[u428464]

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.

Comment 2

[u428464]

I even think than a mockup from the UX team could be useful.

Comment 3

[Asaf Romano (gone)]

I'm assigning this to myself just in case no one else jumps in.

Comment 4

[u428464]

I also think that more commands should be avalaible as buttons in this view (pause/resume for example):

Comment 5

[Marco Bonardo [:mak]]

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

Comment 6

[u428464]

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

Comment 7

[Marco Bonardo [:mak]]

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

Comment 8

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

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

Comment 9

[u428464]

Filed bug 822763, however I'm not sure about what we really want.

Comment 10

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

Attachment: WIP 1

Comment 11

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

Attachment: 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.

Comment 12

[u428464]

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

Comment 13

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

Attachment: Patch v1

Ok, I think I've got something workable here.

Comment 14

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

Attachment: Patch v1 on Windows 7

Comment 15

[Dão Gottwald [:dao]]

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?

Comment 16

[Marco Bonardo [:mak]]

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

Comment 17

[u428464]

Empty download view should also have icon+text indicating that the list is empty.

Comment 18

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

Attachment: Patch v2

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.

Comment 19

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

Attachment: Patch v2 on Windows 7 - Aero

Comment 20

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

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

I'll post some screenshots from Windows XP as well.

Comment 21

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

Attachment: Patch v2 on Windows XP - Luna Blue

Note that the icons are rather hard to see on hover. We'll likely need specialized icons for the selection.

Comment 22

[Marco Bonardo [:mak]]

bug 815352 part 1 landed and, provided it sticks to the tree, moves some of the style files, so this likely needs an unbitrot.

Comment 23

[u428464]

Download view background should be blue-ish like the one in the other subcategories (History, Bookmarks, etc.)

Comment 24

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

Attachment: Patch v2.1

De-bitrotted.

Comment 25

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

(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

Comment 26

[Marco Bonardo [:mak]]

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.

Comment 27

[Marco Bonardo [:mak]]

For the raw details, bug 403147.

Comment 28

[u428464]

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

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

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.

Comment 30

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

Attachment: Patch v2.2

Removes cruft from allDownloadsViewOverlay.css.

Comment 31

[Marco Bonardo [:mak]]

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

Comment 32

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

Attachment: Patch v2.3

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.

Comment 33

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

Comment on attachment 694843 [details] [diff] [review]
Patch v2.3

This has been bitrotted by a patch in bug 822343. Regenerating...

Comment 34

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

Attachment: Patch v2.4

De-bitrotted.

Comment 35

[Marco Bonardo [:mak]]

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.

Comment 36

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

Attachment: Patch v2.5 (r+'d by mak)

Thanks Marco! Added the comment.

Comment 37

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

Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/5836113e3e87

Comment 38

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/5836113e3e87

Comment 39

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 40

[Marco Bonardo [:mak]]

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.