Color picker displayed for P3 colors will include colors outside of sRGB color space. Outline sRGB area on the color square.
<rdar://problem/56688057>
Created attachment 384001 [details] Patch for bots, not for review All tests time out on my build for some reason.
Created attachment 384009 [details] Patch for review
Created attachment 384010 [details] [Video] With patch applied
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.
Created attachment 384079 [details] Patch for landing
Created attachment 384081 [details] Patch for landing
Comment on attachment 384081 [details] Patch for landing Clearing flags on attachment: 384081 Committed r252747: <https://trac.webkit.org/changeset/252747>
All reviewed patches have been landed. Closing bug.
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 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!