1304708 - Use icon-colors.inc.svg in notification-icons.svg

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

We need to use icon-colors.inc.svg in notification-icons.svg so that notification-icons.svg works better on Linux and supports high-contrast mode on Windows.

We still need to use the fill filter for lightweight themes, most notably the dark dev edition theme. Otherwise we'd have to duplicate all the rules setting list-style-image to use "white" and "black" icon variants instead of "fieldtext".

Comment 1

[Johann Hofmann [:johannh]]

Comment on attachment 8793752 [details] [diff] [review]
patch

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

Apart from the opacity these changes look good to me.

::: browser/themes/shared/controlcenter/panel.inc.css
@@ -378,5 @@
>  }
>  
>  .identity-popup-permission-icon.in-use {
>    fill: rgb(224, 41, 29);
> -  opacity: 1;

Isn't this undoing the change from Bug 1303339? When I try it out the problem seems to be there again.

::: browser/themes/shared/notification-icons.inc.css
@@ -244,2 @@
>    fill: #d92215;
> -  opacity: 1;

Same here, not sure why you're removing this but it makes the blocked plugin icon half transparent for me.

Comment 2

[Dão Gottwald [:dao]]

opacity: 1 was needed to override opacity: .4 that we set for .plugin-icon and friends. I removed opacity: .4 so the override isn't useful anymore.

I think the reduced opacity that you see comes from within notification-icons.svg.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8794178 [details] [diff] [review]
patch v2

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

Looks good now! :)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a481cba45293
Use icon-colors.inc.svg in notification-icons.svg. r=jhofmann

Comment 6

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/a481cba45293

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8794178 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/regressing bug #]: followup from bug 1304341. Not strictly required but makes icons in the identity block more consistent.
[User impact if declined]: notification icons use a different opacity than other icons in the identity block on Linux and Windows high-contrast mode.
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: reasonably straightforward fix, makes the code related to these icons overall more consistent, low risk
[String/UUID change made/needed]: none

Comment 8

[Gerry Chang [:gchang]]

Comment on attachment 8794178 [details] [diff] [review]
patch v2

Polish UI. Take it in 51 aurora.

Comment 9

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/35ea6ab123bb2391ac829de810c2f6b30ec6a9ac