Split off from bug 1347184. This bug should implement the `colors.*` properties described in bug 1347184.

Comment on attachment 8946977 [details] Bug 1431189 - Add google chrome toolbar button color properties: https://reviewboard.mozilla.org/r/216818/#review222758 ::: toolkit/components/extensions/ext-theme.js:150 (Diff revision 2) > this.lwtStyles[color] = cssColor; > break; > + case "button_background": > + this.lwtStyles.button_background = cssColor; > + break; > + case "button_hover_background": > + this.lwtStyles.button_hover_background = cssColor; > + break; > + case "button_active_background": > + this.lwtStyles.button_active_background = cssColor; > + break; Combine these 3 cases with case above. ::: toolkit/components/extensions/schemas/theme.json:76 (Diff revision 2) > + "button_background": { > + "$ref": "ThemeColor", > + "optional": true > + }, > + "button_hover_background": { > + "$ref": "ThemeColor", > + "optional": true > + }, > + "button_active_background": { > + "$ref": "ThemeColor", > + "optional": true > + }, I prefer using button_background_hover/active as opposed to button_hover/active_background Also, please move these properties at the end of the block (after "toolbar_vertical_separator"). ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js:3 (Diff revision 2) > +// This test checks whether applied WebExtension themes that attempt to change > +// the background color properties are applied correctly. Can you change "the background color properties" to "the button background color properties" ? ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js:34 (Diff revision 2) > + let toolbarbutton = document.querySelector("#home-button"); > + > + Assert.equal( This is just testing the variable value, not testing whether the background is actually set. Try this: Components.utils.importGlobalProperties(["InspectorUtils"]); let toolbarButton = document.querySelector("#home-button"); let toolbarButtonIcon = document.getAnonymousElementByAttribute(toolbarButton, "class", "toolbarbutton-icon"); Assert.equal( window.getComputedStyle(toolbarbuttonIcon).getPropertyValue("background-color"), `rgb(${hexToRgb(BUTTON_BACKGROUND).join(", ")})`, "Toolbar button background is set." ); InspectorUtils.addPseudoClassLock(toolbarButton, ":hover"); Assert.equal( window.getComputedStyle(toolbarbuttonIcon).getPropertyValue("background-color"), `rgb(${hexToRgb(BUTTON_BACKGROUND_HOVER).join(", ")})`, "Toolbar button hover background is set." ); InspectorUtils.addPseudoClassLock(toolbarButton, ":active"); Assert.equal( window.getComputedStyle(toolbarbuttonIcon).getPropertyValue("background-color"), `rgb(${hexToRgb(BUTTON_BACKGROUND_ACTIVE).join(", ")})`, "Toolbar button active background is set!" ); InspectorUtils.clearPseudoClassLocks(toolbarButton);

Comment on attachment 8946977 [details] Bug 1431189 - Add google chrome toolbar button color properties: https://reviewboard.mozilla.org/r/216818/#review222992 ntim's review here is sufficient.

Comment on attachment 8946977 [details] Bug 1431189 - Add google chrome toolbar button color properties: https://reviewboard.mozilla.org/r/216818/#review223372 r- for the lack of passing test. Please fix the test and make sure it passes before submitting for review. ::: toolkit/components/extensions/ext-theme.js:149 (Diff revision 3) > case "toolbar_vertical_separator": > this.lwtStyles[color] = cssColor; > break; > + case "button_background": > + this.lwtStyles.button_background = cssColor; > + break; > + case "button_background_hover": > + this.lwtStyles.button_background_hover = cssColor; > + break; > + case "button_background_active": > + this.lwtStyles.button_background_active = cssColor; > + break; Since these three strings ("button_background", "button_background_hover", and "button_background_active") are equal to their property names, we don't need to separately handle them here. Instead we can add them to the list above (line 149) like so: ```js ... case "toolbar_vertical_separator": case "button_background": case "button_background_hover": case "button_background_active": this.lwtStyles[color] = cssColor; break; ``` ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js:40 (Diff revision 3) > + let toolbarButton = document.querySelector("#home-button"); > + let toolbarButtonIcon = document.getAnonymousElementByAttribute(toolbarButton, "class", "toolbarbutton-icon"); > + > + Assert.equal( > + window.getComputedStyle(toolbarButtonIcon).getPropertyValue("background-color"), > + 'rgb(${hexToRgb(BUTTON_BACKGROUND).join(", ")})', Did you try running this test? This isn't actually going to build the string you think it will since you need to be using a grave character (`) instead of a single quote character (') to bound the string when you are using JavaScript template strings. This is the error I got when I run your test locally: TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js | Toolbar button background is set. - "rgb(222, 222, 222)" == "rgb(${hexToRgb(BUTTON_BACKGROUND).join(\", \")})" - JS frame :: chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js :: test_button_background_properties :: line 38 Stack trace: chrome://mochitests/content/browser/toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js:test_button_background_properties:38 Note how you wanted "rgb(222, 222, 222)" but instead you ended up with "rgb(${hexToRgb(BUTTON_BACKGROUND).join(\", \")})".

Comment on attachment 8947996 [details] Bug 1431189 - Add google chrome toolbar button color properties: https://reviewboard.mozilla.org/r/217636/#review223424 ::: browser/themes/shared/toolbarbuttons.inc.css:26 (Diff revision 2) > > /* This default value of --toolbarbutton-height is defined to prevent > CSS errors for an invalid variable. The value should not get used, > as a more specific value should be set when the value will be used. */ > --toolbarbutton-height: 0; > -} > + } nit: undo this whitespace change.

Comment on attachment 8947996 [details] Bug 1431189 - Add google chrome toolbar button color properties: https://reviewboard.mozilla.org/r/217636/#review223430 Looks good! r+ from me with the one remaining change below to make. ::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js:47 (Diff revision 3) > + ); > + > + InspectorUtils.addPseudoClassLock(toolbarButton, ":hover"); > + > + Assert.equal( > + window.getComputedStyle(toolbarButtonIcon).getPropertyValue("background-color"), The return value from getComputedStyle() is a "live" value, meaning that it will update when the pseudo-classes are added to the element. This means that you don't have to re-call getComputedStyle each time. You can cache the result of the first call, and then call .getPropertyValue("background-color") each time that you add a pseudo-class.

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1687250b0dd9 Add google chrome toolbar button color properties: r=jaws

Sorry, this had to be backed out for eslint failures in toolkit/components/extensions/test/browser/browser_ext_themes_toolbarbutton_colors.js: https://hg.mozilla.org/integration/autoland/rev/f53bac4e3d94b4a954dd3d05b82bb0bb4cba0896 Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1687250b0dd9df82d5710e317d4bf488270da993&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=160165552&repo=autoland Please fix the issue and submit an updated patch. Thank you in advance.

Vivek, did your latest changes fix the failure (./mach eslint path/to/file) ? If not, does adding /* global InspectorUtils */ on top of the file fix it ?

I tested locally with his latest changes, it looks fine. Going to land this. Thanks Vivek!

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/749425381bba Add google chrome toolbar button color properties: r=jaws

Woohoo! Thanks, Vivek. This has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#February_2018 Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you.

Woohoooo! This is super exciting and yes, I would like to make an account on mozillians.org I went ahead and signed in using my google account and this is the link where you should be able to vouch on: https://mozillians.org/en-US/u/vivek3zero/ Thanks again!

(In reply to Vivek from comment #24) > Woohoooo! This is super exciting and yes, I would like to make an account on > mozillians.org > I went ahead and signed in using my google account and this is the link > where you should be able to vouch on: > > https://mozillians.org/en-US/u/vivek3zero/ > > Thanks again! \o/ You're vouched! Welcome onboard -- I hope to see you around!

I think https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme is updated with the properties added in this bug (minus the one that was later removed: button_background_active button_background_hover But let me know if we need anything else.

Looks good, I would just document how the properties are applied using a screenshot.

ntim: I decided in the end that we should have a separate screenshot for each of the `colors` items: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme#colors Let me know if this works for you.

(In reply to Will Bamberg [:wbamberg] from comment #28) > ntim: I decided in the end that we should have a separate screenshot for > each of the `colors` items: > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/ > theme#colors > > Let me know if this works for you. This is a big improvement, thanks! One thing though, right now, you have to read the code sample to know exactly what the property is doing, I think those two suggestions can improve this: * It would be nice to use the same base example all the time, and only change the relevant property. * it would be nice if the described item would always be in red in the examples (so the brain can connect the red color with the item). It's currently the case for most items, but some of them use a different color. The styled item should just be brought out visually without having to spend time reading the code sample :) Also, some very minor nits: * now that `headerURL` is optional, it would be nice to avoid putting it in the examples when it isn't relevant. * the `toolbar_top_separator` seems to screenshot a bug (the red separator being visible under the selected tab, because of the alpha channel).

(In reply to Tim Nguyen :ntim from comment #29) > (In reply to Will Bamberg [:wbamberg] from comment #28) > * It would be nice to use the same base example all the time, and only > change the relevant property. I don't think this is totally practical, as in some cases it would make the screenshots hard to decipher. But, I've tried to keep to a fairly consistent and simple set of properties. > * it would be nice if the described item would always be in red in the > examples (so the brain can connect the red color with the item). It's > currently the case for most items, but some of them use a different color. That's a good plan, and I've done this. > Also, some very minor nits: > * now that `headerURL` is optional, it would be nice to avoid putting it in > the examples when it isn't relevant. I've done this, except for `tab_selected`, which didn't seem to work without it. > * the `toolbar_top_separator` seems to screenshot a bug (the red separator > being visible under the selected tab, because of the alpha channel). This seems to have gone away in the new screenshot.

ntim, please let me know if you think this is good enough now, so I can mark this and the related bugs dev-doc-complete. Thanks!

(In reply to Will Bamberg [:wbamberg] from comment #31) > ntim, please let me know if you think this is good enough now, so I can mark > this and the related bugs dev-doc-complete. Thanks! Looks good, thanks! Just a small note, the tab_text property can be used independently from tab_selected.

> Just a small note, the tab_text property can be used independently from tab_selected. Sure. The only reason I used both in the example is just that otherwise the tab text is red on black->unreadable. Thanks for the reviews ntim!