1453035 - --lwt-text-color fallback doesn't work

Status: VERIFIED FIXED

(Toolkit :: Themes, defect, P1)

Description

[Tim Nguyen :ntim]

See bug 1449327 comment 9.

Comment 1

[Dão Gottwald [:dao]]

Is someone still available to fix this?

Comment 2

[Tim Nguyen :ntim]

Attachment: Bug 1453035 - Fix textcolor theme fallback.

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

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8971176 [details]
Bug 1453035 - Fix textcolor theme fallback.

https://reviewboard.mozilla.org/r/239974/#review245714

::: toolkit/modules/LightweightThemeConsumer.jsm:27
(Diff revision 1)
>    }],
>    ["--lwt-text-color", {
>      lwtProperty: "textcolor",
>      processColor(rgbaChannels, element) {
> -      if (!rgbaChannels) {
> +      if (!rgbaChannels || rgbaChannels.a == 0) {
>          element.removeAttribute("lwthemetextcolor");

So this means we could activate a lwtheme without setting --lwt-text-color and the lwthemetextcolor attribute. I don't think we want that.

::: toolkit/modules/LightweightThemeConsumer.jsm:33
(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})`;

Can we just drop the alpha channel here?

Comment 4

[Marion Daly [:mdaly]]

Can we make sure this gets into 61?

Comment 5

[Marion Daly [:mdaly]]

Can we find someone for this?

Comment 6

[Tim Nguyen :ntim]

A newer version of this patch is in bug 1460287 (and fixes both this bug and bug 1460287). Thanks Gijs!

Comment 7

[:Gijs (he/him)]

Fixed in bug 1460287.

Comment 8

[Tim Nguyen :ntim]

needinfo? myself to provide manual testing instructions

Comment 9

[Tim Nguyen :ntim]

Testing instructions:

1. create a webextension with:

      "theme": {
        "colors": {
          "accentcolor": "orange",
          "textcolor": "transparent",
        },
      }

in the manifest.

2. check that the text color is black and not transparent

Comment 10

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Following steps from comment 9 I reproduced this issue using Fx 61.0a1 (20180410100115)on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 61.0 and Fx 62.0a1, on Windows 10 x64, mac os X 10.12.6 and Ubuntu 14.04 LTS.