Bug 203436 – Web Inspector: Display color picker for p3 colors

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

[Nikita Vasilyev]

Currently, Web Inspector doesn't show color swatches (and thus color picker) for `color(display-p3 ...)`.

- Detect colors defined via `color` function as colors. Currently, Web Inspector detects `rgb()`, `hsl()`, and etc, but doesn't getect `color()`.
- Modify color picker to show `display-p3` color space for `color(display-p3 ...)` colors. Currenly, the color picker only includes sRGB color space.

[Radar WebKit Bug Importer]

<rdar://problem/56635062>

[Nikita Vasilyev]

Created attachment 382758 [details]
Patch

[Nikita Vasilyev]

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

[Blaze Burg]

Comment on attachment 382758 [details]
Patch

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

r=me with a suggestion

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

We generally do not name enum values in lowercase-only. It would work out better I think, to have the enum value be the same, but change the name to `WI.Color.Gamut.(SRGB|HSL)`.

> Source/WebInspectorUI/UserInterface/Models/Color.js:181
> +            if (!WI.Color.Gamut.hasOwnProperty(gamut))

...then this line can be more robust:

 if (!WI.Color.Gamut.fromString(gamut))

[Devin Rousso]

Comment on attachment 382758 [details]
Patch

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

We should also show the R, G, B, and A input fields for p3 colors.

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

We should assert that `gamut` is an expected value if provided.

```
    console.assert(gamut === undefined || Object.values(WI.Color.Gamut).includes(gamut));
```

> Source/WebInspectorUI/UserInterface/Models/Color.js:181
> +            if (!WI.Color.Gamut.hasOwnProperty(gamut))

We should use the value, not the key.
```
    if (!Object.values(WI.Color.Gamut).includes(gamut))
```

> Source/WebInspectorUI/UserInterface/Models/Color.js:320
> +                h = (g - b) / delta + (g < b ? 6 : 0);

NIT: I'd wrap `g < b` in another set of parenthesis so it's completely clear what the condition of the ternary is.
NIT: I'd wrap `(g - b) / delta` in another set of parenthesis so it's completely clear.

> Source/WebInspectorUI/UserInterface/Models/Color.js:323
> +                h = (b - r) / delta + 2;

Ditto (320)

> Source/WebInspectorUI/UserInterface/Models/Color.js:326
> +                h = (r - g) / delta + 4;

Ditto (320)

> Source/WebInspectorUI/UserInterface/Models/Color.js:342
> +        let f = (n) => {

NIT: can we have a better name other than `f`?
NIT: can we have a better name other than `n`?
NIT: does this need to be an arrow function?

> Source/WebInspectorUI/UserInterface/Models/Color.js:343
> +            let k = (n + h / 60) % 6;

Ditto (320)

> Source/WebInspectorUI/UserInterface/Models/Color.js:344
> +            return v - v * s * Math.max(Math.min(k, 4 - k, 1), 0);

Ditto (320)

> Source/WebInspectorUI/UserInterface/Models/Color.js:457
> +                    rgba[3]

Style: missing trailing comma

> Source/WebInspectorUI/UserInterface/Models/Color.js:729
> +    "srgb": "srgb",
> +    "display-p3": "display-p3",

We should use dot-syntax friendly keys.
```
    WI.Color.Gamut = {
        sRGB: "srgb",
        DisplayP3: "display-p3",
    };
```

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:69
> +.color-picker.gamut-p3 > .hue {

We should wrap this inside a `@media (color-gamut: p3) {`

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:53
> +    box-shadow: 0 0 2px black;

Where did this come from?

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:57
> +    --border-width: 1px;

Should we still be using a 0.5px border if supported?

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:86
> +                rgb = rgb.map((x => Math.roundTo(x, 0.001)));

Style: missing parenthesis => `(x)`

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:90
> +                return new WI.Color(WI.Color.Format.HSL, hsl);

Shouldn't this be `this._color =`?

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:95
> +        return this._color;

Are we saving a reference to `this._color` now?  If so, we should add a `this._color = null;` in the constructor and early return inside this function if it's already set.  We could also eliminate `this._hue` and `this._gamut`.

Was this intentional, or was this a copypasta error?

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:215
> +        if (this._crosshairElement)

I don't think it's possible for `this._crosshairElement` to not exist.

[Nikita Vasilyev]

Comment on attachment 382758 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:53
>> +    box-shadow: 0 0 2px black;
> 
> Where did this come from?

I changed the style of the "crosshair". It works better with both light and dark backgrounds, in my opinion.

>> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:95
>> +        return this._color;
> 
> Are we saving a reference to `this._color` now?  If so, we should add a `this._color = null;` in the constructor and early return inside this function if it's already set.  We could also eliminate `this._hue` and `this._gamut`.
> 
> Was this intentional, or was this a copypasta error?

At one iteration I used `this._color` but it's no longer used outside of this method. I'll remove `this._color` — good catch!

[Nikita Vasilyev]

Created attachment 382861 [details]
[Image] Selected color before/after

[Nikita Vasilyev]

Created attachment 382980 [details]
Patch

[Devin Rousso]

Comment on attachment 382980 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Setting.js:215
> +WI.previewFeatures = ["p3-gamut-color-picker"];

Please make a bug for this to be removed and add it as a FIXME.
```
    WI.previewFeatures = [
        "p3-gamut-color-picker", // FIXME: ...
    ];
```

> Source/WebInspectorUI/UserInterface/Models/Color.js:346
> +            let k = (n + h / 60) % 6;
> +            return v - v * s * Math.max(Math.min(k, 4 - k, 1), 0);

NIT: can you also add parenthesis around the operations on these lines too?
```
    let k = (n + (h / 60)) % 6;
    return v - (v * s * Math.max(Math.min(k, 4 - k, 1), 0));
```

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:72
> +        color(display-p3 1 0 0) 0%,

Style: these should be indented

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:107
> +            let y = (1 - hsv[2] / 100) * this._dimension;

Ditto
```
    let y = (1 - (hsv[2] / 100)) * this._dimension;
```

[Nikita Vasilyev]

Created attachment 382988 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 382988 [details]
Patch

Clearing flags on attachment: 382988

Committed r252168: <https://trac.webkit.org/changeset/252168>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.