1423626 - Installing the 'Quantum Lights dynamic theme' extension breaks resizing the window on Windows 10

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Seen with 59.0a1 (2017-12-05) (64-bit) Nightly on Windows 10 and using the theme located at https://addons.mozilla.org/en-US/firefox/addon/quantum-lights-dynamic/ (version 1)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

We need to first find out if this will happen to users on Release 57, which the theme has listed as it's minVersion.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Ran mozregression, and it said:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=82c2eecf82ba820c4593aa4a9749662f7d54d9a7&tochange=c4db9dfba44dc89ac669b314204fc1139dc75543

On Windows 10 Pro, Version 1709 (OS Build 16299.19) 250% zoom.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

[Tracking Requested - why for this release]: Unusable browser after installing the theme (starts by not allowing resizing, maximized windows act as fullscreen) and eventually becomes invisible requiring Safe Mode to disable/uninstall the theme.

Comment 4

[Dão Gottwald [:dao]]

This sounds like bug 1404386.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: theme.xpi - simplified theme which reproduces the bug

Mozregression points to bug 1403824

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

I've confirmed that it's the use of the accentcolor, which is using a rgba value.

Comment 7

[Tim Nguyen :ntim]

Attachment: Bug 1423626 - Sanitize accent color to ignore alpha channel.

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

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Looking at other changesets around the same regression range and I see a couple bugs that look more likely:
bug 1403951 and bug 1366405

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

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

Comment 10

[Tim Nguyen :ntim]

[Tracking Requested - why for this release]: Installing a theme with a rgba accent color makes the browser unusable.

Comment 11

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim from comment #10)
> [Tracking Requested - why for this release]: Installing a theme with a rgba
> accent color makes the browser unusable.

on Windows*

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

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

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

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

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

https://reviewboard.mozilla.org/r/206040/#review211716

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60f434a1d85b
Sanitize accent color to ignore alpha channel. r=jaws

Comment 16

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 60f434a1d85b (bug 1423626) for browser-chrome failures at browser/base/content/test/urlbar/browser_URLBarSetURI.js r=backout on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=60f434a1d85b45aae5a72ce6c536e057bdb2ece7&group_state=expanded&selectedJob=150409118

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

Backout: https://hg.mozilla.org/integration/autoland/rev/9d6a18731cf00eaac5b45e2ba3dcad0cfacdab0e

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

https://reviewboard.mozilla.org/r/206040/#review211776

::: browser/themes/shared/tabs.inc.css
(Diff revision 4)
>  :root[uidensity=touch] {
>    --tab-min-height: 41px;
>  }
>  
> -:root:-moz-lwtheme {
> -  --tab-line-color: var(--lwt-accent-color);

Why this change?

::: toolkit/modules/LightweightThemeConsumer.jsm:130
(Diff revision 4)
>      // We need to clear these either way: either because the theme is being removed,
>      // or because we are applying a new theme and the data might be bogus CSS,
>      // so if we don't reset first, it'll keep the old value.
>      root.style.removeProperty("--lwt-text-color");
>      root.style.removeProperty("--lwt-accent-color");
> +    root.style.removeProperty("--tab-line-color");

This doesn't belong in this file.

Comment 18

[Tim Nguyen :ntim]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

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

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

https://reviewboard.mozilla.org/r/206040/#review211792

::: toolkit/modules/LightweightThemeConsumer.jsm:182
(Diff revision 5)
>      cssColor = span.style.color;
>      if (cssColor == "transparent" ||
>          cssColor == "rgba(0, 0, 0, 0)") {
>        return "";
>      }
> -    return cssColor;
> +    return `rgb(${_parseRGB(cssColor).join(", ")})`;

Please document that you're removing the alpha channel here.

Comment 20

[Tim Nguyen :ntim]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206040/diff/5-6/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92f0f022897e
Sanitize accent color to ignore alpha channel. r=jaws

Comment 22

[Andreea Pavel [:apavel]]

Backed out for failing browser chrome toolkit/components/extensions/test/browser/browser_ext_themes_chromeparity.js r=backout on a CLOSED TREE

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

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

Backout: https://hg.mozilla.org/integration/autoland/rev/0759cc0b25d7819356d5320058a9764e920b5460

Comment 23

[Tim Nguyen :ntim]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206040/diff/6-7/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69c0d6c48015
Sanitize accent color to ignore alpha channel. r=jaws

Comment 25

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/69c0d6c48015

Comment 26

[Jim Jeffery not reading bug-mail 1/2/11]

This bug appears to have broken the supplied 'Default' theimes in latest Nightly build.

Ran Mozregression tool and its pointing to this bug.

Default theme is OK I think.
Light theme is not 'Light' anymore, black tab strip, text blacked out, Hamburger button and others blacked out.
Dark theme looks like the Light theme, in other words no difference in action.

Customize is broken totally, only displays blank panel. 

This should be backed out ASAP as the latest Nightly is this state is unusable.

Comment 27

[Raul Gurzau (:RaulG)]

Backed out changeset 69c0d6c48015 (bug 1423626) on request for filled regressions on a CLOSED TREE

Backout: https://hg.mozilla.org/integration/autoland/rev/47334da8aed2f88c2607d76607ad67166d262498

Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=69c0d6c48015c05e5db1e39d02957d6d783e1288

Comment 28

[Panos Astithas (he/him) [:past] (please ni?)]

Error from bug 1424456 sheds some light:

TypeError: rgb is null[Learn More] LightweightThemeConsumer.jsm:211:3

Comment 29

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/3a33e3beb0cd
Backout revived test file of bug 1423626. r=merge-fix a=merge-fix

Comment 30

[Julien Cristau [:jcristau]]

Tim, are you working on this still?

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

https://reviewboard.mozilla.org/r/206040/#review213532

::: toolkit/modules/LightweightThemeConsumer.jsm:137
(Diff revision 7)
>      _setProperty(root, active, "--lwt-accent-color", this._sanitizeCSSColor(aData.accentcolor) || "white");
> +
>      if (active) {
>        let dummy = this._doc.createElement("dummy");
>        dummy.style.color = textcolor;
>        let [r, g, b] = _parseRGB(this._doc.defaultView.getComputedStyle(dummy).color);

While you are making these changes can you replace `this._doc.defaultView` with `this._win`?

::: toolkit/modules/LightweightThemeConsumer.jsm:177
(Diff revision 7)
>    _sanitizeCSSColor(cssColor) {
>      // style.color normalizes color values and rejects invalid ones, so a
>      // simple round trip gets us a sanitized color value.
>      let span = this._doc.createElementNS("http://www.w3.org/1999/xhtml", "span");
>      span.style.color = cssColor;
>      cssColor = span.style.color;

This line needs to be using getComputedStyle to convert from color names (such as "black") to "rgb(0, 0, 0)".

Then you can remove the line below that compares against "transparent" since computed style will return rgba(0, 0, 0, 0) for transparent.

Comment 32

[Tim Nguyen :ntim]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206040/diff/7-8/

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d54913a8826
Sanitize accent color to ignore alpha channel. r=jaws

Comment 34

[Cosmin Sabou [:CosminS]]

Backed out for ESlint failure on toolkit/components/extensions/test/browser/browser_ext_themes_alpha_accentcolor.js:27:1

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5d54913a8826a8f44e4859ab24ed3d798b4fe128&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=151866289

https://treeherder.mozilla.org/logviewer.html#?job_id=151868507&repo=autoland&lineNumber=227

https://hg.mozilla.org/integration/autoland/rev/ed12e55558f7c2cdd6c08d186c4fcc7b9aa72ec3

Comment 35

[Tim Nguyen :ntim]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206040/diff/8-9/

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd25e7aba003
Sanitize accent color to ignore alpha channel. r=jaws

Comment 37

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/dd25e7aba003

Comment 38

[Julien Cristau [:jcristau]]

Please request uplift to beta when you get a chance.

Comment 39

[Tim Nguyen :ntim]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

Approval Request Comment
[Feature/Bug causing the regression]: theme api
[User impact if declined]: Broken experience on Windows when using themes with alpha accent color
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no, only local testing
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: small parsing change in themes
[String changes made/needed]: none

Comment 40

[marius.santa]

Attachment: resize working.gif

I can reproduce this issue on Firefox 58.0b12 (20171218174357) under Wind 10 64-bit. 

This issue is verified as fixed on Firefox 59.0a1 (20171221100108) under Wind 10 64-bit and Mac OS X 10.13.

Comment 41

[Gerry Chang [:gchang]]

Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

Fix a broken resizing window issue caused by other extension. Beta58+.

Comment 42

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7e335e4d4a5f

Comment 43

[Florian Quèze [:florian]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #31)

> ::: toolkit/modules/LightweightThemeConsumer.jsm:177
> (Diff revision 7)
> >    _sanitizeCSSColor(cssColor) {
> >      // style.color normalizes color values and rejects invalid ones, so a
> >      // simple round trip gets us a sanitized color value.
> >      let span = this._doc.createElementNS("http://www.w3.org/1999/xhtml", "span");
> >      span.style.color = cssColor;
> >      cssColor = span.style.color;
> 
> This line needs to be using getComputedStyle to convert from color names
> (such as "black") to "rgb(0, 0, 0)".

This introduces a style flush before first paint :-(. https://perfht.ml/2D3XiWJ