1412591 - Implement Google Chrome frame_inactive property

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement, P5)

Description

[Tim Nguyen :ntim]

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.

Comment 1

[Tim Nguyen :ntim]

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

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

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 4

[Bogdan Pozderca [:bogdan]]

Attachment: Bug 1412591 - Implement Google Chrome frame_inactive property.

Review commit: https://reviewboard.mozilla.org/r/215128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215128/

Comment 5

[Code Review Bot [:reviewbot]]

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 6

[Bogdan Pozderca [:bogdan]]

Comment on attachment 8944976 [details]
Bug 1412591 - Implement Google Chrome frame_inactive property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215128/diff/1-2/

Comment 7

[Tim Nguyen :ntim]

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`

Comment 8

[Tim Nguyen :ntim]

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.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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

[Jared Wein [:jaws] (please needinfo? me)]

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 11

[Bogdan Pozderca [:bogdan]]

Comment on attachment 8944976 [details]
Bug 1412591 - Implement Google Chrome frame_inactive property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215128/diff/2-3/

Comment 12

[Tim Nguyen :ntim]

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 13

[Tim Nguyen :ntim]

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 14

[Tim Nguyen :ntim]

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 15

[Bogdan Pozderca [:bogdan]]

Comment on attachment 8944976 [details]
Bug 1412591 - Implement Google Chrome frame_inactive property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215128/diff/3-4/

Comment 16

[Bogdan Pozderca [:bogdan]]

Comment on attachment 8944976 [details]
Bug 1412591 - Implement Google Chrome frame_inactive property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215128/diff/4-5/

Comment 17

[Tim Nguyen :ntim]

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

Comment 18

[Bogdan Pozderca [:bogdan]]

Comment on attachment 8944976 [details]
Bug 1412591 - Implement Google Chrome frame_inactive property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215128/diff/5-6/

Comment 19

[Tim Nguyen :ntim]

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

Comment 20

[Dão Gottwald [:dao]]

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 21

[Bogdan Pozderca [:bogdan]]

Comment on attachment 8944976 [details]
Bug 1412591 - Implement Google Chrome frame_inactive property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215128/diff/6-7/

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8944976 [details]
Bug 1412591 - Implement Google Chrome frame_inactive property.

https://reviewboard.mozilla.org/r/215128/#review222980

Looks good!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/902bc8e0b638
Implement Google Chrome frame_inactive property. r=jaws

Comment 24

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/902bc8e0b638

Comment 25

[Caitlin Neiman [:caitmuenster]]

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.

Comment 26

[Bogdan Pozderca [:bogdan]]

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

:)

Comment 27

[marius.santa]

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.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

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