WebExtension themes additional backgrounds alignment should be relative to the toolbox

VERIFIED FIXED in Firefox -esr60

Status

()

P1
normal
VERIFIED FIXED
8 months ago
2 months ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed})

unspecified
Firefox 63
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr6062+ fixed, firefox61+ wontfix, firefox62+ verified, firefox63+ verified)

Details

(Whiteboard: [blocking-static-themes-fx][ntim-intern-project])

Attachments

(5 attachments, 6 obsolete attachments)

13.55 KB, patch
Details | Diff | Splinter Review
4.76 KB, patch
Details | Diff | Splinter Review
156.11 KB, image/png
Details
13.66 KB, patch
Details | Diff | Splinter Review
38.54 KB, image/png
Details
: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.
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.
> 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.
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?
Flags: needinfo?(amckay)

Updated

8 months ago
Flags: needinfo?(amckay) → needinfo?(ddurst)
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.
Flags: needinfo?(ddurst)
"a different theme" == the same theme but it will render potentially differently depending on the content of headerURL.
(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/
Flags: needinfo?(awilliamson)
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.
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.
Flags: needinfo?(awilliamson) → needinfo?(jorge)
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.
Flags: needinfo?(jorge)
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.
Flagging needinfo from the two add-on authors.
Flags: needinfo?(smartell)
Flags: needinfo?(cynthia)
(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.
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.
Flags: needinfo?(cynthia)
Vivek, can you work on this?
Flags: needinfo?(vivek3zero)

Comment 15

8 months ago
Yes I can get started right away
Flags: needinfo?(vivek3zero)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

7 months ago
Attachment #8967298 - Flags: review?(mconley)
Attachment #8967298 - Flags: review?(jaws)
Assignee: nobody → vivek3zero
Status: NEW → ASSIGNED
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.
Attachment #8967298 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

7 months ago
mozreview-review-reply
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 on attachment 8967298 [details]
Bug 1443561 - WebExtension themes additional backgrounds alignment should be relative to the toolbox

https://reviewboard.mozilla.org/r/235980/#review242024
Attachment #8967298 - Flags: review?(jaws) → review+

Comment 25

7 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f3e01de682f1
WebExtension themes additional backgrounds alignment should be relative to the toolbox r=jaws

Comment 26

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f3e01de682f1
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
(Assignee)

Updated

7 months ago
Attachment #8967298 - Flags: review?(mconley)
Depends on: 1454016
Backed out on request: https://hg.mozilla.org/mozilla-central/rev/ceac91dc08bef5d099c10dda632fc3651b23c897
Status: RESOLVED → REOPENED
Flags: needinfo?(vivek3zero)
Resolution: FIXED → ---

Updated

7 months ago
status-firefox61: fixed → ---
Target Milestone: Firefox 61 → ---

Comment 28

7 months ago
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.
Flags: needinfo?(vivek3zero)

Updated

6 months ago
Priority: -- → P5
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

3 months ago
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
Flags: needinfo?(dao+bmo)

Updated

3 months ago
Flags: needinfo?(dao+bmo) → needinfo?(jaws)
This is now blocking AMO's release of static theme support.
Priority: P5 → P1
(Assignee)

Updated

3 months ago
Depends on: 1482157
status-firefox61: --- → affected
status-firefox62: --- → affected
status-firefox63: --- → affected
status-firefox-esr60: --- → affected
tracking-firefox61: --- → ?
tracking-firefox62: --- → +
tracking-firefox63: --- → +
tracking-firefox-esr60: --- → ?
status-firefox61: affected → fix-optional
tracking-firefox61: ? → +
(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.
(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).
Flags: needinfo?(jaws)
Whiteboard: [blocking-static-themes-fx]
Created attachment 9001889 [details] [diff] [review]
Part 1: Make additional backgrounds alignment relative to toolbox

MozReview-Commit-ID: 5gjrzZiLx0Q
(Assignee)

Updated

3 months ago
Assignee: vivek3zero → ntim.bugs
Status: REOPENED → ASSIGNED
(Assignee)

Updated

3 months ago
Attachment #9001889 - Attachment is obsolete: true
Created attachment 9001890 [details] [diff] [review]
Part 0: Workaround bug 1482157, set -moz-appearance:toolbox on #navigator-toolbox::after

MozReview-Commit-ID: CuJIYn9ioPO
Attachment #9001890 - Flags: review?(dao+bmo)
Created attachment 9001892 [details] [diff] [review]
Part 1: Make additional backgrounds alignment relative to toolbox

MozReview-Commit-ID: 5gjrzZiLx0Q
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.
Attachment #9001892 - Flags: review+
Does -moz-appearance: toolbox have any visual effect on Mac other than removing borders and backgrounds?
(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.
Created attachment 9001896 [details] [diff] [review]
Part 1: Make additional backgrounds alignment relative to toolbox

MozReview-Commit-ID: 5gjrzZiLx0Q
(Assignee)

Updated

3 months ago
Attachment #9001892 - Attachment is obsolete: true
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.
Created attachment 9001997 [details] [diff] [review]
Part 1: Make additional backgrounds alignment relative to toolbox

MozReview-Commit-ID: 5gjrzZiLx0Q
(Assignee)

Updated

3 months ago
Attachment #9001896 - Attachment is obsolete: true
Created attachment 9001999 [details] [diff] [review]
Part 1: Make additional backgrounds alignment relative to toolbox

MozReview-Commit-ID: 5gjrzZiLx0Q
(Assignee)

Updated

3 months ago
Attachment #9001997 - Attachment is obsolete: true

Updated

3 months ago
Attachment #9001890 - Flags: review?(dao+bmo) → review+
(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

3 months ago
Pushed by ntim.bugs@gmail.com:
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
(Assignee)

Updated

3 months ago
Whiteboard: [blocking-static-themes-fx] → [blocking-static-themes-fx][ntim-intern-project]

Comment 46

3 months ago
Backout by ebalazs@mozilla.com:
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
Created attachment 9002772 [details] [diff] [review]
Part 0: Workaround bug 1482157, set -moz-appearance:toolbox on #navigator-toolbox::after

MozReview-Commit-ID: CuJIYn9ioPO

Comment 49

3 months ago
Pushed by ntim.bugs@gmail.com:
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
(Assignee)

Updated

3 months ago
Flags: needinfo?(ntim.bugs)

Comment 50

3 months ago
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
Attachment #9001999 - Flags: approval-mozilla-esr60?
Attachment #9001999 - Flags: approval-mozilla-beta?

Comment 51

3 months ago
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.
Flags: needinfo?(ntim.bugs)
Attachment #9002772 - Flags: approval-mozilla-esr60?
Attachment #9002772 - Flags: approval-mozilla-beta?

Comment 52

3 months ago
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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7555241630ce
https://hg.mozilla.org/mozilla-central/rev/6086b5e84d5b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago3 months ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
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.
Flags: qe-verify+
Flags: needinfo?(rares.bologa)
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
Flags: needinfo?(ntim.bugs)
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
Flags: needinfo?(ntim.bugs)
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 ?
Flags: needinfo?(ntim.bugs) → needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)

Comment 59

3 months ago
Pushed by ntim.bugs@gmail.com:
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

3 months ago
Pushed by ntim.bugs@gmail.com:
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 62

3 months ago
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)
Flags: needinfo?(ntim.bugs)

Comment 63

3 months ago
Created attachment 9003137 [details]
backg_not_applied.png
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).
Status: RESOLVED → REOPENED
status-firefox61: fix-optional → wontfix
status-firefox63: fixed → affected
tracking-firefox-esr60: ? → 62+
Resolution: FIXED → ---
Target Milestone: Firefox 63 → ---
(Assignee)

Updated

3 months ago
Attachment #9001890 - Attachment is obsolete: true
Flags: needinfo?(ntim.bugs)
Created attachment 9003183 [details] [diff] [review]
Part 1 for beta uplift

This contains the part 1 + the followup.
https://hg.mozilla.org/mozilla-central/rev/c896958f358e
Status: REOPENED → RESOLVED
Last Resolved: 3 months ago3 months ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
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.
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
Flags: qe-verify+
Hey Alexandra, can you test if all themes from comment 62 work as expected ?
Flags: needinfo?(amoga)

Comment 69

3 months ago
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/
Flags: needinfo?(rares.bologa)
Flags: needinfo?(amoga)

Comment 70

3 months ago
Created attachment 9003452 [details]
top_center_bottom.png
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.
Attachment #9002772 - Flags: approval-mozilla-esr60?
Attachment #9002772 - Flags: approval-mozilla-esr60+
Attachment #9002772 - Flags: approval-mozilla-beta?
Attachment #9002772 - Flags: approval-mozilla-beta+
Attachment #9003183 - Flags: approval-mozilla-esr60+
Attachment #9003183 - Flags: approval-mozilla-beta+
Attachment #9001999 - Flags: approval-mozilla-esr60?
Attachment #9001999 - Flags: approval-mozilla-beta?
Attachment #8967298 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Depends on: 1486018
Flags: qe-verify+

Comment 74

3 months ago
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
Flags: needinfo?(ntim.bugs)

Updated

3 months ago
status-firefox62: fixed → verified
Hi Alexandra,

Thank you for testing. Does bug 1486018 fix the issue on ESR ?
Flags: needinfo?(ntim.bugs) → needinfo?(amoga)
Also, was this a pre-existing issue before this bug ?

Comment 77

3 months ago
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.
Flags: needinfo?(amoga) → needinfo?(ntim.bugs)

Updated

3 months ago
Flags: qe-verify+ → qe-verify-

Updated

3 months ago
Flags: qe-verify-
(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)
Flags: needinfo?(ntim.bugs)

Updated

3 months ago
Blocks: 1487631
No longer blocks: 1487631
See Also: → bug 1487631
(Assignee)

Updated

2 months ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.