Closed Bug 1309908 Opened 7 years ago Closed 7 years ago

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

Categories

(WebExtensions :: General, defect)

Product:
WebExtensions
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

Bug 1309908 - support the Lightweight Themes feature set in the themes API.
58 bytes, text/x-review-board-request
mattw
: review+
Details 
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.
What do you think about also supporting the lwtheme equivalents as an alias?
Flags: needinfo?(mdeboer)
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.
(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?
Flags: needinfo?(mdeboer)
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.
Attachment #8800715 - Flags: review?(mwein) → review+
Comment on attachment 8800715 [details]
Bug 1309908 - support the Lightweight Themes feature set in the themes API.

https://reviewboard.mozilla.org/r/85584/#review84462
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
https://hg.mozilla.org/projects/cedar/rev/8bdb50d9b1fd1a267f499693a3e9e0e44608b0aa
Bug 1309908 - support the Lightweight Themes feature set in the themes API. r=k-9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.