Addon banner needs to be lighter on dark theme

RESOLVED FIXED in Firefox 62

Status

()

defect
P5
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: ntim, Assigned: harry)

Tracking

({good-first-bug})

unspecified
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 wontfix, firefox61 wontfix, firefox62 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Reporter

Description

Last year
Posted 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.
Reporter

Comment 1

Last year
Amy, would it be possible to get a specification for the colors ?
Flags: needinfo?(amlee)
Reporter

Comment 2

Last year
Posted 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)
Reporter

Updated

Last year
Blocks: 1408121
No longer blocks: dark-theme-darkening
Reporter

Comment 4

Last year
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
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 9

Last year
mozreview-review-reply
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.
Reporter

Comment 10

Last year
mozreview-review
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-
Assignee

Comment 11

Last year
mozreview-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-
Comment hidden (mozreview-request)
Assignee

Updated

Last year
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 hidden (mozreview-request)
Reporter

Comment 18

Last year
mozreview-review
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 hidden (mozreview-request)
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!
Reporter

Comment 22

Last year
mozreview-review
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 hidden (mozreview-request)

Comment 24

Last year
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0beab09f8058
Improved dark theme colors of popup warnings. r=jaws

Comment 25

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/0beab09f8058
Status: ASSIGNED → RESOLVED
Closed: Last year
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.