1443561 - WebExtension themes additional backgrounds alignment should be relative to the toolbox

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Tim Nguyen :ntim]

:eviljeff pointed out on IRC that bottom aligned images didn't appear. This is because the alignment is relative to the browser window as opposed to the toolbox, which means that the alignment on the y-axis will be wrong.

Comment 1

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

For compatibility purposes we should continue to set headerURL/theme_frame on the :root, but for additional_backgrounds we should set it on #navigator-toolbox. This *will* cause additional_backgrounds to appear above the :root background.

I'm proposing this because on some platforms there is a gap between #navigator-toolbox and the top of the window.

Comment 2

[Andrew Williamson [:eviljeff]]

> This *will* cause additional_backgrounds to appear above the :root background.

The headerURL/theme_frame will be rendered *under* the additional backgrounds?  If so that sounds like a big compatibility break with existing versions.

Comment 3

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

If we made the change proposed in comment 1, yes that would end up happening. It's not ideal, but we should see how many themes are using additional_backgrounds to determine if the change in behavior will actually affect enough themes to make it an issue.

@andym, are you able to see how many webextension themes on AMO are using additional_backgrounds?

Comment 4

[Andrew Williamson [:eviljeff]]

AMO doesn't currently allow static webextension theme uploads (only dynamic themes), so the answer is 0.

However, when we *do* enable uploads in Q2 they will be available for all versions of Firefox that support webextension themes, so if, for example, a change is made in 61 to change the order of headerURL & additional backgrounds then users on 60 will get a different theme than users on 61.

Comment 5

[Andrew Williamson [:eviljeff]]

"a different theme" == the same theme but it will render potentially differently depending on the content of headerURL.

Comment 6

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

(In reply to Andrew Williamson [:eviljeff] from comment #4)
> AMO doesn't currently allow static webextension theme uploads (only dynamic
> themes), so the answer is 0.

This is not true. Themes can be uploaded as "dynamic themes" to AMO and they will use the JavaScript API to set the theme. See for example https://addons.mozilla.org/en-US/firefox/addon/quantum-lights-dynamic/

Comment 7

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

To add some more clarify, dynamic themes have been able to use the "additional_backgrounds" API so I'm not sure why we would conclude this as 0 themes using it.

Comment 8

[Andrew Williamson [:eviljeff]]

I was talking about static themes specifically but yeah, sure, dynamic themes can use it too - it's not exposed in the manifest though so AMO don't store it in the database and doesn't know about it.  Maybe jorgev can do some trawling to find out which add-ons use it.

Comment 9

[Jorge Villalobos [:jorgev] (he/him)]

There's an up to date query here: https://sql.telemetry.mozilla.org/queries/51288. We don't have a good tool to search for add-on code currently, so going over the whole (short) list is the only option.

Comment 10

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

Thanks for the query Jorge. I went through the list and I was only able to find two add-ons that are using additional_backgrounds:
1. https://addons.mozilla.org/en-US/firefox/addon/quantum-lights-dynamic/
2. https://addons.mozilla.org/en-US/firefox/addon/gradientus/

We could make this change and test those two themes to see how this will affect them, as well as reach out to those authors to let them know that their theme will be affected.

Comment 11

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

Flagging needinfo from the two add-on authors.

Comment 12

[Tim Nguyen :ntim]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> Thanks for the query Jorge. I went through the list and I was only able to
> find two add-ons that are using additional_backgrounds:
> 1. https://addons.mozilla.org/en-US/firefox/addon/quantum-lights-dynamic/
> 2. https://addons.mozilla.org/en-US/firefox/addon/gradientus/
> 
> We could make this change and test those two themes to see how this will
> affect them, as well as reach out to those authors to let them know that
> their theme will be affected.

Both those themes shouldn't be affected, they use a transparent headerURL image, and they only use top alignments.

Comment 13

[Cynthia Pereira [:cynthiapereira] (needinfo me)]

Hi there! Thanks for letting me know. What :ntim said is correct. So, from my side, it will not be a problem and I always can adapt my extension, if necessary.

Comment 14

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

Vivek, can you work on this?

Comment 15

[Vivek]

Yes I can get started right away

Comment 16

[Vivek]

Attachment: Bug 1443561 - WebExtension themes additional backgrounds alignment should be relative to the toolbox

This patch changes alignment property of additional backgrounds to be relative to #navigator-toolbox, instead of root.

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

Comment 17

[Vivek]

Comment on attachment 8967298 [details]
Bug 1443561 - WebExtension themes additional backgrounds alignment should be relative to the toolbox

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

Comment 18

[Vivek]

Comment on attachment 8967298 [details]
Bug 1443561 - WebExtension themes additional backgrounds alignment should be relative to the toolbox

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

Comment 19

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

Comment on attachment 8967298 [details]
Bug 1443561 - WebExtension themes additional backgrounds alignment should be relative to the toolbox

https://reviewboard.mozilla.org/r/235980/#review241856

This is very close and should be done once you fix the issues below.

::: browser/base/content/browser.css:24
(Diff revision 3)
> -  background-repeat: var(--lwt-background-tiling) !important;
>  }
>  
>  :root:-moz-lwtheme[lwtheme-image] {
> -  background-image: var(--lwt-header-image), var(--lwt-additional-images) !important;
> +  background-image: var(--lwt-header-image) !important;
>  }

Please add `background-position: right top !important;` here

::: toolkit/components/extensions/test/browser/browser_ext_themes_additional_backgrounds_alignment.js:5
(Diff revision 3)
> +// Case 1 - When there is a headerURL image and additional backgrounds property is not specified.
> +// So additional backgrounds alignment should default to "left top"

Replace "additional backgrounds property" with "additional_backgrounds_alignment" and replace "additional backgrounds alignment" with "background-position"

::: toolkit/components/extensions/test/browser/browser_ext_themes_additional_backgrounds_alignment.js:33
(Diff revision 3)
> +  let style = window.getComputedStyle(docEl);
> +
> +  Assert.equal(
> +    style.getPropertyValue("background-position"),

Please rename `style` to `rootCS`.

::: toolkit/components/extensions/test/browser/browser_ext_themes_additional_backgrounds_alignment.js:86
(Diff revision 3)
> +  let style = window.getComputedStyle(docEl);
> +
> +  Assert.equal(
> +    style.getPropertyValue("background-position"),

Same rename here too

::: toolkit/components/extensions/test/browser/browser_ext_themes_multiple_backgrounds.js:27
(Diff revision 3)
>            "additional_backgrounds_alignment": ["left top", "center top", "right bottom"],
>          },
>        },
>      },
>      files: {
>        "face.png": imageBufferFromDataURI(ENCODED_IMAGE_DATA),

Please rename this to "face1.png" and add another line and make a "face2.png". Then use "face1.png" for  headerURL and face2.png for additional_backgrounds.

::: toolkit/components/extensions/test/browser/browser_ext_themes_multiple_backgrounds.js:41
(Diff revision 3)
>    Assert.ok(docEl.hasAttribute("lwtheme"), "LWT attribute should be set");
>    Assert.equal(docEl.getAttribute("lwthemetextcolor"), "bright",
>                 "LWT text color attribute should be set");
>  
> -  let style = window.getComputedStyle(docEl);
> -  let bgImage = style.backgroundImage.split(",")[0].trim();
> +  let toolboxCS = window.getComputedStyle(toolbox);
> +  let rootBgImage = toolboxCS.backgroundImage.split(",")[0].trim();

This is not getting the root background image. It's getting the background image from the toolbox since it's using toolboxCS.

The test is passing because the headerURL and additional_backgrounds all use "face.png".

::: toolkit/components/extensions/test/browser/browser_ext_themes_multiple_backgrounds.js:45
(Diff revision 3)
> -  let style = window.getComputedStyle(docEl);
> -  let bgImage = style.backgroundImage.split(",")[0].trim();
> -  Assert.ok(bgImage.includes("face.png"),
> +  let toolboxCS = window.getComputedStyle(toolbox);
> +  let rootBgImage = toolboxCS.backgroundImage.split(",")[0].trim();
> +  let bgImage = toolboxCS.backgroundImage.split(",")[0].trim();
> +  Assert.ok(rootBgImage.includes("face.png"),
>              `The backgroundImage should use face.png. Actual value is: ${bgImage}`);
> -  Assert.equal(Array(4).fill(bgImage).join(", "), style.backgroundImage,
> +  Assert.equal(Array(3).fill(bgImage).join(", "), toolboxCS.backgroundImage,

Can you swap these two arguments? The first argument to Assert.equal() is the "actual" and the second argument is the "expected".

::: toolkit/components/extensions/test/browser/browser_ext_themes_multiple_backgrounds.js:55
(Diff revision 3)
> -  style = window.getComputedStyle(docEl);
> +  toolboxCS = window.getComputedStyle(docEl);
>    // Styles should've reverted to their initial values.
> -  Assert.equal(style.backgroundImage, "none");
> -  Assert.equal(style.backgroundPosition, "0% 0%");
> -  Assert.equal(style.backgroundRepeat, "repeat");
> +  Assert.equal(toolboxCS.backgroundImage, "none");
> +  Assert.equal(toolboxCS.backgroundPosition, "0% 0%");
> +  Assert.equal(toolboxCS.backgroundRepeat, "repeat");

`toolboxCS` is the wrong name since this is the computed styles of the root element. `rootCS` is the proper name.

Comment 20

[Vivek]

Comment on attachment 8967298 [details]
Bug 1443561 - WebExtension themes additional backgrounds alignment should be relative to the toolbox

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

Comment 21

[Vivek]

Comment on attachment 8967298 [details]
Bug 1443561 - WebExtension themes additional backgrounds alignment should be relative to the toolbox

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

Comment 22

[Vivek]

Comment on attachment 8967298 [details]
Bug 1443561 - WebExtension themes additional backgrounds alignment should be relative to the toolbox

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

Comment 23

[Vivek]

Comment on attachment 8967298 [details]
Bug 1443561 - WebExtension themes additional backgrounds alignment should be relative to the toolbox

https://reviewboard.mozilla.org/r/235980/#review241856

> `toolboxCS` is the wrong name since this is the computed styles of the root element. `rootCS` is the proper name.

I've fixed this and also added more asserts to test if both rootCS and toolboxCS revert back to their intial values on unloading an extension. Previously only rootCS was checked since additional_backgrounds_alignment was applied in root but is now applied in toolbox.

Comment 24

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

Comment on attachment 8967298 [details]
Bug 1443561 - WebExtension themes additional backgrounds alignment should be relative to the toolbox

https://reviewboard.mozilla.org/r/235980/#review242024

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3e01de682f1
WebExtension themes additional backgrounds alignment should be relative to the toolbox r=jaws

Comment 26

[Arthur Iakab [arthur_iakab]]

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

Comment 27

[Andreea Pavel [:apavel]]

Backed out on request: https://hg.mozilla.org/mozilla-central/rev/ceac91dc08bef5d099c10dda632fc3651b23c897

Comment 28

[Vivek]

Patch did not work on OSX(refer bug 1454016). Better fix needs to be written to work along with https://searchfox.org/mozilla-central/source/toolkit/themes/osx/global/toolbar.css#10.

Comment 29

[Andrew Williamson [:eviljeff]]

p5?  Alignment for additional backgrounds is horribly broken.  If we don't want to fix it then at least "additional_backgrounds_alignment" should be dropped as a supported property.

Comment 30

[Mike Conca [:mconca]]

Since static themes are going live on AMO very soon, this issue is going to gain in visibility.  It would be good to fix this before a lot of static theme authors (independent authors, or via the AMO theme wizard) run into it.  Dao, can we raise this up in priority?

See also:  https://github.com/mozilla/addons-server/issues/8038

Comment 31

[David Durst [:ddurst]]

This is now blocking AMO's release of static theme support.

Comment 32

[David Durst [:ddurst]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> For compatibility purposes we should continue to set headerURL/theme_frame
> on the :root, but for additional_backgrounds we should set it on
> #navigator-toolbox. This *will* cause additional_backgrounds to appear above
> the :root background.
> 
> I'm proposing this because on some platforms there is a gap between
> #navigator-toolbox and the top of the window.

FWIW, I just (perhaps clumsily) checked Nightly 63 (OSX) and Release 61 (Win10) and don't see a gap if I manually disable the lwt-theme and apply the same image as a background on #navigator-toolbox.

If we set additional_backgrounds *and* headerURL relative to #navigator-toolbox, we could preserve the currently documented layering order (not that there seems to be any intention behind that that I know of[1]).

jaws, is there something known that this would break? Because that seems like the easy answer to this problem and it would preserve theme previews, fix additional_backgrounds' visibility, etc, while keeping compatibility between Chrome and Firefox for headerURL.


[1] This is not to say that I'm supporting the current ordering. It's a Product call, and I'm not sure one is inherently better than another, one is just current.

Comment 33

[Tim Nguyen :ntim]

(In reply to David Durst [:ddurst] (REO for 63) from comment #32)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> > For compatibility purposes we should continue to set headerURL/theme_frame
> > on the :root, but for additional_backgrounds we should set it on
> > #navigator-toolbox. This *will* cause additional_backgrounds to appear above
> > the :root background.
> > 
> > I'm proposing this because on some platforms there is a gap between
> > #navigator-toolbox and the top of the window.
> 
> FWIW, I just (perhaps clumsily) checked Nightly 63 (OSX) and Release 61
> (Win10) and don't see a gap if I manually disable the lwt-theme and apply
> the same image as a background on #navigator-toolbox.

We were more thinking about platforms like Windows 7/8 and Linux, which still use a native titlebar.

> If we set additional_backgrounds *and* headerURL relative to
> #navigator-toolbox, we could preserve the currently documented layering
> order (not that there seems to be any intention behind that that I know
> of[1]).

It would break current lightweight themes (and static themes once the lightweight themes are converted over).


Anyway, changing the ordering would be perfectly fine considering the current uses (Quantum Lights + Gradientus), which don't rely on the layering. The only thing that's preventing this bug from moving forward is that the current patch broke themes on MacOS (bug 1454016). For the MacOS issue to be fixed, bug 1482157 needs to be fixed, and probably needs someone familiar with Cocoa (mstange/spohl).

Comment 34

[Tim Nguyen :ntim]

Attachment: Part 1: Make additional backgrounds alignment relative to toolbox

MozReview-Commit-ID: 5gjrzZiLx0Q

Comment 35

[Tim Nguyen :ntim]

Attachment: Part 0: Workaround bug 1482157, set -moz-appearance:toolbox on #navigator-toolbox::after

MozReview-Commit-ID: CuJIYn9ioPO

Comment 36

[Tim Nguyen :ntim]

Attachment: Part 1: Make additional backgrounds alignment relative to toolbox

MozReview-Commit-ID: 5gjrzZiLx0Q

Comment 37

[Tim Nguyen :ntim]

Comment on attachment 9001892 [details] [diff] [review]
Part 1: Make additional backgrounds alignment relative to toolbox

Already reviewed by :jaws. Haven't changed anything here.

Comment 38

[Dão Gottwald [:dao]]

Does -moz-appearance: toolbox have any visual effect on Mac other than removing borders and backgrounds?

Comment 39

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #38)
> Does -moz-appearance: toolbox have any visual effect on Mac other than
> removing borders and backgrounds?

I've tested it on random elements in web content, and I can confirm that's all it does.

Comment 40

[Tim Nguyen :ntim]

Attachment: Part 1: Make additional backgrounds alignment relative to toolbox

MozReview-Commit-ID: 5gjrzZiLx0Q

Comment 41

[Dão Gottwald [:dao]]

Comment on attachment 9001896 [details] [diff] [review]
Part 1: Make additional backgrounds alignment relative to toolbox

>--- a/browser/base/content/browser.css
>+++ b/browser/base/content/browser.css
>@@ -8,7 +8,6 @@
> :root {
>   --panelui-subview-transition-duration: 150ms;
>   --lwt-additional-images: none;
>-  --lwt-background-alignment: right top;
>   --lwt-background-tiling: no-repeat;
> }
> 
>@@ -18,13 +17,11 @@
> 
> :root:-moz-lwtheme {
>   background-color: var(--lwt-accent-color) !important;
>-  background-image: var(--lwt-additional-images) !important;
>-  background-position: var(--lwt-background-alignment) !important;
>-  background-repeat: var(--lwt-background-tiling) !important;
> }
> 
> :root:-moz-lwtheme[lwtheme-image] {
>-  background-image: var(--lwt-header-image), var(--lwt-additional-images) !important;
>+  background-image: var(--lwt-header-image) !important;
>+  background-position: right top !important;
> }
> 
> :root:-moz-lwtheme:-moz-window-inactive {
>@@ -350,6 +347,14 @@ toolbarpaletteitem {
>   transition: 1.5s margin-top ease-out;
> }
> 
>+/* Set additional backgrounds alignment relative to toolbox*/
>+#navigator-toolbox:-moz-lwtheme {
>+  background-image: var(--lwt-additional-images);
>+  background-position: var(--lwt-background-alignment);
>+  background-repeat: var(--lwt-background-tiling);
>+}

Please move this to browser/themes/shared/browser.inc.css.

Comment 42

[Tim Nguyen :ntim]

Attachment: Part 1: Make additional backgrounds alignment relative to toolbox

MozReview-Commit-ID: 5gjrzZiLx0Q

Comment 43

[Tim Nguyen :ntim]

Attachment: Part 1: Make additional backgrounds alignment relative to toolbox

MozReview-Commit-ID: 5gjrzZiLx0Q

Comment 44

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #41)
> Comment on attachment 9001896 [details] [diff] [review]
> Part 1: Make additional backgrounds alignment relative to toolbox
> 
> >--- a/browser/base/content/browser.css
> >+++ b/browser/base/content/browser.css
> >@@ -8,7 +8,6 @@
> > :root {
> >   --panelui-subview-transition-duration: 150ms;
> >   --lwt-additional-images: none;
> >-  --lwt-background-alignment: right top;
> >   --lwt-background-tiling: no-repeat;
> > }
> > 
> >@@ -18,13 +17,11 @@
> > 
> > :root:-moz-lwtheme {
> >   background-color: var(--lwt-accent-color) !important;
> >-  background-image: var(--lwt-additional-images) !important;
> >-  background-position: var(--lwt-background-alignment) !important;
> >-  background-repeat: var(--lwt-background-tiling) !important;
> > }
> > 
> > :root:-moz-lwtheme[lwtheme-image] {
> >-  background-image: var(--lwt-header-image), var(--lwt-additional-images) !important;
> >+  background-image: var(--lwt-header-image) !important;
> >+  background-position: right top !important;
> > }
> > 
> > :root:-moz-lwtheme:-moz-window-inactive {
> >@@ -350,6 +347,14 @@ toolbarpaletteitem {
> >   transition: 1.5s margin-top ease-out;
> > }
> > 
> >+/* Set additional backgrounds alignment relative to toolbox*/
> >+#navigator-toolbox:-moz-lwtheme {
> >+  background-image: var(--lwt-additional-images);
> >+  background-position: var(--lwt-background-alignment);
> >+  background-repeat: var(--lwt-background-tiling);
> >+}
> 
> Please move this to browser/themes/shared/browser.inc.css.

I moved the #navigator-toolbox:-moz-lwtheme rule to browser.inc.css, but I filed bug 1484942 for content/browser.css (the current patch is something we want to uplift, so I'd like to avoid unrelated changes).

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37b4dc3bc73a
Part 0: Workaround bug 1482157, set -moz-appearance:toolbox on #navigator-toolbox::after. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d748613485f
Part 1: Make additional backgrounds alignment relative to toolbox. r=jaws

Comment 46

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1ff98d5aab
Backed out 2 changesets for causing bc perma failures in toolkit/components/extensions/test/browser/browser_ext_themes_sanitization.js

Comment 47

[Eliza Balazs [:ebalazs_]]

Backed out 2 changesets (Bug 1443561) for causing bc perma failures in toolkit/components/extensions/test/browser/browser_ext_themes_sanitization.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1d748613485fee744aa70641100664334118fa23&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1d748613485fee744aa70641100664334118fa23&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable

Backout push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3e1ff98d5aab1040e6333f9fc4547e3aafe81b1b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 48

[Tim Nguyen :ntim]

Attachment: Part 0: Workaround bug 1482157, set -moz-appearance:toolbox on #navigator-toolbox::after

MozReview-Commit-ID: CuJIYn9ioPO

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7555241630ce
Part 0: Workaround bug 1482157, set -moz-appearance:toolbox on #navigator-toolbox::after. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/6086b5e84d5b
Part 1: Make additional backgrounds alignment relative to toolbox. r=jaws

Comment 50

[Mike Conca [:mconca]]

Comment on attachment 9001999 [details] [diff] [review]
Part 1: Make additional backgrounds alignment relative to toolbox

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: this patch fixes the alignment and changes the z-order of additional backgrounds so that Firefox themes match watch can be created and shown on AMO
User impact if declined: themes that use additional backgrounds will appear incorrectly when installed. AMO theme creation and previewer will show themes as if this patch were applied in ESR (it is not aware of the version of firefox the user is running).
Fix Landed on Version: 63 (uplift request to 62)
Risk to taking this patch (and alternatives if risky): see Beta uplift risks
String or UUID changes made by this patch: none

Comment 51

[Mike Conca [:mconca]]

Comment on attachment 9002772 [details] [diff] [review]
Part 0: Workaround bug 1482157, set -moz-appearance:toolbox on #navigator-toolbox::after

Tim, I requested uplift of both of your patches to 62 (beta) and 60 (esr). Could you please fill in the Beta information form?  Thanks.

Comment 52

[Mike Conca [:mconca]]

Tim, can you also document here the final end state of additional_backgrounds and headerURL so that we all know where things landed.  When you do, can you please tag the bug with dev-doc-needed so that MDN also gets updated?  Thanks, again.

Comment 53

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/7555241630ce
https://hg.mozilla.org/mozilla-central/rev/6086b5e84d5b

Comment 54

[Ryan VanderMeulen [:RyanVM]]

If at all possible, it would be good to get some exploratory testing on this before taking it on Beta given its past history with causing regressions.

Comment 55

[Tim Nguyen :ntim]

Approval Request Comment
[Feature/Bug causing the regression]: additional_backgrounds API feature
[User impact if declined]: See comment 50
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: Install themes that use additional_backgrounds like Quantum Lights or Gradientus, and see if anything has regressed on those themes
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: CSS only.
[String changes made/needed]: n/a

Comment 56

[Chris Peterson [:cpeterson]]

Tim, I think this bug broke the Quantum Lights and Gradientus themes. I'm running macOS 10.13.6. I bisected the regression to this mozilla-inbound pushlog:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=51acbb8dc5013a729f91bb6127d7234be010904b&tochange=6086b5e84d5b5b89629f9b56c51de4e636aef6b6

Comment 57

[Tim Nguyen :ntim]

I could have sworn it was working when I tested it. 

In Nightly, it seems like the image loads are somehow blocked by CSP.

What's strange is that if you force load the images on :root, the images start loading again on #navigator-toolbox.

Kris, any idea where the CSP restrictions are set up ?

Comment 58

[Tim Nguyen :ntim]

Oh, I think I know what's happening... The CSS rules need to be in browser/base/content/browser.css for the images to be loaded.

It seems like I broke things after addressing Comment 41 and forgot to test it.

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/054c113b5cb1
Part 1b: Move #navigator-toolbox rule back to browser/base/content to avoid CSP issues. r=me

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c896958f358e
Part 1b: Move #navigator-toolbox rule back to browser/base/content to avoid CSP issues. r=me

Comment 61

[Tim Nguyen :ntim]

(I backed out the changeset from comment 59 in https://hg.mozilla.org/integration/mozilla-inbound/rev/a92aacd177b541c80474ea95f50dbc3c3bd5d57e)

Comment 62

[Alexandra Moga [:LexaSV]]

I have installed several themes that are using multiple_background_alignments and detailed my results below.

Tested Platforms:

- Windows 10x64 and 7x64 with Nightly 63.0a1 (Build 20180822100104)
- macOS High Sierra 10.13.2 with Nightly 63.0a1 (Build 20180821220101)


1. installed - https://addons.mozilla.org/en-US/firefox/addon/gradientus/ => theme is not correctly applied
2. submitted other static themes that are using multiple_background_alignments and for each example the theme was not applied correctly:
 - https://addons-dev.allizom.org/en-US/firefox/addon/top-align/ - theme using only top alignments - backgrounds not applied
 - https://addons-dev.allizom.org/en-US/firefox/addon/multiple-st-backgr-not-fixed/ - theme using top and bottom alignments - backgrounds not applied
 - https://addons-dev.allizom.org/en-US/firefox/addon/standard-multiple-backg/ - theme without any alignments - background not applied (I've attached a screenshot for this example)

Comment 63

[Alexandra Moga [:LexaSV]]

Attachment: backg_not_applied.png

Comment 64

[Ryan VanderMeulen [:RyanVM]]

Presumably comment 62 is just referencing the same issues found in comment 56 and are addressed by the follow-up on inbound. That said, please update the patches attached accordingly so we don't miss anything on the uplifts (also, it would be helpful if obsolete patches could be marked as such).

Comment 65

[Tim Nguyen :ntim]

Attachment: Part 1 for beta uplift

This contains the part 1 + the followup.

Comment 66

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 67

[Maria Berlinger [:mberlinger], Services QA]

I have managed to reproduce this bug on an affected Nightly build 63.0a1 (2018-08-22), using the theme from comment 12 (https://addons.mozilla.org/en-US/firefox/addon/gradientus/). 

This is verified fixed on latest Nightly 63.0a1 (20180822221004) on the following OSes: Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13.

Comment 68

[Tim Nguyen :ntim]

Hey Alexandra, can you test if all themes from comment 62 work as expected ?

Comment 69

[Alexandra Moga [:LexaSV]]

I have retested with the themes mentioned above and the backgrounds are applied as expected now.


Tested Platforms:

- Windows 10x64 and 7x64 with Nightly 63.0a1 (Build 20180823100106)
- macOS High Sierra 10.13.2 with Nightly 63.0a1 (Build 20180822221004)

I've made another sample that uses top, center and bottom alignments and attached a screenshot to show how the layout look after the theme is installed.
Example: https://addons-dev.allizom.org/en-US/firefox/addon/top-center-bottom/

Comment 70

[Alexandra Moga [:LexaSV]]

Attachment: top_center_bottom.png

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9002772 [details] [diff] [review]
Part 0: Workaround bug 1482157, set -moz-appearance:toolbox on #navigator-toolbox::after

Thanks for the verification, Alexandra. I wish this would have had more time to bake on Nightly before taking it in Beta, but I'm also not crazy about taking it directly in the RC builds next week. Approved for 62.0b20 so we have time to suss out any regressions in the field before next week.

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6cbbaad0935b
https://hg.mozilla.org/releases/mozilla-beta/rev/7fda8e538067

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/2d5cc6eecb7c
https://hg.mozilla.org/releases/mozilla-esr60/rev/4c33709c2271

Comment 74

[Alexandra Moga [:LexaSV]]

I've verified additional backgrounds again with:

1. Beta 62.0b20 (20180823143155) with Win10x64 and macOS High Sierra 10.13.2 - themes are correctly applied in the browser

2. Firefox 60.1.1esr (20180823175230) with Win10x64 and macOS High Sierra 10.13.2 - most of multiple background themes can be installed but there are some examples that are not applied in the browser, although the installation process is completed.

Here are some examples that are not working with esr:
https://addons-dev.allizom.org/en-US/firefox/addon/alignment-none/?src=search
https://addons-dev.allizom.org/en-US/firefox/addon/alignment-center/?src=search
https://addons-dev.allizom.org/en-US/firefox/addon/alignment-left-center-right/?src=search

Comment 75

[Tim Nguyen :ntim]

Hi Alexandra,

Thank you for testing. Does bug 1486018 fix the issue on ESR ?

Comment 76

[Tim Nguyen :ntim]

Also, was this a pre-existing issue before this bug ?

Comment 77

[Alexandra Moga [:LexaSV]]

Hey Tim,

I've checked again with the latest ESR - Firefox 60.2.0esr (20180827170958) - and I'm still reproducing the issue.

I've also looked at an older version - 60.1.0esr (20180621121604) - where the issue can also be reproduced.  It does seem that this has older roots. 

If it helps, this is the error I receive in the console after installing one of the themes that are not applied:

"[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) 
[nsIWebBrowserPersist.saveURI]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: 
resource://gre/modules/LightweightThemeManager.jsm :: _persistImage :: line 872"  data: no]"

I can file a new bug if you'd like to track this separately.

Comment 78

[Tim Nguyen :ntim]

(In reply to AlexandraGM from comment #77)
> Hey Tim,
> 
> I've checked again with the latest ESR - Firefox 60.2.0esr (20180827170958)
> - and I'm still reproducing the issue.
> 
> I've also looked at an older version - 60.1.0esr (20180621121604) - where
> the issue can also be reproduced.  It does seem that this has older roots. 
> 
> If it helps, this is the error I receive in the console after installing one
> of the themes that are not applied:
> 
> "[Exception... "Component returned failure code: 0x80004005
> (NS_ERROR_FAILURE) 
> [nsIWebBrowserPersist.saveURI]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)" 
> location: "JS frame :: 
> resource://gre/modules/LightweightThemeManager.jsm :: _persistImage :: line
> 872"  data: no]"
> 
> I can file a new bug if you'd like to track this separately.

Yes, that would be preferable, thanks! (Also it would be nice to get a regression range if possible)

Comment 79

[Chris Mills [:cmills]]

We didn't think this needed any documentation; if we are wrong, please let us know what you think MDN needs. Thanks!