Web Inspector: Outline sRGB-safe areas on P3 color picker

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Color picker displayed for P3 colors will include colors outside of sRGB color space.
Outline sRGB area on the color square.

[Radar WebKit Bug Importer]

<rdar://problem/56688057>

[Nikita Vasilyev]

Created attachment 384001 [details]
Patch for bots, not for review

All tests time out on my build for some reason.

[Nikita Vasilyev]

Created attachment 384009 [details]
Patch for review

[Nikita Vasilyev]

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

[BJ Burg]

Comment on attachment 384009 [details]
Patch for review

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:232
> +            this._srgbLabelElement.title = WI.UIString("This line marks the edge of sRGB color space");

Please add a localization key that describes the larger context (i.e., label for a guide within the color picker). As is, a localizer will have no idea how to find this text in the UI.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:256
> +                    // The point is within sRGB.

I think this comment is backwards. If the P3 value is clipped at all in sRGB, then it's not within sRGB.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:263
> +        if (points.lastValue.y < this._dimension * 0.95) {

Nice attention to detail.

[Nikita Vasilyev]

Created attachment 384079 [details]
Patch for landing

[Nikita Vasilyev]

Created attachment 384081 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 384081 [details]
Patch for landing

Clearing flags on attachment: 384081

Committed r252747: <https://trac.webkit.org/changeset/252747>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Devin Rousso]

Comment on attachment 384009 [details]
Patch for review

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

> Source/WebInspectorUI/UserInterface/Models/Color.js:367
> +            for (let i = 0; i < width; i++)

Style: missing `{` and `}`
Style: we prefer `++i`

> Source/WebInspectorUI/UserInterface/Models/Color.js:368
> +                for (let rowIndex = 0; rowIndex < height; rowIndex++)

Style: we prefer `++rowIndex`

> Source/WebInspectorUI/UserInterface/Models/Color.js:377
> +        let matrix = [

Can we use a different variable name that's more descriptive?

NIT: we can use `const`

> Source/WebInspectorUI/UserInterface/Models/Color.js:380
> +            [0.0000000000000000, 0.04511338185890264, 1.043944368900976]

Style: missing trailing `,`

> Source/WebInspectorUI/UserInterface/Models/Color.js:385
> +        matrix = [

Can we use a different variable name that's more descriptive?

> Source/WebInspectorUI/UserInterface/Models/Color.js:398
> +        return rgb.map(function(value) {

Style: use an arrow function

> Source/WebInspectorUI/UserInterface/Models/Color.js:399
> +            if (value < 0.04045)

Are these numbers defined in a spec somewhere?  Can you link to it in a comment?

> Source/WebInspectorUI/UserInterface/Models/Color.js:410
> +        return rgb.map(function(value) {

Ditto (398)

> Source/WebInspectorUI/UserInterface/Models/Color.js:411
> +            if (value > 0.0031308)

Ditto (399)

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:64
> +.color-square .svg-root {

Please use direct descendant selectors.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:73
> +.color-square .srgb-edge {

Ditto

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:80
> +.color-square .srgb-label {

Ditto

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:87
> +.color-square .srgb-label:hover {

Ditto

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:91
> +.color-square .srgb-label:hover + .svg-root > .srgb-edge {

Ditto

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:96
> +    .color-square .srgb-edge {

Ditto

>> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:232
>> +            this._srgbLabelElement.title = WI.UIString("This line marks the edge of sRGB color space");
> 
> Please add a localization key that describes the larger context (i.e., label for a guide within the color picker). As is, a localizer will have no idea how to find this text in the UI.

I personally find it weird that we have the words "this line" in here, as the only way this is visible is if they're hovering their cursor right next to the line, so it's somewhat unnecessary.  I'd just use "Edge of sRGB color space".

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:237
> +            this._element.append(this._svgElement);

NIT: inline this above?  Also, please use `appendChild`.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:247
> +            let value = 100 - (y / this._dimension) * 100;

NIT: please add another set of parenthesis `100 - ((y / this._dimension) * 100)`

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

NIT: can we flip the order of these?  `value < 0 || value > 1`

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:277
> +        for (let point of points) {

NIT: you could destructure `point` as `{x, y}`

[Nikita Vasilyev]

Comment on attachment 384009 [details]
Patch for review

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

Thanks, I'll address these in the follow-up.

>>> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:232
>>> +            this._srgbLabelElement.title = WI.UIString("This line marks the edge of sRGB color space");
>> 
>> Please add a localization key that describes the larger context (i.e., label for a guide within the color picker). As is, a localizer will have no idea how to find this text in the UI.
> 
> I personally find it weird that we have the words "this line" in here, as the only way this is visible is if they're hovering their cursor right next to the line, so it's somewhat unnecessary.  I'd just use "Edge of sRGB color space".

I agree!