Implement Google Chrome frame_inactive property

RESOLVED FIXED in Firefox 60

Status

P5
enhancement
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: ntim, Assigned: bogdan, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla60
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 wontfix, firefox60 fixed)

Details

(Whiteboard: [theming])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.

Updated

a year ago
Severity: normal → enhancement
status-firefox57: --- → wontfix
Priority: -- → P5
Whiteboard: [theming]
(Reporter)

Comment 1

a year ago
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 hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 7

a year ago
mozreview-review
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`
(Reporter)

Comment 8

a year ago
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.
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
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
Attachment #8944976 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 12

a year ago
mozreview-review
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"
(Reporter)

Comment 13

a year ago
mozreview-review
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.
(Reporter)

Comment 14

a year ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 17

a year ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 19

a year ago
Triggered a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=72c0505b84f6
Summary: Implement Google chrome frame properties → Implement Google Chrome frame_inactive property

Comment 20

a year ago
mozreview-review
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 hidden (mozreview-request)

Comment 23

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

Comment 24

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/902bc8e0b638
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.
(Assignee)

Comment 26

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

:)

Comment 27

a year ago
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.
Flags: needinfo?(bogdan)
This patch included automated tests so we shouldn't need manual testing here. Thanks!
Flags: needinfo?(bogdan)

Updated

a year ago
Flags: qe-verify-

Updated

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