Bug 203533 - Web Inspector: Outline sRGB-safe areas on P3 color picker
Summary: Web Inspector: Outline sRGB-safe areas on P3 color picker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on: 203436
Blocks:
  Show dependency treegraph
 
Reported: 2019-10-28 15:39 PDT by Nikita Vasilyev
Modified: 2019-11-21 14:18 PST (History)
5 users (show)

See Also:


Attachments
Patch for bots, not for review (14.41 KB, patch)
2019-11-20 16:34 PST, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch for review (14.72 KB, patch)
2019-11-20 17:33 PST, Nikita Vasilyev
bburg: review+
bburg: commit-queue-
Details | Formatted Diff | Diff
[Video] With patch applied (5.73 MB, video/quicktime)
2019-11-20 17:38 PST, Nikita Vasilyev
no flags Details
Patch for landing (14.76 KB, patch)
2019-11-21 12:00 PST, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (14.81 KB, patch)
2019-11-21 12:26 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2019-10-28 15:39:54 PDT
Color picker displayed for P3 colors will include colors outside of sRGB color space.
Outline sRGB area on the color square.
Comment 1 Radar WebKit Bug Importer 2019-10-28 15:40:05 PDT
<rdar://problem/56688057>
Comment 2 Nikita Vasilyev 2019-11-20 16:34:39 PST Comment hidden (obsolete)
Comment 3 Nikita Vasilyev 2019-11-20 17:33:30 PST
Created attachment 384009 [details]
Patch for review
Comment 4 Nikita Vasilyev 2019-11-20 17:38:03 PST
Created attachment 384010 [details]
[Video] With patch applied
Comment 5 Brian Burg 2019-11-21 11:35:24 PST
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.
Comment 6 Nikita Vasilyev 2019-11-21 12:00:01 PST Comment hidden (obsolete)
Comment 7 Nikita Vasilyev 2019-11-21 12:26:26 PST
Created attachment 384081 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2019-11-21 13:11:14 PST
Comment on attachment 384081 [details]
Patch for landing

Clearing flags on attachment: 384081

Committed r252747: <https://trac.webkit.org/changeset/252747>
Comment 9 WebKit Commit Bot 2019-11-21 13:11:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Devin Rousso 2019-11-21 14:02:03 PST
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}`
Comment 11 Nikita Vasilyev 2019-11-21 14:18:18 PST
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!