Add toolbar button icon color properties

VERIFIED FIXED in Firefox 60

Status

P5
normal
VERIFIED FIXED
a year ago
2 months ago

People

(Reporter: mikedeboer, Assigned: dyl, Mentored)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla60
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox60 verified)

Details

(Whiteboard: triaged)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

a year ago
See (again) https://docs.google.com/spreadsheets/d/1YScpOVL5WdNDhQY2Nngh4YkK0bOpkfwJvpRjpMSxMWo/edit#gid=0

Constant as defined in https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/themes/theme_properties.h is:
 * COLOR_BUTTON_BACKGROUND
 * TINT_BUTTONS
 * COLOR_TOOLBAR_BUTTON_STROKE
 * COLOR_TOOLBAR_BUTTON_STROKE_INACTIVE
 * GRADIENT_TOOLBAR_BUTTON
 * GRADIENT_TOOLBAR_BUTTON_INACTIVE
 * GRADIENT_TOOLBAR_BUTTON_PRESSED
 * GRADIENT_TOOLBAR_BUTTON_PRESSED_INACTIVE

It's important here to first implement the properties in the order and with names that fit with Firefox well and try to match these up with the Google equivalents above.

Comment 1

a year ago
Dão, I've noticed the --toolbarbutton-icon-fill variable was removed in bug 1387723. Would you mind introducing a new similar variable for this bug ? If so, which name would it take ?

Updated

a year ago
Flags: needinfo?(dao+bmo)

Updated

11 months ago
Priority: -- → P5

Comment 2

11 months ago
Sure, we can add a variable specifically for lightweight themes. --toolbarbutton-icon-fill had a different (weak) use case.

As for the name, I think it might make sense to start using a common prefix for variables that only exist for lightweight theme support.
Flags: needinfo?(dao+bmo)

Comment 3

10 months ago
Only 

COLOR_BUTTON_BACKGROUND,
TINT_BUTTONS,

seem exposed to Google chrome themes (see https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/themes/theme_properties.h#28 )

Updated

10 months ago
Assignee: nobody → ntim.bugs

Updated

10 months ago
Blocks: 1386004

Updated

8 months ago
Assignee: ntim.bugs → nobody

Comment 4

8 months ago
Only tints.buttons and colors.button_background is exposed to chrome themes.

This means other properties can be implemented with better names on Firefox.

Here's what I'm suggesting:

tints.buttons - the toolbar button icon fill
tints.buttons_hover - the hover toolbar button icon fill
tints.buttons_active - the pressed toolbar button icon fill

tints.buttons_attention - the attention toolbar button icon fill
tints.buttons_attention_hover - the attention toolbar button fill on hover
tints.buttons_attention_active - the pressed attention toolbar button fill 

colors.button_background - the default toolbar button background
colors.button_background_hover
colors.button_background_active

colors.button_stroke - border 
...
...

gradients.button_background
...
...
mconley, dao, and myself looked further at this bug and have limited the scope of it to:

tints.buttons - the toolbar button icon fill
tints.buttons_attention - the attention toolbar button icon fill
colors.button_background, colors.button_background_hover, and colors.button_background_active

We've removed colors.button_stroke* since we don't have a stroke for buttons in our current design and gradients.button_background* since we don't currently support background-images for our buttons. There is no need for *_hover and *_active for the tints because we don't change icon colors in those states.
(Reporter)

Comment 6

7 months ago
Comment 5 sounds great!
Assignee: nobody → stokesdy
Status: NEW → ASSIGNED
I have asked Vivek to also work on this with Dylan.

Dylan can work on the `tints.*` and Vivek can work on the `colors.*` properties.

Note that the `tints.*` properties will need to have extra code added to https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/toolkit/components/extensions/ext-theme.js and a new section in https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/toolkit/components/extensions/schemas/theme.json since we currently don't support `details.tints`.
Summary: Add Google Chrome toolbar button color properties → Add Google Chrome toolbar button tint properties
To make this simpler I have split off Vivek's work to bug 1431189. This bug will now only cover the `tints.*` work.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

7 months ago
mozreview-review
Comment on attachment 8944975 [details]
Bug 1347184 - Add support for colors.icons and colors.icons_attention properties.

https://reviewboard.mozilla.org/r/215124/#review220738


Static analysis found 2 defects in this patch.
 - 2 defects found by mozlint

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


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


::: toolkit/components/extensions/ext-theme.js:88
(Diff revision 2)
>  
>      if (details.properties) {
>        this.loadProperties(details.properties);
>      }
>  
> +    if(details.tints) {

Error: Expected space(s) after "if". [eslint: keyword-spacing]

::: toolkit/components/extensions/ext-theme.js:89
(Diff revision 2)
>      if (details.properties) {
>        this.loadProperties(details.properties);
>      }
>  
> +    if(details.tints) {
> +      this.loadTints(details.tints)

Error: Missing semicolon. [eslint: semi]
Comment hidden (mozreview-request)

Comment 13

7 months ago
mozreview-review
Comment on attachment 8944975 [details]
Bug 1347184 - Add support for colors.icons and colors.icons_attention properties.

https://reviewboard.mozilla.org/r/215124/#review220788

Nice work!

::: toolkit/components/extensions/ext-theme.js:284
(Diff revision 3)
> +    for (let tint of Object.keys(tints)) {
> +      let val = tints[tint];

`for (let tint in tints) {` is probably more appropriate :)

(I realize loadColors should also get changed).

::: toolkit/components/extensions/ext-theme.js:296
(Diff revision 3)
> +      switch (tint) {
> +        case "buttons":
> +          this.lwtStyles.tints.button_color = cssColor;
> +          break;
> +        case "buttons_attention":
> +          this.lwtStyles.tints.button_attention_color = cssColor;
> +          break;
> +      }

It's probably easier if you set those as top-level items on this.lwtStyles rather than making a sub-item for tints.

this.lwtStyles.tint_button_color
this.lwtStyles.tint_button_attention_color

::: toolkit/modules/LightweightThemeConsumer.jsm:172
(Diff revision 3)
> +    if (aData.tints) {
> +      _setProperty(root, active, "--lwt-toolbarbutton-icon-fill", this._sanitizeCSSColor(aData.tints.button_color) || "black");
> +      _setProperty(root, active, "--toolbarbutton-icon-fill-attention", this._sanitizeCSSColor(aData.tints.button_attention_color) || "blue");
> +    }

Setting the tints as top-level items in lwtStyles means you can just put the variables in the "kCSSVarsMap" map on top of the file without having to call _setProperty manually.
Are we sure we want to copy the "tints" terminology for this? It's not even clear to me why this should be separate from the colors namespace. E.g. this seems significantly cleaner to me:

colors.icons
colors.icons_attention
colors.button_background
colors.button_background_hover
colors.button_background_active

Comment 15

7 months ago
(In reply to Dão Gottwald [::dao] from comment #14)
> Are we sure we want to copy the "tints" terminology for this? It's not even
> clear to me why this should be separate from the colors namespace. E.g. this
> seems significantly cleaner to me:
> 
> colors.icons
> colors.icons_attention
> colors.button_background
> colors.button_background_hover
> colors.button_background_active

We use tints.buttons simply for chrome compatibility, and tints.buttons_attention to be consistent with tints.buttons.
I figured as much. I don't think we should follow Chrome here. tints.buttons is confusing; it's not clear enough that this is about the icon.

Perhaps we can support tints.buttons as an alias for compatibility and document it as such?

Comment 17

7 months ago
(In reply to Dão Gottwald [::dao] from comment #16)
> I figured as much. I don't think we should follow Chrome here. tints.buttons
> is confusing; it's not clear enough that this is about the icon.
> 
> Perhaps we can support tints.buttons as an alias for compatibility and
> document it as such?

Hmm, I've just tested button tints on chrome again, and I've noticed they aren't supported anymore on the latest Chrome versions.

Apart from that, I just remembered chrome tints have a different syntax (they have a [h, s, l] syntax with a 0-1 scale).

I guess I'm ok with using colors.icons and colors.icons_attention and not supporting the chrome tints counterparts then.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8944975 [details]
Bug 1347184 - Add support for colors.icons and colors.icons_attention properties.

https://reviewboard.mozilla.org/r/215124/#review221092

Looks good, you're on the right path. See my notes below about some changes to make.

::: browser/themes/shared/toolbarbutton-icons.inc.css:7
(Diff revision 5)
>    --toolbarbutton-icon-fill-attention: #0a84ff;
>  }
>  
>  :root:-moz-lwtheme {
>    --toolbarbutton-icon-fill-opacity: 1;
> +  --lwt-toolbarbutton-icon-fill: #ff0000;

This looks like it was added for testing but should be removed now.

::: toolkit/components/extensions/ext-theme.js:88
(Diff revision 5)
> +    if (details.tints) {
> +      this.loadTints(details.tints);

Please change to using `colors` as was decided in comment 16 and comment 17.

::: toolkit/components/extensions/ext-theme.js:284
(Diff revision 5)
> +    for (let tint in tints) {
> +      let val = tints[tint];
> +
> +      if (!val) {
> +        continue;
> +      }

`for (let [name, value] of Object.entries(tints)) {`
will create two variables (`name` and `value`) you can use instead of using `tints[tint]` to get access to the value.

Note that I have recommend a for-of loop, not for-in. for-in will only provide the keys.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_tints.js:33
(Diff revision 5)
> +    },
> +  });
> +
> +  await extension.startup();
> +
> +  let toolbarbutton = document.querySelector(".toolbarbutton-1");

Can you choose a specific button by using an ID instead of just the first .toolbarbutton-1 ? I'm asking so we can make the test a little more predictable as the first .toolbarbutton-1 may change without thought to this.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_tints.js:40
(Diff revision 5)
> +  let root = document.querySelector(":root");
> +  Assert.equal(
> +    window.getComputedStyle(root).getPropertyValue("--toolbarbutton-icon-fill-attention"),

You can fake an attention state on the bookmark star which will then use the attention color. Just set the `starred="true"` attribute on the `#star-button`.

You will need to make sure that the current webpage is not about:home or about:newtab since we don't show the star-button on those pages.

This will be a better way to test it since we want to make sure that the color from the theme is actually changing the fill color on an icon.
Attachment #8944975 - Flags: review?(jaws) → review-

Comment 21

7 months ago
Jared, wdyt about dao's comment 14 ? I realized chrome stopped supporting button tints in recent versions of chrome, not to mention tints have a different [H, S, L] syntax, so I guess it means we can use names that makes sense for us.
Flags: needinfo?(jaws)

Comment 22

7 months ago
mozreview-review
Comment on attachment 8944975 [details]
Bug 1347184 - Add support for colors.icons and colors.icons_attention properties.

https://reviewboard.mozilla.org/r/215124/#review221226

::: browser/themes/shared/toolbarbutton-icons.inc.css:513
(Diff revision 5)
>  /* ----- BOOKMARK BUTTONS ----- */
>  
>  .bookmark-item {
>    list-style-image: url("chrome://mozapps/skin/places/defaultFavicon.svg");
>    -moz-context-properties: fill, fill-opacity;
> -  fill: currentColor;
> +  fill: var(--lwt-toolbarbutton-fill,currentColor);

nit: space after comma
(Reporter)

Comment 23

7 months ago
(In reply to Tim Nguyen :ntim from comment #21)
> Jared, wdyt about dao's comment 14 ? I realized chrome stopped supporting
> button tints in recent versions of chrome, not to mention tints have a
> different [H, S, L] syntax, so I guess it means we can use names that makes
> sense for us.

Good riddance!! I felt really uncomfortable having that asymmetric design pollute the Theming API and now we don't have to :)

I'd be happy if we'd support Dão's suggestions in comment 14.
Flags: needinfo?(jaws)

Comment 24

7 months ago
Dylan, can you please use colors.icons and colors.icons_attention instead and remove the tints section ?
Flags: needinfo?(stokesdy)

Updated

7 months ago
Blocks: 1348034
No longer blocks: 1347171
Summary: Add Google Chrome toolbar button tint properties → Add toolbar button icon color properties
(Assignee)

Comment 25

7 months ago
(In reply to Tim Nguyen :ntim from comment #24)
> Dylan, can you please use colors.icons and colors.icons_attention instead
> and remove the tints section ?

Sure thing. I'll begin to make those changes.
Flags: needinfo?(stokesdy)
Comment on attachment 8944975 [details]
Bug 1347184 - Add support for colors.icons and colors.icons_attention properties.

https://reviewboard.mozilla.org/r/215124/#review221842

::: toolkit/modules/LightweightThemeConsumer.jsm:29
(Diff revision 5)
>    ["--lwt-toolbar-field-border-color", "toolbar_field_border"],
>    ["--tabs-border-color", "toolbar_top_separator"],
>    ["--toolbox-border-bottom-color", "toolbar_bottom_separator"],
>    ["--urlbar-separator-color", "toolbar_vertical_separator"],
> +  ["--lwt-toolbarbutton-icon-fill", "tints_button_color"],
> +  ["--toolbarbutton-icon-fill-attention", "tints_button_attention_color"],

To fix the issue you have seen with [brighttext], you should do the following:

Change this line from `--toolbarbutton-icon-fill-attention` to `--lwt-toolbarbutton-icon-fill-attention-without-fallback-color`

Because `--toolbarbutton-icon-fill-attention` is set by a couple rules, we will need to use a different variable name for the value that comes from themes.

Then, in toolbarbutton-icons.inc.css, you should change
```css
toolbar[brighttext] {
  --toolbarbutton-icon-fill-attention: #45a1ff;
}
```
to
```css
toolbar[brighttext] {
  --toolbarbutton-icon-fill-attention: var(--lwt-toolbarbutton-icon-fill-attention-without-fallback-color, #45a1ff);
}
```

And change:
```css
:root {
  --toolbarbutton-icon-fill-attention: #0a84ff;
}
```
to
```css
:root {
  --toolbarbutton-icon-fill-attention: var(--lwt-toolbarbutton-icon-fill-attention-without-fallback-color, #0a84ff);
}
```

The reason I'm asking you to use such a long (and ugly) name is that we don't want this custom property copied around our codebase since it's value will not incorporate the correct fallback value. The variable name is designed to be ugly and cause confusion, bringing developers to the point in code where it is used and thus they will see a comment explaining it.

At the first usage of the property in toolbarbutton-icons.inc.css (for the `:root` selector), please add the following comment:

/* --lwt-toolbarbutton-icon-fill-attention-without-fallback-color has a long and ugly name to prevent its usage in other places than here, specifically because its value will not fall back to the non-lwt provided color. */

Then please add the following comment at the second usage of the property in toolbarbutton-icons.inc.css (the `toolbar[brighttext]` selector):

/* Please see comment above for explanation of --lwt-toolbarbutton-icon-fill-attention-without-fallback-color. */
ntim has suggested using --lwt-toolbarbutton-icon-fill-attention-without-fallback, which I think is sufficient and a bit more concise. You can drop the -color suffix.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #26)
> /* --lwt-toolbarbutton-icon-fill-attention-without-fallback-color has a long
> and ugly name to prevent its usage in other places than here, specifically
> because its value will not fall back to the non-lwt provided color. */

This is basically the point of the --lwt prefix. Most --lwt variables already don't have a fallback, so it's not clear to me that "without fallback" should be highlighted here.

Comment 29

7 months ago
(In reply to Dão Gottwald [::dao] from comment #28)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #26)
> > /* --lwt-toolbarbutton-icon-fill-attention-without-fallback-color has a long
> > and ugly name to prevent its usage in other places than here, specifically
> > because its value will not fall back to the non-lwt provided color. */
> 
> This is basically the point of the --lwt prefix. Most --lwt variables
> already don't have a fallback, so it's not clear to me that "without
> fallback" should be highlighted here.


:jaws reason for the ugly name is that so other people don't accidentally mistake using the --lwt variable and the non --lwt variable in wrong situations.
I don't understand how this answers to my comment.

Comment 31

7 months ago
(In reply to Dão Gottwald [::dao] from comment #30)
> I don't understand how this answers to my comment.

The --lwt- prefix is small and can be overlooked and easily mis-used. If the variable is longer and uglier, it will make other people think about whether they're using the variable properly. I don't have a preference over having the suffix or not, so you might want to talk with :jaws about this.

Comment 32

7 months ago
If it's the "without-fallback-color" bit that's bothering you, we can potentially use a different suffix name.
(In reply to Tim Nguyen :ntim from comment #31)
> (In reply to Dão Gottwald [::dao] from comment #30)
> > I don't understand how this answers to my comment.
> 
> The --lwt- prefix is small and can be overlooked and easily mis-used. If the
> variable is longer and uglier, it will make other people think about whether
> they're using the variable properly.

Then we should rename all --lwt variables accordingly. I suspect we're not going to do this, so we should just stick to the existing naming scheme here.
Okay, it's fine with me if we just use --lwt-toolbarbutton-icon-fill-attention and --lwt-toolbarbutton-icon-fill. ntim summarized my viewpoints well, but I'm fine with dropping my issue.
Comment hidden (mozreview-request)

Comment 36

7 months ago
mozreview-review
Comment on attachment 8944975 [details]
Bug 1347184 - Add support for colors.icons and colors.icons_attention properties.

https://reviewboard.mozilla.org/r/215124/#review222604

::: toolkit/components/extensions/ext-theme.js:152
(Diff revision 6)
> +        case "buttons":
> +          this.lwtStyles.icons = cssColor;
> +          break;
> +        case "buttons_attention":

Sorry for the confusion :) Let's use "icons" and "icons_attention" here too.

::: toolkit/components/extensions/schemas/theme.json:128
(Diff revision 6)
> +              "buttons": {
> +                "$ref": "ThemeColor",
> +                "optional": true
> +              },
> +              "buttons_attention": {
> +                "$ref": "ThemeColor",
> +                "optional": true

Same here

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_tints.js:19
(Diff revision 6)
> +          "buttons": BUTTONS_COLOR,
> +          "buttons_attention": BUTTONS_ATTENTION_COLOR,

Same here, let's use icons/icons_attention.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_tints.js:38
(Diff revision 6)
> +  let root = document.querySelector("#star-button");
> +  root.setAttribute("starred", "true");
> +  Assert.equal(
> +    window.getComputedStyle(root).getPropertyValue("--lwt-toolbarbutton-icon-fill-attention"),
> +    BUTTONS_ATTENTION_COLOR,
> +    "Buttons attention color set!"
> +  );

This is not actually testing the fill...

Try something like this:

  let starButton = document.querySelector("#star-button");
  starButton.setAttribute("starred", "true");
  
  let starComputedStyle = window.getComputedStyle(starButton);

  Assert.equal(
    starComputedStyle.getPropertyValue("--toolbarbutton-icon-fill-attention"),
    ICONS_ATTENTION_COLOR,
    "Variable is properly set"
  );
  Assert.equal(
    starComputedStyle.getPropertyValue("fill"),
    `rgb(${hexToRGB(ICONS_ATTENTION_COLOR).join(", ")})`,
    "Starred icon fill is properly set"
  );
  
  starButton.removeAttribute("starred");

Comment 37

7 months ago
mozreview-review
Comment on attachment 8944975 [details]
Bug 1347184 - Add support for colors.icons and colors.icons_attention properties.

https://reviewboard.mozilla.org/r/215124/#review222610

::: commit-message-3d23e:1
(Diff revision 6)
> +Bug 1347184 - Add Google Chrome toolbar button tint properties. r?jaws

The commit message needs to be updated:

Bug 1347184 - Add support for colors.icons and colors.icons_attention properties. r?jaws
Comment hidden (mozreview-request)

Updated

7 months ago
Keywords: dev-doc-needed

Comment 40

7 months ago
mozreview-review
Comment on attachment 8944975 [details]
Bug 1347184 - Add support for colors.icons and colors.icons_attention properties.

https://reviewboard.mozilla.org/r/215124/#review222976

::: toolkit/components/extensions/ext-theme.js:149
(Diff revision 7)
>          case "toolbar_vertical_separator":
>            this.lwtStyles[color] = cssColor;
>            break;
> +        case "icons":
> +          this.lwtStyles.icons = cssColor;
> +          break;
> +        case "icons_attention":
> +          this.lwtStyles.icons_attention = cssColor;
> +          break;

nit: combine the 3 cases

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_tints.js:53
(Diff revision 7)
> +  starButton.removeAttribute("starred");
> +
> +
> +  await extension.unload();

nit: remove blank lines
Comment on attachment 8944975 [details]
Bug 1347184 - Add support for colors.icons and colors.icons_attention properties.

https://reviewboard.mozilla.org/r/215124/#review222984

This looks good. Please make the changes that ntim and I have requested and re-upload, then we can get this landed.

::: toolkit/components/extensions/test/browser/browser.ini:20
(Diff revision 7)
>  [browser_ext_themes_separators.js]
>  [browser_ext_themes_static_onUpdated.js]
>  [browser_ext_themes_tab_text.js]
>  [browser_ext_themes_toolbar_fields.js]
>  [browser_ext_themes_toolbars.js]
> +[browser_ext_themes_toolbarbutton_tints.js]

Please rename this file to browser_ext_themes_toolbarbutton_icons.js since we're not using the "tints" name anymore.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_tints.js:4
(Diff revision 7)
> +"use strict";
> +
> +// This test checks whether applied WebExtension themes that attempt to change
> +// tint properties

Swap "tint properties" with "icon colors" here.
Attachment #8944975 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Comment on attachment 8944975 [details]
Bug 1347184 - Add support for colors.icons and colors.icons_attention properties.

https://reviewboard.mozilla.org/r/215124/#review223038

Thanks!

Comment 44

7 months ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0b217170ed52
Add support for colors.icons and colors.icons_attention properties. r=jaws

Comment 45

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0b217170ed52
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60

Updated

7 months ago
Depends on: 1435117
Icons got black.

mozregression --good 2018-01-31 --bad 2018-02-01 --pref startup.homepage_welcome_url:"https://addons.mozilla.org/en-US/firefox/addon/quantum-lights-dynamic/"
> 6:17.44 INFO: Last good revision: bf1ed01c25ea2de9da041c5d09bc893a562af9ef
> 6:17.44 INFO: First bad revision: 0b217170ed529e17e68838ab83d14100dc9adcd8
> 6:17.44 INFO: Pushlog:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=bf1ed01c25ea2de9da041c5d09bc893a562af9ef&tochange=0b217170ed529e17e68838ab83d14100dc9adcd8

> 0b217170ed52	Dylan Stokes — Bug 1347184 - Add support for colors.icons and colors.icons_attention properties. r=jaws
(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #46)
> Icons got black.
> 
> mozregression --good 2018-01-31 --bad 2018-02-01 --pref
> startup.homepage_welcome_url:"https://addons.mozilla.org/en-US/firefox/addon/
> quantum-lights-dynamic/"
> > 6:17.44 INFO: Last good revision: bf1ed01c25ea2de9da041c5d09bc893a562af9ef
> > 6:17.44 INFO: First bad revision: 0b217170ed529e17e68838ab83d14100dc9adcd8
> > 6:17.44 INFO: Pushlog:
> > https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=bf1ed01c25ea2de9da041c5d09bc893a562af9ef&tochange=0b217170ed529e17e68838ab83d14100dc9adcd8
> 
> > 0b217170ed52	Dylan Stokes — Bug 1347184 - Add support for colors.icons and colors.icons_attention properties. r=jaws

Thank you for the report, this is on file as bug 1435117.
(In reply to Raul Gurzau (:RaulGurzau) from comment #45)
> https://hg.mozilla.org/mozilla-central/rev/0b217170ed52

Woohoo, thanks Dylan! This has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#Add-on_Contributor_Recognition

Also, would you be interested in creating an account on mozillians.org? I'd love to vouch for you.
(Assignee)

Comment 49

7 months ago
(In reply to Caitlin Neiman [:caitmuenster] from comment #48)
> (In reply to Raul Gurzau (:RaulGurzau) from comment #45)
> > https://hg.mozilla.org/mozilla-central/rev/0b217170ed52
> 
> Woohoo, thanks Dylan! This has been added to our recognition wiki:
> https://wiki.mozilla.org/Add-ons/Contribute/Recognition#Add-
> on_Contributor_Recognition
> 
> Also, would you be interested in creating an account on mozillians.org? I'd
> love to vouch for you.

Woo, Awesome! I just created a mozillians.org account (dyl).

Comment 50

6 months ago
Created attachment 8949350 [details]
buton icons color.PNG

I have tested FF 59.0a1 (20171201220040) vs FF 60.0a1 (20180208103049) using https://addons.mozilla.org/en-US/firefox/addon/quantum-lights-dynamic/ and was unable to find the difference, can you please provide STR or a different addon?
Flags: needinfo?(stokesdy)
(Assignee)

Comment 51

6 months ago
Created attachment 8949573 [details]
example.png

(In reply to marius.santa from comment #50)
> Created attachment 8949350 [details]
> buton icons color.PNG
> 
> I have tested FF 59.0a1 (20171201220040) vs FF 60.0a1 (20180208103049) using
> https://addons.mozilla.org/en-US/firefox/addon/quantum-lights-dynamic/ and
> was unable to find the difference, can you please provide STR or a different
> addon?

This bug adds the icon and icon_attention properties. Any current themes would need to update and define these new properties in order to change the color of the icons/icon_attention.

This is what an example would look like for changing the icons to magenta and the icon attention color to green (see attachment):

colors: {
    icons: '#ff00ff',
    icons_attention: '#00ff00'
}
Flags: needinfo?(stokesdy)

Comment 52

6 months ago
Created attachment 8952349 [details]
icon color.gif

I was able to reproduce the issue on Windows 10 64Bit Firefox 60.0a1(20180130102929).
Tested and verified on Windows 10 64Bit and Mac OS  X 10.13.2 in Firefox  	60.0a1 (20180220103456).

Updated

6 months ago
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme is updated with colors.icons and colors.icons_attention. Is that everything for this bug?
Flags: needinfo?(ntim.bugs)

Comment 54

6 months ago
Same as the other bug
Flags: needinfo?(ntim.bugs)
Keywords: dev-doc-needed → dev-doc-complete

Updated

2 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.