890039 - Opening the download panel with inverted Lightweight theme changes the icon to the dark variant

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Stephen Horlander [:shorlander]]

STR:

1) Apply lightweight theme with inverted icons
2) Open download panel

Should use inverted open state and return to the normal inverted state after closing.

Comment 1

[Justin Dolske [:Dolske]]

http://cl.ly/image/020I282w4731

(with https://addons.mozilla.org/en-US/firefox/addon/dark-fox-18066/)

Comment 3

[:Gijs (he/him)]

Attachment: Download indicator isn't styled correctly in dark LWTs,

This works on OS X, need to test it on Windows still. I've filed bug 909349 to take care of the pressed state (for which we don't really have a design/images right now.

Comment 4

[:Gijs (he/him)]

Comment on attachment 795464 [details] [diff] [review]
Download indicator isn't styled correctly in dark LWTs,

This works on Windows, too.

Note that I've also removed the mode="full" styling which, AFAIK, is no longer necessary, and that I've not touched linux as the icon situation there is still unresolved, as far as I know.

Marco, can you give this a casual look in addition to Jared's review, just to make sure I'm not doing anything wrong wrt the downloads stuff here?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 795464 [details] [diff] [review]
Download indicator isn't styled correctly in dark LWTs,

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

::: browser/themes/osx/downloads/indicator.css
@@ +22,5 @@
>                                0, 198, 18, 180) center no-repeat;
>  }
>  
> +#downloads-indicator-icon:-moz-lwtheme-brighttext {
> +  background-image: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"), 0, 198, 18, 180);

It looks like this rule can be placed lower with the other #download-indicator-counter rule since they both have the same background-image property name+value pair.

@@ +41,5 @@
>    background-size: 12px;
>  }
>  
> +#downloads-indicator:not([counter])
> +#downloads-indicator-counter:-moz-lwtheme-brighttext {

Placing this separator on two lines is confusing, at first I thought it was missing a comma! It appears that this is done elsewhere in the file though. Can you file a bug to place all of these selectors on the same line?

::: browser/themes/shared/toolbarbuttons.inc.css
@@ -40,5 @@
>  #history-panelmenu[customizableui-areatype="toolbar"] {
>    -moz-image-region: rect(0, 180px, 18px, 162px);
>  }
>  
> -#downloads-indicator[customizableui-areatype="toolbar"],

Why does this line need to be deleted?

Comment 6

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #5)
> Comment on attachment 795464 [details] [diff] [review]
> Download indicator isn't styled correctly in dark LWTs,
> 
> Review of attachment 795464 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/osx/downloads/indicator.css
> @@ +22,5 @@
> >                                0, 198, 18, 180) center no-repeat;
> >  }
> >  
> > +#downloads-indicator-icon:-moz-lwtheme-brighttext {
> > +  background-image: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"), 0, 198, 18, 180);
> 
> It looks like this rule can be placed lower with the other
> #download-indicator-counter rule since they both have the same
> background-image property name+value pair.

I kept the rules with their corresponding non-lwtheme rules. I could also put them all together, but I thought this way around made more sense.

> 
> @@ +41,5 @@
> >    background-size: 12px;
> >  }
> >  
> > +#downloads-indicator:not([counter])
> > +#downloads-indicator-counter:-moz-lwtheme-brighttext {
> 
> Placing this separator on two lines is confusing, at first I thought it was
> missing a comma! It appears that this is done elsewhere in the file though.
> Can you file a bug to place all of these selectors on the same line?

I can; I was also confused! But yeah, I followed the file's style...

 
> ::: browser/themes/shared/toolbarbuttons.inc.css
> @@ -40,5 @@
> >  #history-panelmenu[customizableui-areatype="toolbar"] {
> >    -moz-image-region: rect(0, 180px, 18px, 162px);
> >  }
> >  
> > -#downloads-indicator[customizableui-areatype="toolbar"],
> 
> Why does this line need to be deleted?

AFAIK it's useless; there's no toolbarbutton-icon that will have the icon applied, so I don't see why we should keep it, essentially.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 795464 [details] [diff] [review]
Download indicator isn't styled correctly in dark LWTs,

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

it looks ok

::: browser/themes/osx/downloads/indicator.css
@@ +41,5 @@
>    background-size: 12px;
>  }
>  
> +#downloads-indicator:not([counter])
> +#downloads-indicator-counter:-moz-lwtheme-brighttext {

I confirm this indentation style is very confusing, it was surely unnoticed in the review or it wouldn't have landed.

Comment 8

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #7)
> Comment on attachment 795464 [details] [diff] [review]
> Download indicator isn't styled correctly in dark LWTs,
> 
> Review of attachment 795464 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> it looks ok
> 
> ::: browser/themes/osx/downloads/indicator.css
> @@ +41,5 @@
> >    background-size: 12px;
> >  }
> >  
> > +#downloads-indicator:not([counter])
> > +#downloads-indicator-counter:-moz-lwtheme-brighttext {
> 
> I confirm this indentation style is very confusing, it was surely unnoticed
> in the review or it wouldn't have landed.

Cool, filed bug 909783.

Meanwhile, landed this as: https://hg.mozilla.org/projects/ux/rev/9ca205f212b7


(In reply to Jared Wein [:jaws] from comment #5)
> > +#downloads-indicator-icon:-moz-lwtheme-brighttext {
> > +  background-image: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"), 0, 198, 18, 180);
> 
> It looks like this rule can be placed lower with the other
> #download-indicator-counter rule since they both have the same
> background-image property name+value pair.

(Jared, I assumed your r+ without further comments meant the current organization of those rules is OK per my reasoning in comment 6... let me know if I misunderstood)

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Jared Wein [:jaws] from comment #5)
> > > +#downloads-indicator-icon:-moz-lwtheme-brighttext {
> > > +  background-image: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"), 0, 198, 18, 180);
> > 
> > It looks like this rule can be placed lower with the other
> > #download-indicator-counter rule since they both have the same
> > background-image property name+value pair.
> 
> (Jared, I assumed your r+ without further comments meant the current
> organization of those rules is OK per my reasoning in comment 6... let me
> know if I misunderstood)

Yes, the explanation is fine. My issue with it is not huge :)

Comment 10

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/9ca205f212b7