Bug 192729

Summary: Web Inspector: conic-gradient color picker doesn't accurately show color when saturation value is not 100%
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 189485    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
[Image] After Patch is applied
none
Patch
none
Archive of layout-test-results from ews100 for mac-sierra
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Patch none

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 EWS Watchlist 2018-12-21 17:14:16 PST Comment hidden (obsolete)
Comment 11 EWS Watchlist 2018-12-21 17:14:18 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2018-12-21 17:48:31 PST Comment hidden (obsolete)
Comment 13 EWS Watchlist 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.