Theme doesn't load and will not load after Firefox update

VERIFIED FIXED in Firefox 61

Status

defect
P1
blocker
VERIFIED FIXED
Last year
Last year

People

(Reporter: cmshea, Assigned: Gijs)

Tracking

(Depends on 1 bug, {regression})

Trunk
mozilla62
Dependency tree / graph
Bug Flags:
in-qa-testsuite +
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61 verified, firefox62+ verified)

Details

Attachments

(1 attachment)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180509100510

Steps to reproduce:

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180509100510

Install https://addons.mozilla.org/en-US/firefox/addon/dream-of-waves/

It works. But wait for Firefox to update.


Actual results:

After an update, the theme doesn't get applied. And trying to select the theme in about:addons doesn't work.

The only way to get it to work again is to uninstall the theme and reinstall it.


Expected results:

A custom theme should work across updates.
Component: Untriaged → Add-ons Manager
Product: Firefox → Toolkit
:jaws, do you have cycles to take a look?
Flags: needinfo?(jaws)
See Also: → 1460522
Duplicate of this bug: 1460522
[Tracking Requested - why for this release]: bug 1450975 tracked 60 and was fixed for 60 but the fix seems to be incomplete on 61 and 62, so we should track for 61. :-\


As far as I can tell bug 1450975 was never fixed on 61. The uplifted patch works on 60 release (lucky us!), but if I follow the verification steps I gave QA in bug 1450975 comment 81 things are still broken, even on the very first nightly in which the patch from bug 1450975 landed. The 61 beta cycle is really short, so we need to get this addressed ASAP.
Status: UNCONFIRMED → NEW
Ever confirmed: true
This is caused by the new window not having the `lwtheme` attribute, which means none of the lwtheme selectors apply,which means things are broken.

bug 1453035 changed where we set the lwtheme attribute on 61, and didn't land on 60, so presumably that's how this is broken.
Blocks: 1417883
See Also: → 1453035
Component: Add-ons Manager → WebExtensions: Themes
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
This still leaves a bit of a mess in that the accentcolor changes from black (initial install) to white (after an update), because the property disappears and gets sanitized differently (black for the invalid "#" and white for the non-existent property).

I asked ntim about this and apparently the white fallback has been there since forever. I dunno if this means that this was already the case prior to the original regression in bug 1404688. I guess so? Depends if we process fallbacks differently now. Worth checking, but I don't see an obvious way to detect this case without breaking any webextensions themes, and the patch seems like a strict improvement. I'll happily take suggestions about how to fix this without breaking any of the umpteen different combinatorial setups between lwthemes/webextensions themes, and all the stuff they can specify.
(This is largely the same patch as bug 1453035 but with different fallback stuff so we *do* set the text color, which seemed like the main issue in the review in that bug.)
Blocks: 1453035
Comment on attachment 8975098 [details]
Bug 1460287 - use black as a fallback for textcolor and always set lwtheme attribute if theme is active,

https://reviewboard.mozilla.org/r/243444/#review249330

::: toolkit/modules/LightweightThemeConsumer.jsm:32
(Diff revision 1)
> -        return null;
>        }
>        const {r, g, b, a} = rgbaChannels;
>        const luminance = _getLuminance(r, g, b);
>        element.setAttribute("lwthemetextcolor", luminance <= 110 ? "dark" : "bright");
> -      element.setAttribute("lwtheme", "true");
> +      return `rgba(${r}, ${g}, ${b}, ${a})`;

As far as I can tell there's no good reason to support an alpha channel for textcolor, so let's drop that here and remove the earlier rgbaChannels.a == 0 check.
Attachment #8975098 - Flags: review?(dao+bmo)
(In reply to :Gijs (he/him) from comment #6)
> This still leaves a bit of a mess in that the accentcolor changes from black
> (initial install) to white (after an update), because the property
> disappears and gets sanitized differently (black for the invalid "#" and
> white for the non-existent property).
> 
> I asked ntim about this and apparently the white fallback has been there
> since forever. I dunno if this means that this was already the case prior to
> the original regression in bug 1404688. I guess so? Depends if we process
> fallbacks differently now. Worth checking, but I don't see an obvious way to
> detect this case without breaking any webextensions themes, and the patch
> seems like a strict improvement. I'll happily take suggestions about how to
> fix this without breaking any of the umpteen different combinatorial setups
> between lwthemes/webextensions themes, and all the stuff they can specify.

Dão, seems this is *also* a regression, because prior to bug 1404688 this stayed black (that is, the line at the top of the tabs stayed black). Tested on macOS. With the attached patch, it goes from black to white.

I think this is also a regression from bug 1417883. Before, we did this:

    _setProperty(root, active, "--lwt-accent-color", this._sanitizeCSSColor(aData.accentcolor) || "white");

where the sanitize code would do:

    let span = this._doc.createElementNS("http://www.w3.org/1999/xhtml", "span");
    span.style.color = cssColor;
    cssColor = this._win.getComputedStyle(span).color;
    if (cssColor == "rgba(0, 0, 0, 0)" ||
        !cssColor) {
      return "";
    }
    // Remove alpha channel from color
    return `rgb(${_parseRGB(cssColor).join(", ")})`;

but presumably because in this case, accentcolor is an empty string and getCS thus returns whatever the default foreground color is (probably black, ie rgb(0,0,0)) we would just return that default foreground color.


Now, instead, we hit 

  ["--lwt-accent-color", {
    lwtProperty: "accentcolor",
    processColor(rgbaChannels, element) {
      if (!rgbaChannels || rgbaChannels.a == 0) {
        return "white";
      }

and return white, because of the early return in https://searchfox.org/mozilla-central/rev/babf96cf0c37b608e9663e2af73a4d21819c4af1/toolkit/modules/LightweightThemeConsumer.jsm#231-234 which returns `null` as the colour if it's unspecified.

Presumably, that last bit is an intentional part of bug 1417883, but it results in confusing consequences here. If the original result (black) was correct, we should probably ensure that we keep that. What do you think is the best way to achieve that here?
Flags: needinfo?(dao+bmo)
Comment on attachment 8975098 [details]
Bug 1460287 - use black as a fallback for textcolor and always set lwtheme attribute if theme is active,

https://reviewboard.mozilla.org/r/243444/#review249450

::: toolkit/modules/LightweightThemeConsumer.jsm:29
(Diff revision 2)
>        if (!rgbaChannels) {
> -        element.removeAttribute("lwthemetextcolor");
> +        rgbaChannels = {r: 0, g: 0, b: 0};
> -        element.removeAttribute("lwtheme");
> -        return null;
>        }
>        const {r, g, b, a} = rgbaChannels;

nit: a is now unused

::: toolkit/modules/LightweightThemeConsumer.jsm:29
(Diff revision 2)
>        if (!rgbaChannels) {
> -        element.removeAttribute("lwthemetextcolor");
> +        rgbaChannels = {r: 0, g: 0, b: 0};
> -        element.removeAttribute("lwtheme");
> -        return null;
>        }
>        const {r, g, b, a} = rgbaChannels;

As with accentcolor, can you add a comment that we're intentionally removing the alpha channel?

::: toolkit/modules/LightweightThemeConsumer.jsm:167
(Diff revision 2)
>      this._active = active;
>  
> +    if (active) {
> +      root.setAttribute("lwtheme", "true");
> +    } else {
> +      root.removeAttribute("lwtheme");

Should we also remove lwthemetextcolor here?
Attachment #8975098 - Flags: review?(dao+bmo) → review+
(In reply to :Gijs (he/him) from comment #10)
> (In reply to :Gijs (he/him) from comment #6)
> > This still leaves a bit of a mess in that the accentcolor changes from black
> > (initial install) to white (after an update), because the property
> > disappears and gets sanitized differently (black for the invalid "#" and
> > white for the non-existent property).
> > 
> > I asked ntim about this and apparently the white fallback has been there
> > since forever. I dunno if this means that this was already the case prior to
> > the original regression in bug 1404688. I guess so? Depends if we process
> > fallbacks differently now. Worth checking, but I don't see an obvious way to
> > detect this case without breaking any webextensions themes, and the patch
> > seems like a strict improvement. I'll happily take suggestions about how to
> > fix this without breaking any of the umpteen different combinatorial setups
> > between lwthemes/webextensions themes, and all the stuff they can specify.
> 
> Dão, seems this is *also* a regression, because prior to bug 1404688 this
> stayed black (that is, the line at the top of the tabs stayed black).

AFAIK that it stayed black was unintended and a regression in itself.

> With the attached patch, it goes from black to white.

That's correct. Missing or invalid accentcolor should be white since we're using it as a background. If we want to change that, that's a whole different discussion. (Options include using the text color instead of the accent color for the tab line, and/or letting authors set the tab line color explicitly.)
Flags: needinfo?(dao+bmo)
Severity: normal → blocker
Flags: qe-verify+
Flags: in-testsuite?
Keywords: regression
Priority: -- → P1
(In reply to Dão Gottwald [::dao] from comment #8)
> Comment on attachment 8975098 [details]
> Bug 1460287 - use black as a fallback for textcolor and always set lwtheme
> attribute if theme is active,
> 
> https://reviewboard.mozilla.org/r/243444/#review249330
> 
> ::: toolkit/modules/LightweightThemeConsumer.jsm:32
> (Diff revision 1)
> > -        return null;
> >        }
> >        const {r, g, b, a} = rgbaChannels;
> >        const luminance = _getLuminance(r, g, b);
> >        element.setAttribute("lwthemetextcolor", luminance <= 110 ? "dark" : "bright");
> > -      element.setAttribute("lwtheme", "true");
> > +      return `rgba(${r}, ${g}, ${b}, ${a})`;
> 
> As far as I can tell there's no good reason to support an alpha channel for
> textcolor, so let's drop that here and remove the earlier rgbaChannels.a ==
> 0 check.

Seems like webextension themes explicitly test for support of this in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js , see these test failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d164332c2e38ecb0be6a2bffb1e968ba8925ca97&selectedJob=178110783

I'm inclined to keep the alpha support for now given that it seems plausible that where themes use more fine-grained control over individual browser foreground/background bits, alpha is wanted, and given that this patch needs to be uplifted - is that OK?
Flags: needinfo?(dao+bmo)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/785ebb4edf6a
use black as a fallback for textcolor and always set lwtheme attribute if theme is active, r=dao
Duplicate of this bug: 1460694
https://hg.mozilla.org/mozilla-central/rev/785ebb4edf6a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(In reply to :Gijs (he/him) from comment #13)
> Seems like webextension themes explicitly test for support of this in
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/
> test/browser/browser_ext_themes_chromeparity.js

Discussed with Dão on IRC yesterday and decided to fix the test instead, removing support for alpha components for the text color. I'll file a follow-up for the black/white accentcolor issue (see comment #12).
Flags: needinfo?(dao+bmo)
Depends on: 1461617
Christopher, are you able to confirm that this fix works now that it's hit nightly? (It should have made at least yesterday's "late" nightly as well as today's)

Thank you!
Flags: needinfo?(cmshea)
(In reply to :Gijs (he/him) from comment #19)
> Christopher, are you able to confirm that this fix works now that it's hit
> nightly? (It should have made at least yesterday's "late" nightly as well as
> today's)
> 
> Thank you!

Yes! The theme now sticks around after updates and opening new windows. Thanks!
Flags: needinfo?(cmshea)
Verifying per comment #20.
Status: RESOLVED → VERIFIED
Comment on attachment 8975098 [details]
Bug 1460287 - use black as a fallback for textcolor and always set lwtheme attribute if theme is active,

Approval Request Comment
[Feature/Bug causing the regression]: 1417883
[User impact if declined]: some lwthemes stop working as soon as they update from AMO
[Is this code covered by automated tests?]: sort of... clearly not sufficient to detect this regression.
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: probably good once this has been uplifted.

Steps:
1. install the 'flag of colombia' theme off AMO ( https://addons.mozilla.org/en-US/firefox/addon/flag-of-co-385587-2/ )
2. turn on browser/chrome debugging in the devtools settings
3. open the browser console
3. run:

Cc["@mozilla.org/addons/integration;1"].getService(Ci.nsITimerCallback).notify(null)

4. wait for all the requests to AMO to complete (you can see them in the browser console if you've got the network bit of logging turned to visible).
5. Restart
6. verify that the theme is still correctly applied (pre-patch, you see something resembling the default theme but with the selected tab foreground colour being wrong on some OSes)

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not too much
[Why is the change risky/not risky?]: still a lot of beta runway, it's a JS/css-only change, adds some tests, has already been verified on nightly.
[String changes made/needed]: none
Attachment #8975098 - Flags: approval-mozilla-beta?
Comment on attachment 8975098 [details]
Bug 1460287 - use black as a fallback for textcolor and always set lwtheme attribute if theme is active,

Fix for broken LWT after being updated. Approved for 61.0b6.
Attachment #8975098 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180517141400

Verified as fixed on Firefox 61.0b6 on MacOS 10.13.4.
Product: Toolkit → WebExtensions
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.