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.

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 ?

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.

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 )

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.

Comment 5 sounds great!

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`.

To make this simpler I have split off Vivek's work to bug 1431189. This bug will now only cover the `tints.*` work.

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

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

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

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.

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

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

Dylan, can you please use colors.icons and colors.icons_attention instead and remove the tints section ?

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

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.

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

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

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

Triggered a try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f9a0662391c

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.

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!

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

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.

(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).

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?

(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' }

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).

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?

Same as the other bug