1309908 - The theme API should be able to support the Lightweight Themes feature set

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Mike de Boer [:mikedeboer]]

We'll be using 'images.theme_frame' for the header image, 'colors.frame' for the background color and 'colors.tab_text' for the text color.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1309908 - support the Lightweight Themes feature set in the themes API.

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

Comment 2

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

What do you think about also supporting the lwtheme equivalents as an alias?

Comment 3

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

Comment on attachment 8800715 [details]
Bug 1309908 - support the Lightweight Themes feature set in the themes API.

https://reviewboard.mozilla.org/r/85584/#review84254

Can you choose a much smaller image for the test? Having a large data URI in the file will just make the test harder to read and take up a bunch of storage.

::: browser/components/extensions/ext-theme.js:32
(Diff revision 1)
> +      let cssURL = 'url("' + manifest.theme.images[image].replace(/"/g, '\\"') + '")'
>        if (image == "theme_ntp_background") {
> -        temp.style.background = manifest.theme.images.theme_ntp_background;
> +        temp.style.background = cssURL;

We shouldn't do this as it will break the ability to set multiple background images, background-position, background-color, etc.

The property previously was left wide open so the full background shorthand could be used.

Note that this may be incompatible with the Chrome API. If that is why you changed this, then we should write some code that looks if the property was just a URL and wrap it if so, otherwise leave it unchanged.

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> What do you think about also supporting the lwtheme equivalents as an alias?

Sure, that's easy.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Comment on attachment 8800715 [details]
> Bug 1309908 - support the Lightweight Themes feature set in the themes API.
> 
> https://reviewboard.mozilla.org/r/85584/#review84254
> 
> Can you choose a much smaller image for the test? Having a large data URI in
> the file will just make the test harder to read and take up a bunch of
> storage.

Sure :) I just thought it was funny.

> We shouldn't do this as it will break the ability to set multiple background
> images, background-position, background-color, etc.

I think you're thinking more CSS-y, less API-y :) What I mean is that we'll probably want to dissect background color/ image/ position into separate properties altogether, even though CSS offers shorthand props.

> The property previously was left wide open so the full background shorthand
> could be used.
> 
> Note that this may be incompatible with the Chrome API. If that is why you
> changed this, then we should write some code that looks if the property was
> just a URL and wrap it if so, otherwise leave it unchanged.

Well, this initially. But now I also think that it's the right way to go. Let's leave the CSS out as 'implementation detail'. What do you think?

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8800715 [details]
Bug 1309908 - support the Lightweight Themes feature set in the themes API.

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

Comment 6

[Matthew Wein [:mattw]]

Comment on attachment 8800715 [details]
Bug 1309908 - support the Lightweight Themes feature set in the themes API.

https://reviewboard.mozilla.org/r/85584/#review84448

I just have some nits, but otherwise LGTM.

::: browser/components/extensions/ext-theme.js:22
(Diff revision 1)
>    this.loadThemeFromManifest(manifest);
>  }
>  
>  Theme.prototype = {
>    loadThemeFromManifest(manifest) {
>      if (!manifest || !manifest.theme || !manifest.theme.images) {

I don't think we should require the `images` property to be set for the entire function. We should check only to see if the `images` and `colors` properties are set right before we process them.

Also, please remove the checks for `manifest` and `manifest.theme` here because `extensions.on("manifest_theme", ...)` will only ever get called if both exist.

::: browser/components/extensions/ext-theme.js:43
(Diff revision 1)
> +      }
> -      }
> +    }
> +    for (let color of Object.getOwnPropertyNames(manifest.theme.colors || {})) {
> +      let val = manifest.theme.colors[color];
> +      let cssColor = "rgb" + (val.length > 3 ? "a" : "") + "(" + val.join(",") + ")";
> +      if (color == "frame")

nit: Please wrap all of the single line if statements in curly braces.

::: browser/components/extensions/ext-theme.js:45
(Diff revision 1)
> +    for (let color of Object.getOwnPropertyNames(manifest.theme.colors || {})) {
> +      let val = manifest.theme.colors[color];
> +      let cssColor = "rgb" + (val.length > 3 ? "a" : "") + "(" + val.join(",") + ")";
> +      if (color == "frame")
> +        LWTStyles.accentcolor = cssColor;
> +      if (color == "tab_text")

Are we following https://developer.chrome.com/extensions/themes for this? I'm just curious because I don't see `tab_text` as a supported property there.

Comment 7

[Matthew Wein [:mattw]]

Comment on attachment 8800715 [details]
Bug 1309908 - support the Lightweight Themes feature set in the themes API.

https://reviewboard.mozilla.org/r/85584/#review84462

Comment 8

[Mike de Boer [:mikedeboer]]

Thanks Matthew!

The Google themes page is not complete in the properties it lists, so I got mine from https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/themes/theme_properties.h

I've started to map all the properties supported by Chrome to Fx CSS variable names here:

https://docs.google.com/a/mozilla.com/spreadsheets/d/1YScpOVL5WdNDhQY2Nngh4YkK0bOpkfwJvpRjpMSxMWo/edit?usp=sharing

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8800715 [details]
Bug 1309908 - support the Lightweight Themes feature set in the themes API.

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

Comment 10

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/projects/cedar/rev/8bdb50d9b1fd1a267f499693a3e9e0e44608b0aa
Bug 1309908 - support the Lightweight Themes feature set in the themes API. r=k-9

Comment 11

[Mike de Boer [:mikedeboer]]

Changes in this commit are:

1. Support for the LWT property names, along with Chrome equivalents.
2. Added a test for this.
3. Applied Matthews' comments.