1441708 - theme_icons is applied incorrectly when frame is dark and toolbar is light

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[Tim Nguyen :ntim]

STR:
- Install Tree Style Tab
- Install a theme where the frame is dark and the toolbar is light

The dark theme_icon (white) gets applied on the light toolbar

Comment 1

[Tim Nguyen :ntim]

https://dxr.mozilla.org/mozilla-central/rev/580d833df9c44acec686a9fb88b5f27e9d29f68d/browser/base/content/browser.css#379-400

Seems like this should be using `toolbar[brighttext]` rather than :-moz-lwtheme-brighttext

Comment 2

[Mike Conca [:mconca]]

Tim, it seems like you have a good handle on the issue as well as a potential solution. Would you be open to landing a patch to fix this?

Comment 3

[Tim Nguyen :ntim]

I don't have a lot of time atm to provide a full fix (one with complete automated tests). Also, a proper fix for the overflow panel needs the work from bug 1417883, so it seems like a good fit the "Dark Theme Darkening" work.

Comment 4

[Mike Conca [:mconca]]

(In reply to Tim Nguyen :ntim from comment #3)
> I don't have a lot of time atm to provide a full fix (one with complete
> automated tests).

Thanks, :ntim. I appreciate the consideration.

> Also, a proper fix for the overflow panel needs the work
> from bug 1417883, so it seems like a good fit the "Dark Theme Darkening"
> work.

:mconley How do you feel about adding this to Bug 1432172?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Makes sense - adding. Thanks for the heads up.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Hey mixedpuppy, we have a set of students working in this area, and one is about to free up for more work. Have you already started this? If not, can we assign it to one of our students?

If you've already started it, don't worry, we can find other stuff.

Comment 7

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

Attachment: manifest.json

Comment 8

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

Attachment: 2018-04-13_1534.png

Screenshot of attached manifest.json on local build using 0528a414c2a8.

I can't reproduce this bug using the attached manifest.json. I'll close this bug now as worksforme as this was likely fixed in the interim.

Comment 9

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

Testing with the Nightly build on Windows from the day that this bug was filed, I'm unable to reproduce this bug. Using the attached manifest I still get the same outcome as the attached screenshot.

Comment 10

[Martin Giger [:freaktechnik]]

I do see the wrong theme_icons used when using dynamic themes like VivaldiFox or Themr. It seems the theme_icon is not changed dynamically together with the toolbar font. I don't think this is directly related to the contrast between tab and toolbar.

ntim refereed me to this issue when a user reported such an issue occurring with the icon of my extension in combination with VivaldiFox.

Comment 11

[Tim Nguyen :ntim]

Attachment: Screenshot

I can still reproduce.

Tree style tab uses the wrong icon in this case.

Comment 12

[Tim Nguyen :ntim]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Testing with the Nightly build on Windows from the day that this bug was
> filed, I'm unable to reproduce this bug. Using the attached manifest I still
> get the same outcome as the attached screenshot.

Probably doesn't happen in the screenshot because "blue" computes as dark text, which means it will choose the black icon (which is correct in this case). Try changing the textcolor to white for example.

Comment 13

[Connor Masini]

Attachment: Bug 1441708 - theme_icons is applied incorrectly when frame is dark and toolbar is light

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

Comment 14

[Connor Masini]

Comment on attachment 8969767 [details]
Bug 1441708 - theme_icons is applied incorrectly when frame is dark and toolbar is light

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

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8969767 [details]
Bug 1441708 - theme_icons is applied incorrectly when frame is dark and toolbar is light

https://reviewboard.mozilla.org/r/238584/#review244326

::: browser/base/content/browser.css:376
(Diff revision 2)
>  @media not all and (min-resolution: 1.1dppx) {
>    .webextension-browser-action {
>      list-style-image: var(--webextension-toolbar-image, inherit);
>    }
>  
> -  .webextension-browser-action:-moz-lwtheme-brighttext {
> +  .webextension-browser-action > toolbar[brighttext] {

This selector is backwards. The toolbarbutton (.webextension-browser-action) is inside the toolbar:

```
toolbar[brighttext] .webextension-browser-action
```
would be the correct one.

::: browser/base/content/browser.css:380
(Diff revision 2)
>  
> -  .webextension-browser-action:-moz-lwtheme-brighttext {
> +  .webextension-browser-action > toolbar[brighttext] {
>      list-style-image: var(--webextension-toolbar-image-light, inherit);
>    }
>  
> -  .webextension-browser-action:-moz-lwtheme-darktext {
> +  .webextension-browser-action > toolbar[brighttext] {

This should be:

toolbar:not([brighttext]) .webextension-browser-action

Since you want to target the non-brighttext case here.

Comment 16

[Connor Masini]

Comment on attachment 8969767 [details]
Bug 1441708 - theme_icons is applied incorrectly when frame is dark and toolbar is light

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

Comment 17

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

Comment on attachment 8969767 [details]
Bug 1441708 - theme_icons is applied incorrectly when frame is dark and toolbar is light

https://reviewboard.mozilla.org/r/238584/#review246472

::: browser/base/content/browser.css:376
(Diff revision 3)
> -  .webextension-browser-action:-moz-lwtheme-brighttext {
> +  toolbar[brighttext] .webextension-browser-action {
>      list-style-image: var(--webextension-toolbar-image-light, inherit);
>    }
>  
> -  .webextension-browser-action:-moz-lwtheme-darktext {
> +  toolbar:not([brighttext]) .webextension-browser-action{
>      list-style-image: var(--webextension-toolbar-image-dark, inherit);
>    }

Please swap the order of these rules, so the :not() rule is first. Then you can remove the :not() selector.

For example,

toolbar .webextension-browser-action {
  list-style-image: ...
}

toolbar[brighttext] .webextension-browser-action {
  list-style-image: ...
}

Note, please also make sure that there is a space before the curly bracket.

::: browser/base/content/browser.css:389
(Diff revision 3)
> -  toolbarpaletteitem[place="palette"] > .webextension-browser-action:-moz-lwtheme-brighttext {
> +  toolbarpaletteitem[place="palette"] > .webextension-browser-action > toolbar[brighttext] {
>      list-style-image: var(--webextension-menupanel-image-light, inherit);
>    }
>  
> -  .webextension-browser-action[cui-areatype="menu-panel"]:-moz-lwtheme-darktext,
> +  .webextension-browser-action[cui-areatype="menu-panel"] > toolbar[brighttext],

I don't see how a toolbar could be a child of a .webextension-browser-action.

::: browser/components/tests/browser/browser.ini:1
(Diff revision 3)
>  [DEFAULT]

Please revert your changes to this file since they're only whitespace changes and appear to be unrelated to this bug.

Comment 18

[Caitlin Neiman [:caitmuenster]]

Hey Connor, just checking in on this bug since it's been marked as a P1 and hasn't been updated in 2 months. Are you still working on it? (And :jaws, is this still a P1?)

Comment 19

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

Yeah, this is still P1. Since Connor still hasn't replied let's see if Tim can take this.

Comment 20

[Tim Nguyen :ntim]

Attachment: Bug 1441708 - Fix theme_icons in different configurations.

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

Comment 21

[Code Review Bot [:reviewbot]]

Comment on attachment 8993450 [details]
Bug 1441708 - Fix theme_icons in different configurations.

https://reviewboard.mozilla.org/r/258200/#review265200


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/extensions/test/browser/browser_ext_browserAction_theme_icons.js:35
(Diff revision 1)
> -  let {themeType, themeIcons, withDefaultIcon} = options;
> +  let {
> +    themeData,
> +    themeIcons,
> +    withDefaultIcon,
> +    expectedIcon,
> +    defaultArea = "navbar"

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/test/browser/browser_ext_browserAction_theme_icons.js:143
(Diff revision 1)
> +add_task(async function browseraction_theme_icons_different_toolbars() {
> +  let themeData = {
> +    "accentcolor": "#000",
> +    "textcolor": "#fff",
> +    "toolbar": "#fff",
> +    "toolbar_text": "#000"

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/test/browser/browser_ext_browserAction_theme_icons.js:198
(Diff revision 1)
> +});
> +
> +add_task(async function browseraction_theme_icons_overflow_panel() {
> +  let themeData = {
> +    "popup": "#000",
> +    "popup_text": "#fff"

Error: Missing trailing comma. [eslint: comma-dangle]

Comment 22

[Tim Nguyen :ntim]

Comment on attachment 8993450 [details]
Bug 1441708 - Fix theme_icons in different configurations.

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

Comment 23

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

Comment on attachment 8993450 [details]
Bug 1441708 - Fix theme_icons in different configurations.

https://reviewboard.mozilla.org/r/258200/#review266476

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fedcc832ac7c
Fix theme_icons in different configurations. r=jaws

Comment 25

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/fedcc832ac7c

Comment 26

[Vlad Jiman]

Verified on Nightly 63.0a1 using Windows 10 x64 bit and MacOS 10.13.2 with multiple dark themes, icons appear correctly using this version of firefox, the problem appears to be solved.

Comment 27

[Vlad Jiman]

Attachment: Verified