Fully transparent values no longer work as of bug 1417883

RESOLVED FIXED in Firefox 61

Status

defect
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: ntim, Assigned: ntim)

Tracking

({regression})

unspecified
mozilla61
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60 unaffected, firefox61 fixed)

Details

Attachments

(1 attachment)

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.
Component: Theme → WebExtensions: Themes
Product: Firefox → Toolkit
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]
Priority: -- → P1
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.
Attachment #8962882 - Flags: review?(dao+bmo) → review-
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
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
Duplicate of this bug: 1450673
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?
Attachment #8962882 - Flags: review?(dao+bmo) → review+
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9d0fc1d12d9a
Stop sanitizing fully transparent values. r=dao
(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
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
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2865f00b0d43
Stop sanitizing fully transparent values. r=dao
https://hg.mozilla.org/mozilla-central/rev/2865f00b0d43
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.