Web Inspector: Provide UI to convert between sRGB and p3 color spaces

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

- Shift-clicking on "color(display-p3 0 1 0)" should convert the color to sRGB.
- Shift-clicking on sRGB color (e.g. #123456) should cycle through all available formats including p3.

- Right clicking on "color(display-p3 0 1 0)" should include sRGB conversion options. Currently, the names of the context menu items are "Format: HEX", "Format: HSL", "Format: RGB".
  Perhaps we could use some extra clarification here because all these formats are in sRGB color space.
- Shift-clicking on sRGB color (e.g. #123456) should include "Convert from sRGB to P3 [color space]" or something along these lines.

These are several points to consider. Let me know if you disagree with any of them.

[Radar WebKit Bug Importer]

<rdar://problem/56688523>

[Nikita Vasilyev]

Created attachment 384115 [details]
Patch

[Nikita Vasilyev]

Created attachment 384117 [details]
[Video] With patch applied

[Nikita Vasilyev]

Since I can't edit the bug's description, I should mention that the patch works in many ways differently from the bug's description. The changelog file contains the expected behavior.

[Devin Rousso]

Comment on attachment 384115 [details]
Patch

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

One random thing I noticed: we don't correctly recognize `color(display-p3 ...)` as being a color inside source code text editors, which we should definitely support (`createCodeMirrorColorTextMarkers`).

> LayoutTests/inspector/model/color.html:642
> +            testColorConversion(WI.Color.srgbToDisplayP3, [1, 1, 1], [1.0000556999202659, 0.9999925808389954, 0.9999044134265872]);

This seems wrong.  Why would a P3 color have a component greater than 1?  Shouldn't sRGB(1, 1, 1) exactly equal P3(1, 1, 1)?

We should do some rounding after a certain number of decimals (4 is used below) so it's more human readable at known cases like this.

> LayoutTests/inspector/model/color.html:645
> +            testColorConversion(WI.Color.srgbToDisplayP3, [0, 0, 1], [3.468682218543684e-7, -2.0348729370262e-8, 0.9594865943699936]);

Are these values accurate?  They're really not exactly 0?

> Source/WebInspectorUI/UserInterface/Models/Color.js:39
> +        this.alpha = 1;

Please put this in an `else` or use a ternary so we avoid setting the value twice.

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

Style: unnecessary newline

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

Ditto (535)

> Source/WebInspectorUI/UserInterface/Models/Color.js:576
> +            this._normalizedRGB = WI.Color.srgbToDisplayP3(...this.normalizedRGB);

I feel like you're overloading the meaning of `_normalizedRGB`.  In reality, there's the actual `_normalizedRGB` which is a 0-1 representation of the normal 0-255 RGB, and then there's what I'd call `_p3` or `_color` which contains the wider-gamut 0-1 RGB.

Given the "loose" API of `WI.Color` (e.g. `format` is a public member variable, so anyone could modify it without the owner `WI.Color` knowing about it), this is a footgun waiting to happen, such as if someone manually changes the `format` from P3 (ColorFunction) to HSL and gets totally wrong results.  I'd either create a separate member variable for the P3 color components or introduce a get-set for `format` so it can't be changed like that.

> Source/WebInspectorUI/UserInterface/Models/Color.js:703
> +            if (rgb[0] === 0 && rgb[1] === 0 && rgb[2] === 0)

This should also check that `this.alpha === 0`.

Alternatively, you could use `Array.shallowEqual(this.rgba, [0, 0, 0, 0])`.

> Source/WebInspectorUI/UserInterface/Models/Color.js:727
>  

Style: unnecessary newline

Drive-by: we could also remove the `else` and just have the `return`

> Source/WebInspectorUI/UserInterface/Models/Color.js:747
>  

Ditto (727)

> Source/WebInspectorUI/UserInterface/Models/Color.js:758
>  

Ditto (727)

> Source/WebInspectorUI/UserInterface/Models/Color.js:808
> +    _rgbToHSL(rgb)

Why not just eliminate this function entirely and have the callsites use `WI.Color.rgb2hsl(...rgb)` instead?

> Source/WebInspectorUI/UserInterface/Models/Color.js:813
> +    _hslToRGB(hsl)

Ditto (808)

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:204
> +        let opaque = new WI.Color(format, rgb.concat(1), color.gamut).toString();

Why not use the `gamut` variable already created above?

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:232
> +            this._srgbLabelElement.title = WI.UIString("The edge of sRGB color space", "Label for a guide within the color picker");

I think we can drop the "The" and just have it as "Edge of sRGB color space".

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:170
> +                    contextMenu.appendItem(WI.UIString("Cannot convert to sRGB without data loss", "Unable to convert to sRGB gamut (color space)"), null, disabled);
> +                    contextMenu.appendItem(WI.UIString("Convert to sRGB (clamp)", "Convert to sRGB gamut (color space)"), () => {
> +                        value.gamut = WI.Color.Gamut.SRGB;
> +                        this._updateSwatch();
> +                    });
> +                    contextMenu.show();

This is really weird.  I don't think we should show a context menu on shift-click.  We should just change the tooltip to not mention anything about shift-clicking, and/or mention that conversion isn't possible.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:330
> +            contextMenu.appendItem(WI.UIString("Convert from Display-P3 to sRGB"), () => {

Can we just say "Convert to sRGB"?

Shouldn't we say "Clamp to sRGB" if `value.isOutsideSRGB()`?  If the value is a valid sRGB, then shouldn't we show the normal "Format: ..." options?

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:338
>              contextMenu.appendItem(WI.UIString("Format: Keyword"), () => {

What about a "Format: Color" or "Format: Function"?  Someone may want to convert from `rgb(...)` to `color(srgb ...)`.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:376
> +        contextMenu.appendSeparator();

Style: I normally have newlines before and after `appendSeparator`.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:377
> +        contextMenu.appendItem(WI.UIString("Convert from sRGB to Display-P3"), () => {

Ditto (330) => "Convert to Display-P3"

[Nikita Vasilyev]

Comment on attachment 384115 [details]
Patch

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

>> LayoutTests/inspector/model/color.html:642
>> +            testColorConversion(WI.Color.srgbToDisplayP3, [1, 1, 1], [1.0000556999202659, 0.9999925808389954, 0.9999044134265872]);
> 
> This seems wrong.  Why would a P3 color have a component greater than 1?  Shouldn't sRGB(1, 1, 1) exactly equal P3(1, 1, 1)?
> 
> We should do some rounding after a certain number of decimals (4 is used below) so it's more human readable at known cases like this.

I was surprised as well but then I checked ColorSync Utility on macOS and it also produced similar values: [1.0001, 0.9999, 1.0001]. It rounds to 4 decimals.

>> LayoutTests/inspector/model/color.html:645
>> +            testColorConversion(WI.Color.srgbToDisplayP3, [0, 0, 1], [3.468682218543684e-7, -2.0348729370262e-8, 0.9594865943699936]);
> 
> Are these values accurate?  They're really not exactly 0?

Ditto, it matches ColorSync Utility on macOS.

>> Source/WebInspectorUI/UserInterface/Models/Color.js:576
>> +            this._normalizedRGB = WI.Color.srgbToDisplayP3(...this.normalizedRGB);
> 
> I feel like you're overloading the meaning of `_normalizedRGB`.  In reality, there's the actual `_normalizedRGB` which is a 0-1 representation of the normal 0-255 RGB, and then there's what I'd call `_p3` or `_color` which contains the wider-gamut 0-1 RGB.
> 
> Given the "loose" API of `WI.Color` (e.g. `format` is a public member variable, so anyone could modify it without the owner `WI.Color` knowing about it), this is a footgun waiting to happen, such as if someone manually changes the `format` from P3 (ColorFunction) to HSL and gets totally wrong results.  I'd either create a separate member variable for the P3 color components or introduce a get-set for `format` so it can't be changed like that.

I think `_normalizedRGB` is a decent descriptive name. I'm not sold on the proposed alternatives:
`_p3` is misleading if we use `color(srgb 1 0 1)`.
`_color` on Color class is just weird. It would result in code like `color.color` in ColorPicker.

>> Source/WebInspectorUI/UserInterface/Models/Color.js:703
>> +            if (rgb[0] === 0 && rgb[1] === 0 && rgb[2] === 0)
> 
> This should also check that `this.alpha === 0`.
> 
> Alternatively, you could use `Array.shallowEqual(this.rgba, [0, 0, 0, 0])`.

It unnecessary because of `if (!this.simple) {` the line above.

    get simple()
    {
        return this.alpha === 1;
    }

BTW, I think `simple` getter is confusing and it should be either renamed to `opaque` or removed entirely.

>> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:232
>> +            this._srgbLabelElement.title = WI.UIString("The edge of sRGB color space", "Label for a guide within the color picker");
> 
> I think we can drop the "The" and just have it as "Edge of sRGB color space".

Sounds good!

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:170
>> +                    contextMenu.show();
> 
> This is really weird.  I don't think we should show a context menu on shift-click.  We should just change the tooltip to not mention anything about shift-clicking, and/or mention that conversion isn't possible.

This is what Greg and I came up with. I'm personally fine not showing it.

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:330
>> +            contextMenu.appendItem(WI.UIString("Convert from Display-P3 to sRGB"), () => {
> 
> Can we just say "Convert to sRGB"?
> 
> Shouldn't we say "Clamp to sRGB" if `value.isOutsideSRGB()`?  If the value is a valid sRGB, then shouldn't we show the normal "Format: ..." options?

Sounds good.

[Nikita Vasilyev]

Created attachment 384129 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 384115 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:330
>>> +            contextMenu.appendItem(WI.UIString("Convert from Display-P3 to sRGB"), () => {
>> 
>> Can we just say "Convert to sRGB"?
>> 
>> Shouldn't we say "Clamp to sRGB" if `value.isOutsideSRGB()`?  If the value is a valid sRGB, then shouldn't we show the normal "Format: ..." options?
> 
> Sounds good.

Re: If the value is a valid sRGB, then shouldn't we show the normal "Format: ..." options?

This feels like too much "magic". I don't want "Format: ..." options to perform an implicit conversion to a different gamut.

[Devin Rousso]

Comment on attachment 384129 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/Color.js:38
> +        this.alpha = components.length === 4 ? components[3] : 1;

We should add a `console.assert(components.length === 3 || components.length === 4, components);` above.

> Source/WebInspectorUI/UserInterface/Models/Color.js:396
> +        // using sRGBâs own white, D65 (no chromatic adaptation)

Please avoid using non-UTF8 characters in JavaScript files.

> Source/WebInspectorUI/UserInterface/Models/Color.js:600
>          }

Let's add a `console.assert();` at the end of this function just in case.

> Source/WebInspectorUI/UserInterface/Models/Color.js:700
> +            if (rgb[0] === 0 && rgb[1] === 0 && rgb[2] === 0)

If this is `!this.simple`, all that means is that `this.alpha !== 1`.  As such, we should still be checking that `Array.shallowEqual(this.rgba, [0, 0, 0, 0])`.

> Source/WebInspectorUI/UserInterface/Models/Color.js:742
>  

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Models/Color.js:752
>  

Ditto (742)

> Source/WebInspectorUI/UserInterface/Models/Color.js:802
> +    _hslToRGB(hsl)

This doesn't appear to be called.  Please remove it.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:34
> +        this._value = value || this._fallbackValue();

Does this need to move?

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:-48
> -            case WI.InlineSwatch.Type.Color:

Removing this means that we're going to hit the `WI.reportInternalError` in the `default`.  We should keep this `case`, and instead leave a comment explaining why the `title` isn't set here.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:76
> +        this._readOnly = readOnly;

If you do need to move `this._value`, please also move this to be right below the `this._value` assignment and use `this._readOnly` instead of `readOnly`.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:159
> +        return !this._readOnly && !(this.value.gamut === WI.Color.Gamut.DisplayP3 && this.value.isOutsideSRGB());

NIT: I try to avoid using `!(...)` as it's not as easily noticeable compared to actually spreading the `!` out.

Also, I think you can drop the `this.value.gamut === WI.Color.Gamut.DisplayP3` as it's already checked for inside `WI.Color.prototype.isOutsideSRGB`.
```
    return !this._readOnly && !this.value.isOutsideSRGB();
```

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:171
> +                if (!this._allowShiftClickColor()) {
> +                    InspectorFrontendHost.beep();

Nice!  This feels much better :)

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:331
> +            let menuItem = value.isOutsideSRGB() ? WI.UIString("Clamp to sRGB") : WI.UIString("Convert to sRGB");

I think it would be useful to offer lossless formatting options for P3 colors.  As an example, right-clicking on `color(display-p3 1 1 1)` should allow me to "Format: HSL" if I wanted to.  We should only be restrictive when the conversion is lossy.

[Nikita Vasilyev]

Comment on attachment 384129 [details]
Patch

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

Thanks for reviewing!

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:331
>> +            let menuItem = value.isOutsideSRGB() ? WI.UIString("Clamp to sRGB") : WI.UIString("Convert to sRGB");
> 
> I think it would be useful to offer lossless formatting options for P3 colors.  As an example, right-clicking on `color(display-p3 1 1 1)` should allow me to "Format: HSL" if I wanted to.  We should only be restrictive when the conversion is lossy.

Well, `color(display-p3 1 1 1)` converts to `[0.9999, 1, 1.0001]`. Perhaps I should add epsilon to isOutsideSRGB.

[Nikita Vasilyev]

Created attachment 384387 [details]
Patch

[Devin Rousso]

Comment on attachment 384387 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/Color.js:38
> +        this._gamut = gamut || WI.Color.Gamut.SRGB;
> +        console.assert(components.length === 3 || components.length === 4, components);

Style: please add a newline

> Source/WebInspectorUI/UserInterface/Models/Color.js:651
> +        let rgb = this.rgb;

NIT: you could inline this

> Source/WebInspectorUI/UserInterface/Models/Color.js:655
> +    isOutsideSRGB()

This needs some tests.

> Source/WebInspectorUI/UserInterface/Models/Color.js:669
> +        return rgb.some((x) => x < -epsilon || x > 1 + epsilon);

Does the 8bit round up or down?  If it rounds up, then you should use `<=` and `>=`, as `255.50` would round to `256`.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:254
> +                if (srgb.some((value) => value < 0 || value > 1)) {

Do we want to use the same `epsilon` concept in `isOutsideSRGB` here as well?  Or does it not matter as much here since we're just doing a visualization?

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:376
> +                contextMenu.appendItem(WI.UIString("Format: Color Function"), () => {

Should we be more specific in that it's going to "Format: sRGB Color"?

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:393
> +            let menuItem = isColorOutsideSRGB ? WI.UIString("Clamp to sRGB") : WI.UIString("Convert to sRGB");

NIT: you could inline this
NIT: we normally use `label` as the variable name (it matches the signature)

[Nikita Vasilyev]

Comment on attachment 384387 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/Color.js:669
>> +        return rgb.some((x) => x < -epsilon || x > 1 + epsilon);
> 
> Does the 8bit round up or down?  If it rounds up, then you should use `<=` and `>=`, as `255.50` would round to `256`.

It rounds up. `254.50` rounds to `255`.

>> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:254
>> +                if (srgb.some((value) => value < 0 || value > 1)) {
> 
> Do we want to use the same `epsilon` concept in `isOutsideSRGB` here as well?  Or does it not matter as much here since we're just doing a visualization?

The curve is more precise if we don't use the epsilon here.

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:376
>> +                contextMenu.appendItem(WI.UIString("Format: Color Function"), () => {
> 
> Should we be more specific in that it's going to "Format: sRGB Color"?

I don't think we should. HSL, RGB, and HEX formats are all sRGB.

[Nikita Vasilyev]

Created attachment 384638 [details]
Patch

[Devin Rousso]

Comment on attachment 384638 [details]
Patch

r=me, nice work!

[WebKit Commit Bot]

Comment on attachment 384638 [details]
Patch

Clearing flags on attachment: 384638

Committed r253018: <https://trac.webkit.org/changeset/253018>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.