Closed
Bug 1443561
Opened 6 years ago
Closed 5 years ago
WebExtension themes additional backgrounds alignment should be relative to the toolbox
Categories
(Firefox :: Theme, defect, P1)
Firefox
Theme
Tracking
()
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
|
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
156.11 KB,
image/png
|
Details | |
13.66 KB,
patch
|
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr60+
|
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.
Comment 1•6 years ago
|
||
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•6 years ago
|
||
> 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•6 years ago
|
||
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•6 years ago
|
Flags: needinfo?(amckay) → needinfo?(ddurst)
Comment 4•6 years ago
|
||
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)
Comment 5•6 years ago
|
||
"a different theme" == the same theme but it will render potentially differently depending on the content of headerURL.
Comment 6•6 years ago
|
||
(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)
Comment 7•6 years ago
|
||
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•6 years ago
|
||
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)
Comment 9•6 years ago
|
||
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)
Comment 10•6 years ago
|
||
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•6 years ago
|
||
Flagging needinfo from the two add-on authors.
Flags: needinfo?(smartell)
Flags: needinfo?(cynthia)
Assignee | ||
Comment 12•6 years ago
|
||
(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•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8967298 -
Flags: review?(mconley)
Attachment #8967298 -
Flags: review?(jaws)
Updated•6 years ago
|
Assignee: nobody → vivek3zero
Status: NEW → ASSIGNED
Comment 19•6 years ago
|
||
mozreview-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 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•6 years 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 24•6 years ago
|
||
mozreview-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/#review242024
Attachment #8967298 -
Flags: review?(jaws) → review+
Updated•6 years ago
|
Flags: needinfo?(smartell)
Comment 25•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f3e01de682f1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Assignee | ||
Updated•6 years ago
|
Attachment #8967298 -
Flags: review?(mconley)
Comment 27•6 years ago
|
||
Backed out on request: https://hg.mozilla.org/mozilla-central/rev/ceac91dc08bef5d099c10dda632fc3651b23c897
Status: RESOLVED → REOPENED
Flags: needinfo?(vivek3zero)
Resolution: FIXED → ---
Updated•6 years ago
|
status-firefox61:
fixed → ---
Target Milestone: Firefox 61 → ---
Comment 28•6 years 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 years ago
|
Priority: -- → P5
Comment 29•6 years ago
|
||
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•5 years 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•5 years ago
|
Flags: needinfo?(dao+bmo) → needinfo?(jaws)
Comment 31•5 years ago
|
||
This is now blocking AMO's release of static theme support.
Priority: P5 → P1
status-firefox61:
--- → affected
status-firefox62:
--- → affected
status-firefox63:
--- → affected
status-firefox-esr60:
--- → affected
tracking-firefox61:
--- → ?
tracking-firefox62:
--- → +
tracking-firefox63:
--- → +
tracking-firefox-esr60:
--- → ?
Updated•5 years ago
|
Comment 32•5 years ago
|
||
(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.
Assignee | ||
Comment 33•5 years ago
|
||
(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).
Updated•5 years ago
|
Flags: needinfo?(jaws)
Whiteboard: [blocking-static-themes-fx]
Assignee | ||
Comment 34•5 years ago
|
||
MozReview-Commit-ID: 5gjrzZiLx0Q
Assignee | ||
Updated•5 years ago
|
Assignee: vivek3zero → ntim.bugs
Status: REOPENED → ASSIGNED
Assignee | ||
Updated•5 years ago
|
Attachment #9001889 -
Attachment is obsolete: true
Assignee | ||
Comment 35•5 years ago
|
||
MozReview-Commit-ID: CuJIYn9ioPO
Attachment #9001890 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 36•5 years ago
|
||
MozReview-Commit-ID: 5gjrzZiLx0Q
Assignee | ||
Comment 37•5 years ago
|
||
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+
Comment 38•5 years ago
|
||
Does -moz-appearance: toolbox have any visual effect on Mac other than removing borders and backgrounds?
Assignee | ||
Comment 39•5 years ago
|
||
(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.
Assignee | ||
Comment 40•5 years ago
|
||
MozReview-Commit-ID: 5gjrzZiLx0Q
Assignee | ||
Updated•5 years ago
|
Attachment #9001892 -
Attachment is obsolete: true
Comment 41•5 years ago
|
||
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.
Assignee | ||
Comment 42•5 years ago
|
||
MozReview-Commit-ID: 5gjrzZiLx0Q
Assignee | ||
Updated•5 years ago
|
Attachment #9001896 -
Attachment is obsolete: true
Assignee | ||
Comment 43•5 years ago
|
||
MozReview-Commit-ID: 5gjrzZiLx0Q
Assignee | ||
Updated•5 years ago
|
Attachment #9001997 -
Attachment is obsolete: true
Updated•5 years ago
|
Attachment #9001890 -
Flags: review?(dao+bmo) → review+
Assignee | ||
Comment 44•5 years ago
|
||
(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•5 years 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•5 years ago
|
Whiteboard: [blocking-static-themes-fx] → [blocking-static-themes-fx][ntim-intern-project]
Comment 46•5 years 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
Comment 47•5 years ago
|
||
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
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 48•5 years ago
|
||
MozReview-Commit-ID: CuJIYn9ioPO
Comment 49•5 years 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•5 years ago
|
Flags: needinfo?(ntim.bugs)
Comment 50•5 years 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•5 years 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•5 years 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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7555241630ce https://hg.mozilla.org/mozilla-central/rev/6086b5e84d5b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago → 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Comment 54•5 years ago
|
||
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)
Assignee | ||
Comment 55•5 years ago
|
||
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)
Comment 56•5 years ago
|
||
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)
Assignee | ||
Comment 57•5 years ago
|
||
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)
Assignee | ||
Comment 58•5 years ago
|
||
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•5 years 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•5 years 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
Assignee | ||
Comment 61•5 years ago
|
||
(I backed out the changeset from comment 59 in https://hg.mozilla.org/integration/mozilla-inbound/rev/a92aacd177b541c80474ea95f50dbc3c3bd5d57e)
Comment 62•5 years 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•5 years ago
|
||
Comment 64•5 years ago
|
||
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 → ---
Assignee | ||
Updated•5 years ago
|
Attachment #9001890 -
Attachment is obsolete: true
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 65•5 years ago
|
||
This contains the part 1 + the followup.
![]() |
||
Comment 66•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c896958f358e
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Comment 67•5 years ago
|
||
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.
Assignee | ||
Comment 68•5 years ago
|
||
Hey Alexandra, can you test if all themes from comment 62 work as expected ?
Flags: needinfo?(amoga)
Comment 69•5 years 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•5 years ago
|
||
Comment 71•5 years ago
|
||
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+
Updated•5 years ago
|
Attachment #9003183 -
Flags: approval-mozilla-esr60+
Attachment #9003183 -
Flags: approval-mozilla-beta+
Updated•5 years ago
|
Attachment #9001999 -
Flags: approval-mozilla-esr60?
Attachment #9001999 -
Flags: approval-mozilla-beta?
Updated•5 years ago
|
Attachment #8967298 -
Attachment is obsolete: true
Comment 72•5 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/6cbbaad0935b https://hg.mozilla.org/releases/mozilla-beta/rev/7fda8e538067
Flags: in-testsuite+
Comment 73•5 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/2d5cc6eecb7c https://hg.mozilla.org/releases/mozilla-esr60/rev/4c33709c2271
Updated•5 years ago
|
Flags: qe-verify+
Comment 74•5 years 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•5 years ago
|
Assignee | ||
Comment 75•5 years ago
|
||
Hi Alexandra, Thank you for testing. Does bug 1486018 fix the issue on ESR ?
Flags: needinfo?(ntim.bugs) → needinfo?(amoga)
Assignee | ||
Comment 76•5 years ago
|
||
Also, was this a pre-existing issue before this bug ?
Comment 77•5 years 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•5 years ago
|
Flags: qe-verify+ → qe-verify-
Assignee | ||
Comment 78•5 years ago
|
||
(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•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Keywords: dev-doc-needed
Comment 79•5 years ago
|
||
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.
Description
•