WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2019-10-28 15:40:05 PDT
<
rdar://problem/56688057
>
Nikita Vasilyev
Comment 2
2019-11-20 16:34:39 PST
Comment hidden (obsolete)
Created
attachment 384001
[details]
Patch for bots, not for review All tests time out on my build for some reason.
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)
Created
attachment 384079
[details]
Patch for landing
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.
Top of Page
Format For Printing
XML
Clone This Bug