Theming API - validate colors in the schema

RESOLVED FIXED in mozilla61

Status

P2
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: mattw, Assigned: ntim, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla61

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: user-story, triaged, [ntim-intern-project])

Attachments

(1 obsolete attachment)

(Reporter)

Description

2 years ago
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
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.
No longer blocks: 1341722
(Reporter)

Updated

2 years ago
Blocks: 1330328
No longer blocks: 1330335
Blocks: 1330335
No longer blocks: 1330328

Updated

2 years ago
Priority: -- → P5
mass move of existing themes bugs to new WebExtensions: Themes component
Component: WebExtensions: Frontend → WebExtensions: Themes
(Assignee)

Comment 3

a year ago
P2, because right now we expose internal CSS variables to themes.
Priority: P5 → P2
Assignee: nobody → anejaalisha
Status: NEW → ASSIGNED
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

a year ago
I largely understood what needs to be done, but where do you want me to place the duplicated file for the color names?
Flags: needinfo?(jaws)
(Assignee)

Comment 6

a year ago
It's probably not a new file, just a list of colors in a regex pattern.
Yeah, I agree with comment 6. It would just be located inside of theme.json.
Flags: needinfo?(jaws)
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

a year ago
Okay, that makes sense. Thanks!

Comment 10

a year ago
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?
Flags: needinfo?(jaws)
(Assignee)

Comment 11

a year ago
(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

a year ago
Created attachment 8950079 [details] [diff] [review]
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?
Flags: needinfo?(ntim.bugs)
Attachment #8950079 - Flags: feedback?(ntim.bugs)
Attachment #8950079 - Flags: feedback?(jaws)
Comment on attachment 8950079 [details] [diff] [review]
Bug1333953.patch

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

Yeah, that's the right idea.
Attachment #8950079 - Flags: feedback?(jaws) → feedback+
(Assignee)

Updated

a year ago
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 14

a year ago
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.
Attachment #8950079 - Flags: feedback?(ntim.bugs)
anejaalisha, have you been able to make any progress on this?
Flags: needinfo?(anejaalisha)
(Assignee)

Comment 16

11 months ago
I might actually be tackling this in bug 1417883 (the second patch)
Flags: needinfo?(anejaalisha)
(Assignee)

Comment 17

11 months ago
Fixed by the refactor in bug 1417883. Bug 1449327 is adding tests for sanitization.
Assignee: anejaalisha → ntim.bugs
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Updated

8 months ago
Product: Toolkit → WebExtensions
Whiteboard: user-story, triaged → user-story, triaged, [ntim-intern-project]
You need to log in before you can comment on or make changes to this bug.