Closed Bug 1454231 Opened 6 years ago Closed 6 years ago

Addon banner needs to be lighter on dark theme

Categories

(Firefox :: Theme, defect, P5)

Product:
Firefox
Component:
Theme
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: ntim, Assigned: bugzilla)

References

Details

(Keywords: good-first-bug)

Attachments

(4 files, 1 obsolete file)

Screenshot
137.28 KB, image/png
Details
Sync banner
141.33 KB, image/png
Details
Screen Shot 2018-04-16 at 12.27.25.png
109.96 KB, image/png
Details
Bug 1454231 - Improved dark theme colors of popup warnings.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Attached image Screenshot — 
The current banner looks roughly like this on the dark theme (emulated the banner using the browser toolbox). It would be nice if it were lighter similar to bug 1452674.
Amy, would it be possible to get a specification for the colors ?
Flags: needinfo?(amlee)
Attached image Sync banner — 
Looks like there's also a FxA banner that uses a similar color.
Priority: -- → P5
I've attached a spec to update the colours for attention notifications.
Flags: needinfo?(amlee)
Blocks: 1408121
No longer blocks: dark-theme-darkening
Thanks Amy!


This bug can be solved by adding new rules for `:root[lwt-popup-text]` in:

https://searchfox.org/mozilla-central/source/browser/themes/shared/customizableui/panelUI.inc.css#454,594-595

using the colors in attachment 8968233 [details]
Keywords: good-first-bug
Assignee: nobody → htwyford
Status: NEW → ASSIGNED
Comment on attachment 8975884 [details]
Bug 1454231 - Improved dark theme colors of popup warnings.

https://reviewboard.mozilla.org/r/244080/#review251558

Thanks for the patch and sorry for the delay in getting you some feedback. I've got some questions below about your patch.

::: browser/themes/shared/customizableui/panelUI.inc.css:112
(Diff revision 1)
> -  background: #FFBF00 url(chrome://browser/skin/update-badge-failed.svg) no-repeat center;
> +  background: #FFBF00 no-repeat center;
> +  mask-image: url(chrome://browser/skin/warning-clear.svg);
> +}
> +
> +:root[lwt-popup-brighttext] #PanelUI-menu-button[badge-status="addon-alert"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
> +  background: #FFE900 no-repeat center;

This only needs to set background-color.

::: browser/themes/shared/customizableui/panelUI.inc.css:466
(Diff revision 1)
>    height: 16px;
>  }
>  
>  .addon-banner-item::after {
> -  background: #FFBF00 url(chrome://browser/skin/update-badge-failed.svg) no-repeat center;
> +  background: #FFBF00 no-repeat center;
> +  mask-image: url(chrome://browser/skin/warning-clear.svg);

Why did you need to introduce a new SVG image?

If you just want to remove the color, you can update update-badge-failed.svg to have fill="context-fill" instead of fill="#fff".

And then at the places in the CSS where it is referenced you would set these two properties:
-moz-context-properties: fill;
fill: #fff (or other color of your choice)

::: browser/themes/shared/customizableui/panelUI.inc.css:470
(Diff revision 1)
> -  background: #FFBF00 url(chrome://browser/skin/update-badge-failed.svg) no-repeat center;
> +  background: #FFBF00 no-repeat center;
> +  mask-image: url(chrome://browser/skin/warning-clear.svg);
> +}
> +
> +:root[lwt-popup-brighttext] .addon-banner-item::after {
> +  background: #FFE900 no-repeat center;

This only needs to set background-color.

::: browser/themes/shared/customizableui/panelUI.inc.css:618
(Diff revision 1)
>  }
>  
> +:root[lwt-popup-brighttext] .addon-banner-item {
> +  color: @appmenuWarningColorDark@;
> +  background: @appmenuWarningBackgroundColorDark@;
> +  border: 0 !important;

Why does border:0 need to be set? And why does this need to be !important?
Attachment #8975884 - Flags: review?(jaws) → review-
Comment on attachment 8975884 [details]
Bug 1454231 - Improved dark theme colors of popup warnings.

https://reviewboard.mozilla.org/r/244080/#review251558

> Why does border:0 need to be set? And why does this need to be !important?

:amylee asked for there to be no borders on dark themed warnings. !important is set to override the preexisting border-top !important on line 440.
Comment on attachment 8975884 [details]
Bug 1454231 - Improved dark theme colors of popup warnings.

https://reviewboard.mozilla.org/r/244080/#review251856

::: browser/themes/shared/customizableui/panelUI.inc.css:108
(Diff revision 2)
>  }
>  
>  #PanelUI-menu-button[badge-status="addon-alert"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    height: 13px;
> -  background: #FFBF00 url(chrome://browser/skin/update-badge-failed.svg) no-repeat center;
> +  background: #FFBF00 no-repeat center;
> +  mask-image: url(chrome://browser/skin/warning-clear.svg);

Please don't use mask-image. You can leave the background as it was and use context-fill (-moz-context-properties: fill; fill: ...) to change the icon color
Attachment #8975884 - Flags: review-
Comment on attachment 8975884 [details]
Bug 1454231 - Improved dark theme colors of popup warnings.

https://reviewboard.mozilla.org/r/244080/#review251952

::: browser/themes/shared/customizableui/panelUI.inc.css:108
(Diff revision 2)
>  }
>  
>  #PanelUI-menu-button[badge-status="addon-alert"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    height: 13px;
> -  background: #FFBF00 url(chrome://browser/skin/update-badge-failed.svg) no-repeat center;
> +  background: #FFBF00 no-repeat center;
> +  mask-image: url(chrome://browser/skin/warning-clear.svg);

I was experimenting with setting it like this: 
```
.addon-banner-item::after {
  background: url(chrome://browser/skin/warning-white.svg) no-repeat center;
  -moz-context-properties: fill;
  fill: #FFBF00;
}

:root[lwt-popup-brighttext] .addon-banner-item::after {
  fill: #FFE900;
}
```
but the icon just appears black no matter what. What am I doing wrong? The new design spec calls for just a coloured icon, rather than a white icon against a colored background, so setting background-color doesn't solve the issue.
Comment on attachment 8975884 [details]
Bug 1454231 - Improved dark theme colors of popup warnings.

https://reviewboard.mozilla.org/r/244080/#review252234

You can use `hg histedit` to roll up your commits.
Attachment #8975884 - Flags: review?(jaws) → review-
Comment on attachment 8979568 [details]
Bug 1454231 - Improved dark theme colors of popup warnings.

https://reviewboard.mozilla.org/r/245700/#review252236

::: browser/base/content/browser-addons.js:508
(Diff revision 1)
>  
>      while (container.firstChild) {
>        container.firstChild.remove();
>      }
>  
> +    this._createAddonButton("test", null, null);

This should be deleted.

::: browser/themes/shared/customizableui/panelUI.inc.css:108
(Diff revision 1)
>  }
>  
>  #PanelUI-menu-button[badge-status="addon-alert"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    height: 13px;
>    background: #FFBF00 no-repeat center;
> -  mask-image: url(chrome://browser/skin/warning-clear.svg);
> +  mask-image: url(chrome://browser/skin/warning-white.svg);

Can you please go back to using the previous image and not introduce another one?

::: browser/themes/shared/customizableui/panelUI.inc.css:470
(Diff revision 1)
>  
>  .addon-banner-item::after {
>    background: #FFBF00 no-repeat center;
> -  mask-image: url(chrome://browser/skin/warning-clear.svg);
> +  mask-image: url(chrome://browser/skin/warning-white.svg);
> +  -moz-context-properties: fill;
> +  fill: #fff 

semi-colon needed at the end and no trailing white space.

::: browser/themes/shared/notification-icons.inc.css:158
(Diff revision 1)
>  }
>  
>  #webRTC-previewWarning {
>    background: rgba(255, 217, 99, .8) url("chrome://browser/skin/warning-white.svg") no-repeat .75em .75em;
> +  -moz-context-properties: fill;
> +  fill: #fff

semi-colon needed at the end.

::: browser/themes/shared/warning-white.svg:5
(Diff revision 1)
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16">
> -  <path fill="#fff" stroke="#000" stroke-opacity="0.3" d="M15.4,12.9 9.46,1.41 C9.12,0.756 8.59,0.381 8,0.381 7.41,0.381 6.88,0.756 6.54,1.41 L0.642,12.9 c-0.331,0.6 -0.348,1.3 -0.05,1.9 0.299,0.5 0.854,0.8 1.534,0.8 H13.9 c0.6,0 1.2,-0.3 1.5,-0.8 0.3,-0.6 0.3,-1.3 0,-1.9z M8.83,5.07 8.65,10.5 H7.34 L7.15,5.07 H8.83z M8,13.7 c-0.55,0 -0.99,-0.5 -0.99,-1 0,-0.6 0.44,-1 0.99,-1 0.56,0 0.99,0.4 0.99,1 0,0.5 -0.43,1 -0.99,1z"/>
> +  <path fill="#context-fill" stroke-opacity="0.3" d="M15.4,12.9 9.46,1.41 C9.12,0.756 8.59,0.381 8,0.381 7.41,0.381 6.88,0.756 6.54,1.41 L0.642,12.9 c-0.331,0.6 -0.348,1.3 -0.05,1.9 0.299,0.5 0.854,0.8 1.534,0.8 H13.9 c0.6,0 1.2,-0.3 1.5,-0.8 0.3,-0.6 0.3,-1.3 0,-1.9z M8.83,5.07 8.65,10.5 H7.34 L7.15,5.07 H8.83z M8,13.7 c-0.55,0 -0.99,-0.5 -0.99,-1 0,-0.6 0.44,-1 0.99,-1 0.56,0 0.99,0.4 0.99,1 0,0.5 -0.43,1 -0.99,1z"/>

This should just be fill="context-fill", not fill="#context-fill"
Attachment #8979568 - Flags: review?(jaws) → review-
Attachment #8975884 - Attachment is obsolete: true
Comment on attachment 8979568 [details]
Bug 1454231 - Improved dark theme colors of popup warnings.

https://reviewboard.mozilla.org/r/245700/#review253000

::: browser/themes/shared/customizableui/panelUI.inc.css:620
(Diff revision 2)
>  }
>  
> +:root[lwt-popup-brighttext] .addon-banner-item {
> +  color: @appmenuWarningColorDark@;
> +  background: @appmenuWarningBackgroundColorDark@;
> +  border: 0 !important;

Please include a comment with the reason why you need to use !important here.

::: browser/themes/shared/notification-icons.inc.css:156
(Diff revision 2)
>    background: rgba(255, 217, 99, .8) url("chrome://browser/skin/warning-white.svg") no-repeat .75em .75em;
> +  -moz-context-properties: fill;
> +  fill: #fff;

Can we remove warning-white.svg and use warning.svg here, and update warning.svg to use context-fill and context-stroke?

The name warning-white.svg doesn't make sense anymore anyways since the file doesn't use the color white in there now.
Attachment #8979568 - Flags: review?(jaws) → review-
I should say that this is looking good and on the right path. I think with the above requested changes made this will be ready to land :)
Comment on attachment 8979568 [details]
Bug 1454231 - Improved dark theme colors of popup warnings.

https://reviewboard.mozilla.org/r/245700/#review254756

::: browser/themes/shared/warning.svg:5
(Diff revision 3)
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> -<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">
> -  <path fill="#ffbf00" d="M14.8,12.5L9.3,1.9C9,1.3,8.5,1,8,1C7.5,1,7,1.3,6.7,1.9L1.2,12.5c-0.3,0.6-0.3,1.2,0,1.7C1.5,14.7,2,15,2.6,15h10.8 c0.6,0,1.1-0.3,1.4-0.8C15.1,13.7,15.1,13.1,14.8,12.5z"/>
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16">
> +  <path fill="context-fill" stroke="context-stroke" stroke-opacity="0.3" d="M15.4,12.9 9.46,1.41 C9.12,0.756 8.59,0.381 8,0.381 7.41,0.381 6.88,0.756 6.54,1.41 L0.642,12.9 c-0.331,0.6 -0.348,1.3 -0.05,1.9 0.299,0.5 0.854,0.8 1.534,0.8 H13.9 c0.6,0 1.2,-0.3 1.5,-0.8 0.3,-0.6 0.3,-1.3 0,-1.9z M8.83,5.07 8.65,10.5 H7.34 L7.15,5.07 H8.83z M8,13.7 c-0.55,0 -0.99,-0.5 -0.99,-1 0,-0.6 0.44,-1 0.99,-1 0.56,0 0.99,0.4 0.99,1 0,0.5 -0.43,1 -0.99,1z"/>

You can use fill="context-fill #FFBF00" to specify a default color to use when fill isn't specified, to avoid having to specify the same fill all the time.

Same for context-stroke.
Comment on attachment 8979568 [details]
Bug 1454231 - Improved dark theme colors of popup warnings.

https://reviewboard.mozilla.org/r/245700/#review255176

::: browser/themes/shared/customizableui/panelUI.inc.css:30
(Diff revision 4)
> +%define appmenuWarningBackgroundColorDark hsla(55, 100%, 50%, 0.1)
> +%define appmenuWarningBackgroundColorHoverDark hsla(55, 100%, 50%, 0.15)
> +%define appmenuWarningBackgroundColorActiveDark hsla(55, 100%, 50%, 0.2)

nit, no spaces after commas in color functions and drop the preceding 0 in front of decimal points.

::: browser/themes/shared/customizableui/panelUI.inc.css:30
(Diff revision 4)
> +%define appmenuWarningBackgroundColorDark hsla(55, 100%, 50%, 0.1)
> +%define appmenuWarningBackgroundColorHoverDark hsla(55, 100%, 50%, 0.15)
> +%define appmenuWarningBackgroundColorActiveDark hsla(55, 100%, 50%, 0.2)
> +%define appmenuWarningColorDark #F9F9FA

Please replace 'Dark' with 'BrightText' so it is consistent with the other usages brighttext.

::: browser/themes/shared/notification-icons.inc.css:156
(Diff revision 4)
>    min-width: 300px;
>    max-height: 200px;
>  }
>  
>  #webRTC-previewWarning {
> -  background: rgba(255, 217, 99, .8) url("chrome://browser/skin/warning-white.svg") no-repeat .75em .75em;
> +  background: rgba(255, 217, 99, .8) url("chrome://browser/skin/warning.svg") no-repeat .75em .75em;

nit, no spaces after comma in color functions.
Attachment #8979568 - Flags: review?(jaws) → review+
(In reply to Tim Nguyen :ntim from comment #18)
> You can use fill="context-fill #FFBF00" to specify a default color to use
> when fill isn't specified, to avoid having to specify the same fill all the
> time.

I didn't know this before, thanks for sharing!
Comment on attachment 8979568 [details]
Bug 1454231 - Improved dark theme colors of popup warnings.

https://reviewboard.mozilla.org/r/245700/#review255208

::: browser/themes/shared/downloads/downloads.inc.css:146
(Diff revision 4)
> +  -moz-context-properties: fill, stroke;
> +  fill: #FFBF00;
> +  stroke: #fff;

This is the default fill/stroke, so you don't need to respecify it.
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0beab09f8058
Improved dark theme colors of popup warnings. r=jaws
https://hg.mozilla.org/mozilla-central/rev/0beab09f8058
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Depends on: 1471906
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: