Closed Bug 890039 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: shorlander, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M9][Australis:P4])

Attachments

(1 file)

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/)
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P4]
Duplicate of this bug: 898839
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.
Assignee: nobody → gijskruitbosch+bugs
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?
Attachment #795464 - Flags: review?(jaws)
Attachment #795464 - Flags: feedback?(mak77)
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.
Attachment #795464 - Flags: review?(jaws) → review+
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.
Attachment #795464 - Flags: feedback?(mak77) → feedback+
Blocks: 909783
(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)
Status: NEW → ASSIGNED
Whiteboard: [Australis:M?][Australis:P4] → [Australis:M9][Australis:P4][fixed-in-ux]
(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 :)
https://hg.mozilla.org/mozilla-central/rev/9ca205f212b7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P4][fixed-in-ux] → [Australis:M9][Australis:P4]
Target Milestone: --- → Firefox 28
Depends on: 1008402
You need to log in before you can comment on or make changes to this bug.