1333953 - Theming API - validate colors in the schema

Status: RESOLVED FIXED

(WebExtensions :: Themes, defect, P2)

Description

[Matthew Wein [:mattw]]

The engineering plan mentions supporting a wide range of color values that we'll eventually support. The way we currently handle this is by accepting arbitrary strings and then validating them in the script. However, it would be more efficient and somewhat less prone to bugs if we could catch these errors during schema validation.

Based on the engineering document, at least the following values should be supported:

 - 3-digit and 6-digit hex values
 - rgb and rgba values
 - hsl and hsla values

Comment 1

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

Right now we don't have any special validation for the colors, but if they're not of the 3- or 4-length array type that Chrome uses, then they will get validated as a CSS color when they're applied. If they aren't a valid CSS color, then no value will get applied for that property. There isn't any explicit error presented to the theme author.

This doesn't seem like something that we should block enabling on Nightly for. I'll remove this from blocking that bug.

Comment 2

[Andrew Swan [:aswan]]

mass move of existing themes bugs to new WebExtensions: Themes component

Comment 3

[Tim Nguyen :ntim]

P2, because right now we expose internal CSS variables to themes.

Comment 4

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

The schema for themes is located at /toolkit/components/extensions/schemas/theme.json and the code to validate it would be embedded in the schema. We already have some validation that you can see there for the ThemeColor, but we also allow a String as a choice which is what this bug is concerned about.

For string names we should probably only support the color names listed in https://searchfox.org/mozilla-central/source/gfx/src/nsColorNameList.h. Unfortunately I don't see a good way for us to include that list during build time so we may need to duplicate it. It would be nice to have a test that tries all of the values we support, this way the test would fail if a new color name gets introduced. I don't know of an API to get those color names though.

In addition, we should make sure that we support each of these values:

 - 3-digit, 6-digit, and 8-digit (new since this bug was filed) hex values
 - rgb and rgba values
 - hsl and hsla values

Comment 5

[anejaalisha]

I largely understood what needs to be done, but where do you want me to place the duplicated file for the color names?

Comment 6

[Tim Nguyen :ntim]

It's probably not a new file, just a list of colors in a regex pattern.

Comment 7

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

Yeah, I agree with comment 6. It would just be located inside of theme.json.

Comment 8

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

Sorry for the back and forth here, but I talked more with Tim and we've decided to simplify this a bit.

Instead of adding one long regex pattern to theme.json, we can allow all string values to pass through the schema, and then use _sanitizeCSSColor to convert color names to their proper RGBA values. We'll want to change the function so it doesn't strip the alpha component, possibly adding a boolean argument that can be passed in to remove the alpha component and updating the one callsite to use that argument.

Then we can use regex for the other types of validation.

Comment 9

[anejaalisha]

Okay, that makes sense. Thanks!

Comment 10

[anejaalisha]

If we are verifying the names of colors in _sanitizeCSSColor, how should I get the color names? Should it be hardcoded in /toolkit/modules/LightweightThemeConsumer.jsm or dynamically load from https://searchfox.org/mozilla-central/source/gfx/src/nsColorNameList.h?

Comment 11

[Tim Nguyen :ntim]

(In reply to anejaalisha from comment #10)
> If we are verifying the names of colors in _sanitizeCSSColor, how should I
> get the color names? Should it be hardcoded in
> /toolkit/modules/LightweightThemeConsumer.jsm or dynamically load from
> https://searchfox.org/mozilla-central/source/gfx/src/nsColorNameList.h?

getComputedStyle should convert the colors to rgba automatically, getting rid of the names.

Comment 12

[anejaalisha]

Attachment: Bug1333953.patch

I probably just want to make sure if my understanding is correct. In the function call, I have hardcoded 'true' for now to take your feedback. I'll change it later.

Am I on the right track?

Comment 13

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

Comment on attachment 8950079 [details] [diff] [review]
Bug1333953.patch

Review of attachment 8950079 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, that's the right idea.

Comment 14

[Tim Nguyen :ntim]

Comment on attachment 8950079 [details] [diff] [review]
Bug1333953.patch

Review of attachment 8950079 [details] [diff] [review]:
-----------------------------------------------------------------

You'll also need to use _sanitizeCSSColor in other calls of _setProperty as well, with alpha set to true.

::: toolkit/modules/LightweightThemeConsumer.jsm
@@ +133,5 @@
>      root.style.removeProperty("--lwt-text-color");
>      root.style.removeProperty("--lwt-accent-color");
>      let textcolor = aData.textcolor || "black";
>      _setProperty(root, active, "--lwt-text-color", textcolor);
> +    _setProperty(root, active, "--lwt-accent-color", this._sanitizeCSSColor(aData.accentcolor, true) || "white");

This should probably have the alpha parameter set to false.

Comment 15

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

anejaalisha, have you been able to make any progress on this?

Comment 16

[Tim Nguyen :ntim]

I might actually be tackling this in bug 1417883 (the second patch)

Comment 17

[Tim Nguyen :ntim]

Fixed by the refactor in bug 1417883. Bug 1449327 is adding tests for sanitization.