1319821 - Themes should allow for multiple background images in the toolbar

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

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

One of the largest requests that we got from lwtheme authors was that they disliked how lwthemes required a 3000px wide image that was docked to the top right of the browser.

This made it impossible to create a theme that had an image in the top-left and top-right corner of the browser, as well as a single image that was positioned in the center of the browser.

The theme API should allow for multiple images as well as unique background-positions and background-size.

Comment 1

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

Attachment: Bug 1319821 - [WIP] Themes should allow for multiple background images in the toolbar.

The current patch also sets the background images on the tab shapes and I'm not sure if that is what we want. We would also want to forward background-position there too.

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

Comment 2

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

Passing this off to mikedeboer for now. I think the current patch might work if we set all of the additional background images as background-attachment: fixed;

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1319821 - Themes should allow for multiple background images in the toolbar.

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

Comment 4

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb511f9017e3

Comment 5

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

Comment on attachment 8848128 [details]
Bug 1319821 - Themes should allow for multiple background images in the toolbar.

https://reviewboard.mozilla.org/r/121108/#review123094

::: toolkit/components/extensions/ext-theme.js:144
(Diff revision 1)
>        this.lwtStyles.icons[variableName] = resolvedURL;
>      }
>    }
>  
>    /**
> +   * Helper method for preparing properties found the extension's manifest.

s/found the/found in the/

::: toolkit/components/extensions/schemas/theme.json:201
(Diff revision 1)
> +                  "enum": [
> +                    "bottom", "center", "left", "right", "top",

Nice!

::: toolkit/components/extensions/test/browser/browser_ext_themes_multiple_backgrounds.js:18
(Diff revision 1)
> +          "frame": FRAME_COLOR,
> +          "tab_text": TAB_TEXT_COLOR,

Can you change this specific test to use accentcolor and textcolor to make sure we're covering both aliases?

::: toolkit/components/extensions/test/browser/browser_ext_themes_multiple_backgrounds.js:65
(Diff revision 1)
> +add_task(function* test_support_backgrounds_repeat() {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "theme": {
> +        "images": {
> +          "headerURL": "face.png",

Can you change this specific test to use theme_frame instead of headerURL so that we cover both aliases?

Comment 6

[Matthew Wein [:mattw]]

Comment on attachment 8848128 [details]
Bug 1319821 - Themes should allow for multiple background images in the toolbar.

https://reviewboard.mozilla.org/r/121108/#review123544

Looks good! Just a few things and a test I think would be good to add.

::: toolkit/components/extensions/ext-theme.js:160
(Diff revision 1)
> +        continue;
> +      }
> +
> +      switch (property) {
> +        case "additional_backgrounds_alignment": {
> +          if (!this.lwtStyles.additionalBackgrounds) {

Since these arrays are connected to `additionalBackgrounds`, I think we should throw an error or at least provide a warning if the length of `backgroundsAlignment` or `backgroundsTiling` differ from the length of `additionalBackgrounds`.

::: toolkit/components/extensions/test/browser/browser_ext_themes_dynamic_updates.js:39
(Diff revision 1)
>  add_task(function* setup() {
>    yield SpecialPowers.pushPrefEnv({
>      set: [["extensions.webextensions.themes.enabled", true]],
>    });
>  });
>  

Could you add a test to make sure this functionality works with browser.themes.update?

::: toolkit/components/extensions/test/browser/browser_ext_themes_multiple_backgrounds.js:66
(Diff revision 1)
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "theme": {
> +        "images": {
> +          "headerURL": "face.png",
> +          "additional_backgrounds": ["face.png", "face.png", "face.png"],

It might be good to use different names so we can confirm the correct ordering.

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Matthew Wein [:mattw] from comment #6)
> Since these arrays are connected to `additionalBackgrounds`, I think we
> should throw an error or at least provide a warning if the length of
> `backgroundsAlignment` or `backgroundsTiling` differ from the length of
> `additionalBackgrounds`.

Good point. I'll add a warning here.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8848128 [details]
Bug 1319821 - Themes should allow for multiple background images in the toolbar.

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

Comment 9

[Mike de Boer [:mikedeboer]]

I'd like to test the console output as well, but IIRC the console watcher is non-e10s only, right? I don't want to restrict this test to run non-e10s only.
I can also add a test that makes sure the properties section is not applied when no additional background images are defined, because we should bail out when that happens.

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8848128 [details]
Bug 1319821 - Themes should allow for multiple background images in the toolbar.

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

Comment 11

[Mike de Boer [:mikedeboer]]

Matt, even though you're on a work week, can you find the time to review this patch? Thanks!

Comment 12

[Matthew Wein [:mattw]]

Yep, will do!

Comment 13

[Matthew Wein [:mattw]]

Comment on attachment 8848128 [details]
Bug 1319821 - Themes should allow for multiple background images in the toolbar.

https://reviewboard.mozilla.org/r/121108/#review127328

Nice!

::: toolkit/components/extensions/schemas/theme.json:28
(Diff revision 3)
>              "type": "object",
>              "optional": true,
>              "properties": {
> +              "additional_backgrounds": {
> +                "type": "array",
> +                "items": { "type": "string" },

Should this type be `ExtensionURL`? Same for headerURL and theme_frame?

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8848128 [details]
Bug 1319821 - Themes should allow for multiple background images in the toolbar.

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d376d7031df
Themes should allow for multiple background images in the toolbar. r=jaws,mattw

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this out in https://hg.mozilla.org/integration/autoland/rev/0bec91c2c87ea9553d96caaa830038879a23a04d for being the likely cause of failures like https://treeherder.mozilla.org/logviewer.html#?job_id=87665309&repo=autoland

Comment 17

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d564761f799

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8848128 [details]
Bug 1319821 - Themes should allow for multiple background images in the toolbar.

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f621d23347cf
Themes should allow for multiple background images in the toolbar. r=jaws,mattw

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f621d23347cf

Comment 21

[Richard Marti (:Paenglab)]

On https://hg.mozilla.org/mozilla-central/file/f621d23347cf/browser/base/content/browser.css#l448 the background-position and background-repeat are still defined. Shouldn't they be removed as they are now defined in the variables?

PS: And I get this in the console: ReferenceError: reference to undefined property "additionalBackgrounds"  LightweightThemeConsumer.jsm:137:5

Comment 22

[Mike de Boer [:mikedeboer]]

(In reply to Richard Marti (:Paenglab) from comment #21)
> On
> https://hg.mozilla.org/mozilla-central/file/f621d23347cf/browser/base/
> content/browser.css#l448 the background-position and background-repeat are
> still defined. Shouldn't they be removed as they are now defined in the
> variables?

Yup, they can be removed at this point. Thanks for mentioning that!

> PS: And I get this in the console: ReferenceError: reference to undefined
> property "additionalBackgrounds"  LightweightThemeConsumer.jsm:137:5

Hmm, that's correct, because aData.additionalBackgrounds is usually undefined. So this is a warning we can fix by explicitly checking for the key's existence, or add ` || null` to each of these statements. I'm not 100% sure if the latter will obsolete the warning, though.