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

VERIFIED FIXED in Firefox 63

Status

P1
normal
VERIFIED FIXED
a year ago
8 months ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

({good-first-bug})

unspecified
mozilla63
good-first-bug

Firefox Tracking Flags

(firefox63 verified)

Details

(Whiteboard: [ntim-intern-project])

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
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
(Assignee)

Comment 1

a year ago
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
Priority: -- → P1
Flags: needinfo?(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?
Flags: needinfo?(mconca) → needinfo?(ntim.bugs)
(Assignee)

Comment 3

a year ago
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.
Depends on: 1417883
Flags: needinfo?(ntim.bugs)
(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?
Flags: needinfo?(mconley)
Makes sense - adding. Thanks for the heads up.
Blocks: 1432172
Flags: needinfo?(mconley)
(Assignee)

Updated

a year ago
Mentor: ntim.bugs
Keywords: good-first-bug
Assignee: nobody → mixedpuppy
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.
Flags: needinfo?(mixedpuppy)
Assignee: mixedpuppy → masinico
Status: NEW → ASSIGNED
Flags: needinfo?(mixedpuppy)
Posted image 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.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → WORKSFORME
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.
Flags: needinfo?(ntim.bugs)
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.
(Assignee)

Comment 11

a year ago
Posted image Screenshot
I can still reproduce.

Tree style tab uses the wrong icon in this case.
Flags: needinfo?(ntim.bugs)
(Assignee)

Updated

a year ago
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Comment 12

a year ago
(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 hidden (mozreview-request)
(Assignee)

Comment 15

11 months ago
mozreview-review
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 hidden (mozreview-request)
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.
Attachment #8969767 - Flags: review-

Updated

9 months ago
Product: Toolkit → WebExtensions
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?)
Flags: needinfo?(masinico)
Flags: needinfo?(jaws)
(Assignee)

Updated

9 months ago
No longer blocks: 1432172
Yeah, this is still P1. Since Connor still hasn't replied let's see if Tim can take this.
Assignee: masinico → nobody
Status: REOPENED → NEW
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(masinico)
Flags: needinfo?(jaws)
(Assignee)

Updated

8 months ago
Assignee: nobody → ntim.bugs
Flags: needinfo?(ntim.bugs)
Comment hidden (mozreview-request)

Comment 21

8 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8969767 - Attachment is obsolete: true

Comment 24

8 months ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fedcc832ac7c
Fix theme_icons in different configurations. r=jaws
(Assignee)

Updated

8 months ago
Mentor: ntim.bugs

Comment 25

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fedcc832ac7c
Status: NEW → RESOLVED
Last Resolved: a year ago8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Comment 26

8 months ago
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.

Updated

8 months ago
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified

Comment 27

8 months ago
Posted image Verified
(Assignee)

Updated

8 months ago
Whiteboard: [ntim-intern-project]
You need to log in before you can comment on or make changes to this bug.