Bug 203439 – Web Inspector: Display color swatches for p3 colors

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Currently, Web Inspector doesn't show color swatches for p3 colors in the style editor. It should!

For example,

    color(display-p3 0 1 0)

should have have a bright green square before it.

[Radar WebKit Bug Importer]

<rdar://problem/56637250>

[Nikita Vasilyev]

Created attachment 381980 [details]
Patch

[Nikita Vasilyev]

Page with P3 colors: https://codepen.io/NV/pen/zVeewL?editors=1100

[Nikita Vasilyev]

Created attachment 381981 [details]
[Image] With patch applied

[Devin Rousso]

Comment on attachment 381980 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381980&action=review

r-, we should have tests for this.  You can add them to 'inspector/model/color.html'.

> Source/WebInspectorUI/UserInterface/Models/Color.js:51
> +        const matchRegExp = /^(?:#(?<hex>[0-9a-f]{3,8})|rgba?\((?<rgb>[^)]+)\)|(?<keyword>\w+)|color\(display-p3\s(?<p3>[^)]+)\)|hsla?\((?<hsl>[^)]+)\))$/i;

It looks like we also support `srgb`.  We should make this more general to support all the color spaces of the `color(...)` function.

> Source/WebInspectorUI/UserInterface/Models/Color.js:584
> +        return `color(display-p3 ${r} ${g} ${b} ${alpha})`;

There should be a `/` before the `alpha`.
```
    return `color(display-p3 ${r} ${g} ${b} / ${alpha})`; 
```

[Nikita Vasilyev]

Created attachment 382384 [details]
Patch

[Devin Rousso]

Comment on attachment 382384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382384&action=review

> LayoutTests/inspector/model/color.html:240
> +            testGood("color(display-p3 999 999 999/0.5)", WI.Color.Format.Function);

Shouldn't the alpha for this also be `999`?

> LayoutTests/inspector/model/color.html:245
> +            testGood("color(display-p3 999 999 999 / 0.5)", WI.Color.Format.Function);

Ditto

> LayoutTests/inspector/model/color.html:250
> +            testGood("color(display-p3 999 999 999/33%)", WI.Color.Format.Function);

Ditto

> LayoutTests/inspector/model/color.html:255
> +            testGood("color(display-p3 999 999 999 / 33%)", WI.Color.Format.Function);

Ditto

> LayoutTests/inspector/model/color.html:324
> +            InspectorTest.expectThat(color.alpha === 1, "the default alpha should be 1.");

These aren't good descriptions.  We should at least include the word "p3" in there somewhere (or better yet the entire color string) like "P3 color should have a default alpha of 1" (or "'color(display-p3 0 1 0.5)' should have a default alpha of 1").

> Source/WebInspectorUI/UserInterface/Models/Color.js:32
> +    constructor(format, components, gamut = "sRGB")

Please move this to be a fallback on the declaration.
```
    this.gamut = gamut || "srgb";
```

> Source/WebInspectorUI/UserInterface/Models/Color.js:177
> +            let alpha = 1;

I think you can simplify this:
```
    let components = splitFunctionString(colorString);
    if (components.length !== 4 && components.length !== 5)
        return null;

    let gamut = components[0].toLowerCase();
    if (gamut !== "srgb" && gamut !== "display-p3")
        return null;

    let alpha = 1;
    if (components.length === 5)
        alpha = parseFunctionAlpha(components[4]);

    function parseFunctionComponent(component) {
        let value = parseFloat(component);
        return Number.constrain(value, 0, 1);
    }

    return new WI.Color(WI.Color.Format.Function, [
        parseFunctionComponent(components[1]),
        parseFunctionComponent(components[2]),
        parseFunctionComponent(components[3]),
        alpha,
    ], gamut);
```

> Source/WebInspectorUI/UserInterface/Models/Color.js:448
> +        if (this.gamut.toLowerCase() !== "srgb")

Why not just make it so that `WI.Color.fromString` calls `toLowerCase` so that we don't need to keep checking it?

> Source/WebInspectorUI/UserInterface/Models/Color.js:590
> +        let [r, g, b, alpha] = this._rgba;

We should still call `this.rgba` just in case this is the first time we're getting the components.

[Nikita Vasilyev]

Comment on attachment 382384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382384&action=review

>> Source/WebInspectorUI/UserInterface/Models/Color.js:32
>> +    constructor(format, components, gamut = "sRGB")
> 
> Please move this to be a fallback on the declaration.
> ```
>     this.gamut = gamut || "srgb";
> ```

Why? Having the default argument makes it immediately clear that `gamut` is optional.

>> Source/WebInspectorUI/UserInterface/Models/Color.js:177
>> +            let alpha = 1;
> 
> I think you can simplify this:
> ```
>     let components = splitFunctionString(colorString);
>     if (components.length !== 4 && components.length !== 5)
>         return null;
> 
>     let gamut = components[0].toLowerCase();
>     if (gamut !== "srgb" && gamut !== "display-p3")
>         return null;
> 
>     let alpha = 1;
>     if (components.length === 5)
>         alpha = parseFunctionAlpha(components[4]);
> 
>     function parseFunctionComponent(component) {
>         let value = parseFloat(component);
>         return Number.constrain(value, 0, 1);
>     }
> 
>     return new WI.Color(WI.Color.Format.Function, [
>         parseFunctionComponent(components[1]),
>         parseFunctionComponent(components[2]),
>         parseFunctionComponent(components[3]),
>         alpha,
>     ], gamut);
> ```

This would parse `color(display-p3 0 1 1 0.5)` as a valid color, but it isn't. It doesn't include `/` before the alpha.

>> Source/WebInspectorUI/UserInterface/Models/Color.js:448
>> +        if (this.gamut.toLowerCase() !== "srgb")
> 
> Why not just make it so that `WI.Color.fromString` calls `toLowerCase` so that we don't need to keep checking it?

Sounds good.

>> Source/WebInspectorUI/UserInterface/Models/Color.js:590
>> +        let [r, g, b, alpha] = this._rgba;
> 
> We should still call `this.rgba` just in case this is the first time we're getting the components.

It must always have `_rgba`.

    get rgba()
    {
        if (!this._rgba)
            this._rgba = this._hslaToRGBA(this._hsla);
        return this._rgba;
    }

The color function doesn't support HSLA.

[Nikita Vasilyev]

Created attachment 382429 [details]
Patch

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #8)
> This would parse `color(display-p3 0 1 1 0.5)` as a valid color, but it
> isn't. It doesn't include `/` before the alpha.

On the other hand, we currently detect `rgb(255 / 255 / 0)` as a color, even though it isn't a valid CSS color. I can go either way here.

[Nikita Vasilyev]

Created attachment 382484 [details]
Patch

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #10)
> (In reply to Nikita Vasilyev from comment #8)
> > This would parse `color(display-p3 0 1 1 0.5)` as a valid color, but it isn't. It doesn't include `/` before the alpha.
> On the other hand, we currently detect `rgb(255 / 255 / 0)` as a color, even though it isn't a valid CSS color. I can go either way here.
Given that we already support invalid (but close to being valid) colors, I think it's fine to do something similar here.  Perhaps we should fix both in a followup.

[Devin Rousso]

Comment on attachment 382384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382384&action=review

>>> Source/WebInspectorUI/UserInterface/Models/Color.js:32
>>> +    constructor(format, components, gamut = "sRGB")
>> 
>> Please move this to be a fallback on the declaration.
>> ```
>>     this.gamut = gamut || "srgb";
>> ```
> 
> Why? Having the default argument makes it immediately clear that `gamut` is optional.

This is just not our style.  One problem with optional parameters is that passing _anything_ will override it, including falsy values like `null` or `undefined`.  We've preferred the fallback approach for this reason.

>>> Source/WebInspectorUI/UserInterface/Models/Color.js:590
>>> +        let [r, g, b, alpha] = this._rgba;
>> 
>> We should still call `this.rgba` just in case this is the first time we're getting the components.
> 
> It must always have `_rgba`.
> 
>     get rgba()
>     {
>         if (!this._rgba)
>             this._rgba = this._hslaToRGBA(this._hsla);
>         return this._rgba;
>     }
> 
> The color function doesn't support HSLA.

I realize that this is the case now, but in the future we'd probably want to support switching formats from something like HSLA, in which case we may not have an `_rgba` yet.  Also, `get rgba` would be a very quick return in that case, so I think we could use it purely on defensive grounds.

[Nikita Vasilyev]

Created attachment 382517 [details]
Patch

[Devin Rousso]

Comment on attachment 382517 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382517&action=review

r=me, nice tests!

Please create a followup bug for supporting `WI.Color.prototype.nextFormat` with `WI.Color.Format.ColorFunction` and add it as a FIXME inside the function's body.  Otherwise, right now, we'd fail an assertion and log an error if a user tries to shift-click on a `color(...)` swatch.

> Source/WebInspectorUI/UserInterface/Models/Color.js:35
> +        this.gamut = gamut || "srgb";

We should pass in `this.gamut` when creating new `WI.Color` inside `WI.Color.prototype.copy` as well.

> Source/WebInspectorUI/UserInterface/Models/Color.js:591
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Models/Color.js:648
> +    Function: "color-format-function",

Rather than just `Function`, can we use something more specific like `ColorFunction`?  Given that rgb/rgba/hsl/hsla are also functions, the more general name could be confusing.

> Source/WebInspectorUI/UserInterface/Models/Color.js:654
> +    "color",

NIT: is there a reason this is after "rgba" instead of just at the end as a new value?  Regardless, we should try to be consistent with the ordering in `WI.Color.Format` (and all the `switch`).

[Nikita Vasilyev]

Created attachment 382594 [details]
Patch

[Devin Rousso]

Comment on attachment 382594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382594&action=review

> Source/WebInspectorUI/UserInterface/Models/Color.js:399
>          case WI.Color.Format.RGBA:

Don't we need a case for `WI.Color.Format.ColorFunction` here too?

[Nikita Vasilyev]

Created attachment 382596 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 382596 [details]
Patch

Clearing flags on attachment: 382596

Committed r251933: <https://trac.webkit.org/changeset/251933>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.