1454231 - Addon banner needs to be lighter on dark theme

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P5)

Description

[Tim Nguyen :ntim]

Attachment: 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.

Comment 1

[Tim Nguyen :ntim]

Amy, would it be possible to get a specification for the colors ?

Comment 2

[Tim Nguyen :ntim]

Attachment: Sync banner

Looks like there's also a FxA banner that uses a similar color.

Comment 3

[Amy Lee [:amylee] UX]

Attachment: Screen Shot 2018-04-16 at 12.27.25.png

I've attached a spec to update the colours for attention notifications.

Comment 4

[Tim Nguyen :ntim]

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]

Comment 5

[Harry Twyford]

Attachment: Bug 1454231 - Improved dark theme colors of popup warnings.

Review commit: https://reviewboard.mozilla.org/r/244080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244080/

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 7

[Harry Twyford]

Attachment: Bug 1454231 - Improved dark theme colors of popup warnings.

Review commit: https://reviewboard.mozilla.org/r/245700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245700/

Comment 8

[Harry Twyford]

Comment on attachment 8975884 [details]
Bug 1454231 - Improved dark theme colors of popup warnings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244080/diff/1-2/

Comment 9

[Harry Twyford]

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 10

[Tim Nguyen :ntim]

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

Comment 11

[Harry Twyford]

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 12

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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"

Comment 14

[Harry Twyford]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245700/diff/1-2/

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

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 17

[Harry Twyford]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245700/diff/2-3/

Comment 18

[Tim Nguyen :ntim]

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 19

[Harry Twyford]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245700/diff/3-4/

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

(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 22

[Tim Nguyen :ntim]

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.

Comment 23

[Harry Twyford]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245700/diff/4-5/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0beab09f8058
Improved dark theme colors of popup warnings. r=jaws

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/0beab09f8058