Closed Bug 1423626 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Frontend, defect, P1)

defect

Tracking

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

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 - wontfix
firefox58 - fixed
firefox59 --- verified

People

(Reporter: jaws, Assigned: ntim)

References

Details

(Keywords: regression)

Attachments

(3 files)

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.
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: nobody → ntim.bugs
Status: NEW → ASSIGNED
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
[Tracking Requested - why for this release]: Installing a theme with a rgba accent color makes the browser unusable.
(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 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+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/60f434a1d85b
Sanitize accent color to ignore alpha channel. r=jaws
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 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.
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/92f0f022897e
Sanitize accent color to ignore alpha channel. r=jaws
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/69c0d6c48015
Sanitize accent color to ignore alpha channel. r=jaws
Flags: needinfo?(ntim.bugs)
https://hg.mozilla.org/mozilla-central/rev/69c0d6c48015
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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
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 → ---
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?
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.
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5d54913a8826
Sanitize accent color to ignore alpha channel. r=jaws
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dd25e7aba003
Sanitize accent color to ignore alpha channel. r=jaws
https://hg.mozilla.org/mozilla-central/rev/dd25e7aba003
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Please request uplift to beta when you get a chance.
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?
Attached 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.
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
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.