Bug 192729 - Web Inspector: conic-gradient color picker doesn't accurately show color when saturation value is not 100%
Summary: Web Inspector: conic-gradient color picker doesn't accurately show color when...
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: Devin Rousso
URL:
Keywords: InRadar
Depends on: 189485
Blocks:
  Show dependency treegraph
 
Reported: 2018-12-14 16:58 PST by Devin Rousso
Modified: 2019-01-03 13:58 PST (History)
7 users (show)

See Also:


Attachments
Patch (44.99 KB, patch)
2018-12-17 16:00 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (134.87 KB, image/png)
2018-12-17 16:01 PST, Devin Rousso
no flags Details
Patch (30.95 KB, patch)
2018-12-21 16:25 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.71 MB, application/zip)
2018-12-21 17:14 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.46 MB, application/zip)
2018-12-21 17:48 PST, Build Bot
no flags Details
Patch (45.01 KB, patch)
2018-12-21 19:09 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-12-14 16:58:11 PST
The conic-gradient tried to represent hue as the angle, saturation as the distance from the center, and lightness/brightness as the slider to the right.  Using a conic-gradient, however, it is not possible to have the color change between pixels on the edge and pixels towards the middle, meaning that the saturation is constant throughout the preview image, even though the resulting computed value is correct.
Comment 1 Radar WebKit Bug Importer 2018-12-14 16:58:33 PST
<rdar://problem/46746815>
Comment 2 Devin Rousso 2018-12-17 16:00:27 PST
Created attachment 357489 [details]
Patch
Comment 3 Devin Rousso 2018-12-17 16:01:32 PST
Created attachment 357491 [details]
[Image] After Patch is applied
Comment 4 Joseph Pecoraro 2018-12-19 19:34:35 PST
This is a pretty large change. Is this fixing a regression (and is therefore very important)?
Comment 5 Devin Rousso 2018-12-19 22:08:41 PST
(In reply to Joseph Pecoraro from comment #4)
> This is a pretty large change. Is this fixing a regression (and is therefore very important)?
Yes and no.  It does fix the regression
Comment 6 Devin Rousso 2018-12-19 22:10:47 PST
(In reply to Joseph Pecoraro from comment #4)
> This is a pretty large change. Is this fixing a regression (and is therefore very important)?
Yes and no.  It does fix the regression introduced by <https://webkit.org/b/189485> (the description/title of this bug explains the issue), but it is also just as viable to roll out <https://webkit.org/b/189485> instead of trying to "fix the fix".  This patch is an attempt to get the best of both, in that it uses some of the logic of the old color picker (<canvas> drawing vs conic-gradient), but has the look of the new color picker (hsl vs rgb).
Comment 7 Joseph Pecoraro 2018-12-21 16:14:57 PST
Comment on attachment 357489 [details]
Patch

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

This is a prettyOverall this seems fine.

When I applied locally I encountered one issue:

Steps to Reproduce:
1. Trigger color picker for hsl() color
2. Modify the S value manually (up/down arrows)
  => Graph always snaps current value pointer to the edge even though S is not 100%

Same when trying to modify any RGB value manually.

Everything else seems fine.

> Source/WebInspectorUI/UserInterface/Models/Color.js:522
> -        let r = WI.Color._eightBitChannel(this.rgb[0]);
> -        let g = WI.Color._eightBitChannel(this.rgb[1]);
> -        let b = WI.Color._eightBitChannel(this.rgb[2]);
> -        return `rgb(${r}, ${g}, ${b})`;
> +        let rgb = this.rgb.map((value) => WI.Color._eightBitChannel(Math.round(value)));
> +        return `rgb(${rgb[0]}, ${rgb[1]}, ${rgb[2]})`;

I find the earlier code much easier to read. Also it didn't round but your new code does, is that a good idea?

> Source/WebInspectorUI/UserInterface/Models/Color.js:528
> -        let r = WI.Color._eightBitChannel(this.rgb[0]);
> -        let g = WI.Color._eightBitChannel(this.rgb[1]);
> -        let b = WI.Color._eightBitChannel(this.rgb[2]);
> -        return `rgba(${r}, ${g}, ${b}, ${this.alpha})`;
> +        let rgb = this.rgb.map((value) => WI.Color._eightBitChannel(Math.round(value)));
> +        return `rgba(${rgb[0]}, ${rgb[1]}, ${rgb[2]}, ${this.alpha})`;

Ditto.
Comment 8 Devin Rousso 2018-12-21 16:22:17 PST
Comment on attachment 357489 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/Color.js:522
>> +        return `rgb(${rgb[0]}, ${rgb[1]}, ${rgb[2]})`;
> 
> I find the earlier code much easier to read. Also it didn't round but your new code does, is that a good idea?

I believe so, since RGB values are supposed to be integers between [0, 255].  Having a long decimal string suddenly appear doesn't make a lot of sense, especially for a CSS string.

I personally find this easier to read, not to mention it avoids code duplication.
Comment 9 Devin Rousso 2018-12-21 16:25:18 PST
Created attachment 358000 [details]
Patch
Comment 10 Build Bot 2018-12-21 17:14:16 PST Comment hidden (obsolete)
Comment 11 Build Bot 2018-12-21 17:14:18 PST Comment hidden (obsolete)
Comment 12 Build Bot 2018-12-21 17:48:31 PST Comment hidden (obsolete)
Comment 13 Build Bot 2018-12-21 17:48:32 PST Comment hidden (obsolete)
Comment 14 Devin Rousso 2018-12-21 19:09:04 PST
Created attachment 358013 [details]
Patch

webkit-patch doesn't include anything outside of the current directory -.-
Comment 15 Joseph Pecoraro 2019-01-03 11:17:01 PST
Comment on attachment 358013 [details]
Patch

r=me
Comment 16 WebKit Commit Bot 2019-01-03 13:58:31 PST
Comment on attachment 358013 [details]
Patch

Clearing flags on attachment: 358013

Committed r239597: <https://trac.webkit.org/changeset/239597>
Comment 17 WebKit Commit Bot 2019-01-03 13:58:33 PST
All reviewed patches have been landed.  Closing bug.