Closed Bug 1443561 Opened 6 years ago Closed 6 years ago

WebExtension themes additional backgrounds alignment should be relative to the toolbox

Categories

(Firefox :: Theme, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox-esr60 62+ fixed
firefox61 + wontfix
firefox62 + verified
firefox63 + verified

People

(Reporter: ntim, Assigned: ntim)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

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

Attachments

(5 files, 6 obsolete files)

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)
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)
Yes I can get started right away
Flags: needinfo?(vivek3zero)
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 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+
Flags: needinfo?(smartell)
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
https://hg.mozilla.org/mozilla-central/rev/f3e01de682f1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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 → ---
Target Milestone: Firefox 61 → ---
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)
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.
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)
Flags: needinfo?(dao+bmo) → needinfo?(jaws)
This is now blocking AMO's release of static theme support.
Priority: P5 → P1
Depends on: 1482157
(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]
MozReview-Commit-ID: 5gjrzZiLx0Q
Assignee: vivek3zero → ntim.bugs
Status: REOPENED → ASSIGNED
Attachment #9001889 - Attachment is obsolete: true
MozReview-Commit-ID: CuJIYn9ioPO
Attachment #9001890 - Flags: review?(dao+bmo)
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.
MozReview-Commit-ID: 5gjrzZiLx0Q
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.
MozReview-Commit-ID: 5gjrzZiLx0Q
Attachment #9001896 - Attachment is obsolete: true
MozReview-Commit-ID: 5gjrzZiLx0Q
Attachment #9001997 - Attachment is obsolete: true
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).
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
Whiteboard: [blocking-static-themes-fx] → [blocking-static-themes-fx][ntim-intern-project]
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
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
Flags: needinfo?(ntim.bugs)
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 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?
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.
https://hg.mozilla.org/mozilla-central/rev/7555241630ce
https://hg.mozilla.org/mozilla-central/rev/6086b5e84d5b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
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)
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
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
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)
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
Resolution: FIXED → ---
Target Milestone: Firefox 63 → ---
Attachment #9001890 - Attachment is obsolete: true
Flags: needinfo?(ntim.bugs)
This contains the part 1 + the followup.
https://hg.mozilla.org/mozilla-central/rev/c896958f358e
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
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
Flags: qe-verify+
Hey Alexandra, can you test if all themes from comment 62 work as expected ?
Flags: needinfo?(amoga)
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 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
Depends on: 1486018
Flags: qe-verify+
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)
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 ?
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)
Flags: qe-verify+ → qe-verify-
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)
Blocks: 1487631
No longer blocks: 1487631
See Also: → 1487631
Keywords: dev-doc-needed

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

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

Attachment

General

Creator:
Created:
Updated:
Size: