Bug 192729 – Web Inspector: conic-gradient color picker doesn't accurately show color when saturation value is not 100%

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

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.

[Radar WebKit Bug Importer]

<rdar://problem/46746815>

[Devin Rousso]

Created attachment 357489 [details]
Patch

[Devin Rousso]

Created attachment 357491 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

This is a pretty large change. Is this fixing a regression (and is therefore very important)?

[Devin Rousso]

(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

[Devin Rousso]

(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).

[Joseph Pecoraro]

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.

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 358000 [details]
Patch

[EWS Watchlist]

Comment on attachment 358000 [details]
Patch

Attachment 358000 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10513780

New failing tests:
inspector/model/color.html

[EWS Watchlist]

Created attachment 358005 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 358000 [details]
Patch

Attachment 358000 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10514089

New failing tests:
inspector/model/color.html

[EWS Watchlist]

Created attachment 358006 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 358013 [details]
Patch

webkit-patch doesn't include anything outside of the current directory -.-

[Joseph Pecoraro]

Comment on attachment 358013 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 358013 [details]
Patch

Clearing flags on attachment: 358013

Committed r239597: <https://trac.webkit.org/changeset/239597>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.