Closed Bug 1304708 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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".
Attachment #8793752 - Flags: review?(jhofmann)
Assignee: nobody → dao+bmo
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.
Attachment #8793752 - Flags: review?(jhofmann) → review-
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.
Attached patch patch v2Splinter Review
Attachment #8794178 - Flags: review?(jhofmann)
Attachment #8793752 - Attachment is obsolete: true
Comment on attachment 8794178 [details] [diff] [review]
patch v2

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

Looks good now! :)
Attachment #8794178 - Flags: review?(jhofmann) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a481cba45293
Use icon-colors.inc.svg in notification-icons.svg. r=jhofmann
https://hg.mozilla.org/mozilla-central/rev/a481cba45293
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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
Attachment #8794178 - Flags: approval-mozilla-aurora?
Comment on attachment 8794178 [details] [diff] [review]
patch v2

Polish UI. Take it in 51 aurora.
Attachment #8794178 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1309604
Depends on: 1322922
Depends on: 1345716
You need to log in before you can comment on or make changes to this bug.