1449327 - Fully transparent values no longer work as of bug 1417883

Status: RESOLVED FIXED

(WebExtensions :: Themes, defect, P1)

Description

[Tim Nguyen :ntim]

There are valid use cases to using transparent values: hiding borders/separators/... The Quantum Lights theme for example hides the top and bottom separators. No longer supporting those is a compatibility issue.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1449327 - Stop sanitizing fully transparent values.

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

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8962882 [details]
Bug 1449327 - Stop sanitizing fully transparent values.

https://reviewboard.mozilla.org/r/231688/#review237256


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/test/browser/browser_ext_themes_sanitization.js:16
(Diff revision 1)
> +          "accentcolor": ACCENT_COLOR,
> +          "textcolor": TEXT_COLOR,
> +          "toolbar_bottom_separator": "ntimsfavoriteblue",
> +        },
> +      },
> +    }

Error: Missing trailing comma. [eslint: comma-dangle]

::: toolkit/components/extensions/test/browser/browser_ext_themes_sanitization.js:42
(Diff revision 1)
> +          "accentcolor": ACCENT_COLOR,
> +          "textcolor": TEXT_COLOR,
> +          "toolbar_bottom_separator": "var(--arrowpanel-dimmed)",
> +        },
> +      },
> +    }

Error: Missing trailing comma. [eslint: comma-dangle]

::: toolkit/components/extensions/test/browser/browser_ext_themes_sanitization.js:69
(Diff revision 1)
> +          "textcolor": TEXT_COLOR,
> +          "toolbar_top_separator": "transparent",
> +          "toolbar_bottom_separator": "transparent",
> +        },
> +      },
> +    }

Error: Missing trailing comma. [eslint: comma-dangle]

Comment 3

[Tim Nguyen :ntim]

Comment on attachment 8962882 [details]
Bug 1449327 - Stop sanitizing fully transparent values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231688/diff/1-2/

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8962882 [details]
Bug 1449327 - Stop sanitizing fully transparent values.

https://reviewboard.mozilla.org/r/231688/#review238332

::: toolkit/modules/LightweightThemeConsumer.jsm
(Diff revision 2)
>    let span = doc.createElementNS("http://www.w3.org/1999/xhtml", "span");
>    span.style.color = cssColor;
>    cssColor = doc.defaultView.getComputedStyle(span).color;
> -  if (cssColor == "rgba(0, 0, 0, 0)") {
> -    return null;
> -  }

Looks like this would regress bug 1404386.

Comment 5

[Tim Nguyen :ntim]

There's some code in processColor for the accentcolor map item that prevents this from happening.

https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/LightweightThemeConsumer.jsm#18-20

Comment 6

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim from comment #5)
> There's some code in processColor for the accentcolor map item that prevents
> this from happening.
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/
> LightweightThemeConsumer.jsm#18-20

That would make black which I don't think we want.

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8962882 [details]
Bug 1449327 - Stop sanitizing fully transparent values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231688/diff/2-3/

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8962882 [details]
Bug 1449327 - Stop sanitizing fully transparent values.

https://reviewboard.mozilla.org/r/231688/#review238338

::: toolkit/modules/LightweightThemeConsumer.jsm:35
(Diff revision 3)
>        }
>        const {r, g, b, a} = rgbaChannels;
>        const luminance = 0.2125 * r + 0.7154 * g + 0.0721 * b;
>        element.setAttribute("lwthemetextcolor", luminance <= 110 ? "dark" : "bright");
>        element.setAttribute("lwtheme", "true");
>        return `rgba(${r}, ${g}, ${b}, ${a})` || "black";

Sigh... This fallback doesn't work either. "black" will never be used here. Can you please file a new bug on this?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d0fc1d12d9a
Stop sanitizing fully transparent values. r=dao

Comment 11

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #9)
> Sigh... This fallback doesn't work either. "black" will never be used here.
> Can you please file a new bug on this?

Filed bug 1453035

Comment 12

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 9d0fc1d12d9a (bug 1449327) for browser-chrome failures at toolkit/components/extensions/test/browser/browser_ext_themes_sanitization.js

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9d0fc1d12d9a29d989df38bf74747490a86b114f

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=172911896&repo=autoland&lineNumber=5175

Backout: https://hg.mozilla.org/integration/autoland/rev/60de60fe27339fc2dcf5f47a31a68c7e6bdd58b0

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8962882 [details]
Bug 1449327 - Stop sanitizing fully transparent values.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231688/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2865f00b0d43
Stop sanitizing fully transparent values. r=dao

Comment 15

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/2865f00b0d43