https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/themes/theme_properties.h#28 COLOR_FRAME_INACTIVE, COLOR_FRAME_INCOGNITO, COLOR_FRAME_INCOGNITO_INACTIVE, TINT_FRAME, TINT_FRAME_INACTIVE, TINT_FRAME_INCOGNITO, TINT_FRAME_INCOGNITO_INACTIVE, I'm not sure exactly how the incognito ones will work with dynamic per-window themes. It introduces lots of complications. I think it would be better to limit the incognito ones to static themes for chrome compat.

The :-moz-window-inactive CSS selector allows targeting inactive windows. The :root[privatebrowsingmode="temporary"] CSS selector allows targeting private windows.

Note that Chrome has colours for unselected background tabs, whereas in Firefox, our unselected background tabs are currently transparent. This means that changing the frame background also will change the background of unselected tabs, which means we might need to change the font of unselected background tabs depending on the frame background colour.

mconley, dao, and myself looked further at this bug and have limited the scope of it to not include styling incognito windows through the static API. Supporting this through the static API would require us to include variants for all of the other colors within the schema since colors for a non-private window may not make sense on the private window background color. Styling private windows using the dynamic API is already possible and allows the theme author to supply all colors specific to private windows at the same time. We will try to implement support for COLOR_FRAME_INACTIVE.

Comment on attachment 8944976 [details] Bug 1412591 - Implement Google Chrome frame_inactive property. https://reviewboard.mozilla.org/r/215128/#review220732 Static analysis found 1 defect in this patch. - 1 defect 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/test/browser/browser_ext_themes_alpha_accentcolor.js:43 (Diff revision 1) > + "headerURL": "image1.png", > + }, > + "colors": { > + "accentcolor": "rgba(230, 128, 0, 0.1)", > + "accentcolor_inactive": ACCENTCOLOR_INACTIVE, > + "textcolor": TEXT_COLOR Error: Missing trailing comma. [eslint: comma-dangle]

Comment on attachment 8944976 [details] Bug 1412591 - Implement Google Chrome frame_inactive property. https://reviewboard.mozilla.org/r/215128/#review220798 ::: toolkit/components/extensions/test/browser/browser_ext_themes_alpha_accentcolor.js:32 (Diff revision 2) > "Window background color should be opaque"); > > await extension.unload(); > }); > + > +add_task(async function test_accentcolor_inactive_property() { If you're adding this test in browser_ext_themes_alpha_accentcolor.js (which is absolutely fine by me), please rename the test file from "browser_ext_themes_alpha_accentcolor.js" to "browser_ext_themes_accentcolors.js" and make the corresponding change in browser.ini. You can use `hg mv path/to/file path/to/newfilename`

Jared, should we support accentcolor_inactive as this patch does ? I don't think we want to, as accentcolor is only here for lightweight theme compatibility.

(In reply to Tim Nguyen :ntim from comment #8) > Jared, should we support accentcolor_inactive as this patch does ? I don't > think we want to, as accentcolor is only here for lightweight theme > compatibility. I agree, accentcolor and frame/frame_inactive are only here for backwards-compat. Let's not introduce new properties here that aren't for compat reasons.

Comment on attachment 8944976 [details] Bug 1412591 - Implement Google Chrome frame_inactive property. https://reviewboard.mozilla.org/r/215128/#review221128 Looks good, you're on the right path. Please make the following changes below and note comment 8 and comment 9 give some background to my comments below. ::: toolkit/components/extensions/ext-theme.js:132 (Diff revision 2) > switch (color) { > case "accentcolor": > case "frame": > this.lwtStyles.accentcolor = cssColor; > break; > + case "accentcolor_inactive": This line should be removed. ::: toolkit/components/extensions/schemas/theme.json:76 (Diff revision 2) > + "accentcolor_inactive": { > + "$ref": "ThemeColor", > + "optional": true > + }, Please remove this. ::: toolkit/components/extensions/test/browser/browser_ext_themes_alpha_accentcolor.js:33 (Diff revision 2) > > await extension.unload(); > }); > + > +add_task(async function test_accentcolor_inactive_property() { > + const ACCENTCOLOR_INACTIVE = "rgb(255, 0, 0)"; FRAMECOLOR_INACTIVE ::: toolkit/components/extensions/test/browser/browser_ext_themes_alpha_accentcolor.js:42 (Diff revision 2) > + "images": { > + "headerURL": "image1.png", > + }, > + "colors": { > + "accentcolor": "rgba(230, 128, 0, 0.1)", > + "accentcolor_inactive": ACCENTCOLOR_INACTIVE, This should be frame_inactive

Comment on attachment 8944976 [details] Bug 1412591 - Implement Google Chrome frame_inactive property. https://reviewboard.mozilla.org/r/215128/#review222592 ::: browser/base/content/browser.css:38 (Diff revision 3) > +#main-window:-moz-window-inactive:-moz-lwtheme { > + background-color: var(--lwt-accent-color-inactive, var(--lwt-accent-color)) !important; > +} Can you move this after the second `:root:-moz-lwtheme {` rule ? ::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:118 (Diff revision 3) > + let style = window.getComputedStyle(docEl); > + > + Assert.equal(style.backgroundColor, FRAME_COLOR, > + "Window background color should be opaque"); > + > + // Now we'll open a new window to see if the inactive browser accent color changed This comment probably needs updating. ::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:122 (Diff revision 3) > + > + // Now we'll open a new window to see if the inactive browser accent color changed > + let window2 = await BrowserTestUtils.openNewBrowserWindow(); > + > + Assert.equal(style.backgroundColor, FRAME_COLOR, > + `Inactive window background color should be ${FRAME_COLOR}`); Can you be more explicit in the message ? "Inactive window background should not change if colors.frame_inactive isn't set"

Comment on attachment 8944976 [details] Bug 1412591 - Implement Google Chrome frame_inactive property. https://reviewboard.mozilla.org/r/215128/#review222594 ::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:50 (Diff revision 3) > +add_task(async function test_support_theme_frame_inactive() { > + const FRAME_COLOR = "rgb(230, 128, 0)"; > + const FRAME_COLOR_INACTIVE = "rgb(255, 0, 0)"; Just a detail: You chose the browser_ext_themes_chromeparity.js file (which seems appropriate to me), but rgb() color strings are only supported in Firefox. Chrome supports arrays: [230, 128, 0]. Also, headerURL/textcolor/accentcolor are Firefox names, so you might want to change them to theme_frame/background_tab_text/frame respectively.

Comment on attachment 8944976 [details] Bug 1412591 - Implement Google Chrome frame_inactive property. https://reviewboard.mozilla.org/r/215128/#review222602 ::: commit-message-b0baa:1 (Diff revision 3) > +Bug 1412591 - Implement Google chrome frame properties. r?jaws Another detail :) The commit message should probably be: Bug 1412591 - Implement Google Chrome frame_inactive property. r?jaws

Comment on attachment 8944976 [details] Bug 1412591 - Implement Google Chrome frame_inactive property. https://reviewboard.mozilla.org/r/215128/#review222612 ::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:78 (Diff revision 4) > + > + let docEl = window.document.documentElement; > + let style = window.getComputedStyle(docEl); > + > + Assert.equal(style.backgroundColor, "rgb(" + FRAME_COLOR.join(", ") + ")", > + "Window background color should be opaque"); I know this message was copied over from another test, but it's not really relevant here. In this test, it should probably be "Window background is set to the colors.frame property" ::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:118 (Diff revision 4) > + > + let docEl = window.document.documentElement; > + let style = window.getComputedStyle(docEl); > + > + Assert.equal(style.backgroundColor, "rgb(" + FRAME_COLOR.join(", ") + ")", > + "Window background color should be opaque"); Same here. ::: toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js:124 (Diff revision 4) > + > + // Now we'll open a new window to make sure the inactive browser accent color stayed the same > + let window2 = await BrowserTestUtils.openNewBrowserWindow(); > + > + Assert.equal(style.backgroundColor, "rgb(" + FRAME_COLOR.join(", ") + ")", > + `Inactive window background should not change if colors.frame_inactive isn't set`); Use a normal double quoted string here, not a template string, since there's no ${...} inside the string

Triggered a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=72c0505b84f6

Comment on attachment 8944976 [details] Bug 1412591 - Implement Google Chrome frame_inactive property. https://reviewboard.mozilla.org/r/215128/#review220822 ::: browser/base/content/browser.css:26 (Diff revision 6) > background-image: var(--lwt-header-image), var(--lwt-additional-images) !important; > background-position: var(--lwt-background-alignment) !important; > background-repeat: var(--lwt-background-tiling) !important; > } > > +#main-window:-moz-window-inactive:-moz-lwtheme { :root:-moz-lwtheme:-moz-window-inactive

Comment on attachment 8944976 [details] Bug 1412591 - Implement Google Chrome frame_inactive property. https://reviewboard.mozilla.org/r/215128/#review222980 Looks good!

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/902bc8e0b638 Implement Google Chrome frame_inactive property. r=jaws

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

That's awesome! Definitely, I've created an account: https://mozillians.org/en-US/u/bogdanp/ :)

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

This patch included automated tests so we shouldn't need manual testing here. Thanks!