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

VERIFIED FIXED in Firefox 58

Status

defect
P1
critical
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: jaws, Assigned: ntim)

Tracking

({regression})

unspecified
mozilla59
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57- wontfix, firefox58- fixed, firefox59 verified)

Details

Attachments

(3 attachments)

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)
We need to first find out if this will happen to users on Release 57, which the theme has listed as it's minVersion.
[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.
See Also: → 1405593
This sounds like bug 1404386.
Summary: Installing the 'Quantum Lights' theme breaks resizing the window on Windows 10 → Installing the 'Quantum Lights dynamic theme' extension breaks resizing the window on Windows 10
I've confirmed that it's the use of the accentcolor, which is using a rgba value.
Assignee

Updated

2 years ago
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Looking at other changesets around the same regression range and I see a couple bugs that look more likely:
bug 1403951 and bug 1366405
Blocks: 1403951, 1366405
No longer blocks: 1403824
Comment hidden (mozreview-request)
Assignee

Comment 10

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

Comment 11

2 years ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 14

2 years ago
mozreview-review
Comment on attachment 8935178 [details]
Bug 1423626 - Sanitize accent color to ignore alpha channel.

https://reviewboard.mozilla.org/r/206040/#review211716
Attachment #8935178 - Flags: review?(jaws) → review+

Comment 15

2 years ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/60f434a1d85b
Sanitize accent color to ignore alpha channel. r=jaws

Comment 17

2 years ago
mozreview-review
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.
Attachment #8935178 - Flags: review-
Comment hidden (mozreview-request)

Comment 19

2 years ago
mozreview-review
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 hidden (mozreview-request)

Comment 21

2 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/92f0f022897e
Sanitize accent color to ignore alpha channel. r=jaws
Comment hidden (mozreview-request)

Comment 24

2 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/69c0d6c48015
Sanitize accent color to ignore alpha channel. r=jaws
Assignee

Updated

2 years ago
Flags: needinfo?(ntim.bugs)

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/69c0d6c48015
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Updated

2 years ago
Depends on: 1424438
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.
No longer depends on: 1424438

Updated

2 years ago
Depends on: 1424438
Error from bug 1424456 sheds some light:

TypeError: rgb is null[Learn More] LightweightThemeConsumer.jsm:211:3
Flags: needinfo?(past) → needinfo?(ntim.bugs)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 29

2 years ago
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/3a33e3beb0cd
Backout revived test file of bug 1423626. r=merge-fix a=merge-fix
Target Milestone: mozilla59 → ---
Tim, are you working on this still?
Reporter

Comment 31

2 years ago
mozreview-review
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 hidden (mozreview-request)

Comment 33

2 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5d54913a8826
Sanitize accent color to ignore alpha channel. r=jaws
Comment hidden (mozreview-request)

Comment 36

2 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dd25e7aba003
Sanitize accent color to ignore alpha channel. r=jaws

Comment 37

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd25e7aba003
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Please request uplift to beta when you get a chance.
Assignee

Comment 39

2 years ago
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
Flags: needinfo?(ntim.bugs)
Attachment #8935178 - Flags: approval-mozilla-beta?

Comment 40

2 years ago
Posted image 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.

Updated

2 years ago
Status: RESOLVED → VERIFIED
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+.
Attachment #8935178 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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

Updated

Last year
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.