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.

http://cl.ly/image/020I282w4731 (with https://addons.mozilla.org/en-US/firefox/addon/dark-fox-18066/)

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

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

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

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