RESOLVED FIXED 203533
Web Inspector: Outline sRGB-safe areas on P3 color picker
https://bugs.webkit.org/show_bug.cgi?id=203533
Summary Web Inspector: Outline sRGB-safe areas on P3 color picker
Nikita Vasilyev
Reported 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.
Attachments
Patch for bots, not for review (14.41 KB, patch)
2019-11-20 16:34 PST, Nikita Vasilyev
nvasilyev: commit-queue-
Patch for review (14.72 KB, patch)
2019-11-20 17:33 PST, Nikita Vasilyev
bburg: review+
bburg: commit-queue-
[Video] With patch applied (5.73 MB, video/quicktime)
2019-11-20 17:38 PST, Nikita Vasilyev
no flags
Patch for landing (14.76 KB, patch)
2019-11-21 12:00 PST, Nikita Vasilyev
nvasilyev: commit-queue-
Patch for landing (14.81 KB, patch)
2019-11-21 12:26 PST, Nikita Vasilyev
no flags
Radar WebKit Bug Importer
Comment 1 2019-10-28 15:40:05 PDT
Nikita Vasilyev
Comment 2 2019-11-20 16:34:39 PST Comment hidden (obsolete)
Nikita Vasilyev
Comment 3 2019-11-20 17:33:30 PST
Created attachment 384009 [details] Patch for review
Nikita Vasilyev
Comment 4 2019-11-20 17:38:03 PST
Created attachment 384010 [details] [Video] With patch applied
Blaze Burg
Comment 5 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.
Nikita Vasilyev
Comment 6 2019-11-21 12:00:01 PST Comment hidden (obsolete)
Nikita Vasilyev
Comment 7 2019-11-21 12:26:26 PST
Created attachment 384081 [details] Patch for landing
WebKit Commit Bot
Comment 8 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>
WebKit Commit Bot
Comment 9 2019-11-21 13:11:16 PST
All reviewed patches have been landed. Closing bug.
Devin Rousso
Comment 10 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}`
Nikita Vasilyev
Comment 11 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!
Note You need to log in before you can comment on or make changes to this bug.