Closed
Bug 1304708
Opened 9 years ago
Closed 9 years ago
Use icon-colors.inc.svg in notification-icons.svg
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
RESOLVED
FIXED
Firefox 52
People
(Reporter: dao, Assigned: dao)
References
Details
Attachments
(1 file, 1 obsolete file)
12.72 KB,
patch
|
johannh
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | 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 | ||
Updated•9 years ago
|
Assignee: nobody → dao+bmo
Comment 1•9 years ago
|
||
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-
Assignee | ||
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8794178 -
Flags: review?(jhofmann)
Assignee | ||
Updated•9 years ago
|
Attachment #8793752 -
Attachment is obsolete: true
Comment 4•9 years ago
|
||
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
Comment 6•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Assignee | ||
Comment 7•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox51:
--- → affected
Comment 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•