Bug 189485 – Web Inspector: change WI.ColorWheel to use conic-gradient()

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Now that conic-gradient() is supported, we don't need to have three different canvases to generate the color wheel, especially since we don't actually use the pixel values of the canvas when we generate the resulting color value.

[Devin Rousso]

Created attachment 349338 [details]
Patch

[Devin Rousso]

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

[Matt Baker]

Comment on attachment 349338 [details]
Patch

r-, because it looks like this patch changes the behavior of the wheel. For example, try the following in shipping Safari and with this patch applied:

1. Bring up the color picker
2. Enter 255 for each channel, in order: R, then, G, then B.
3. Note the "crosshairs" location, and color wheel appearance.

See attached screenshots.

[Matt Baker]

Created attachment 349364 [details]
[Image] ColorWheel in shipping Safari

[Matt Baker]

Created attachment 349365 [details]
[Image] ColorWheel w/ patch applied

[Devin Rousso]

(In reply to Matt Baker from comment #3)
> r-, because it looks like this patch changes the behavior of the wheel. For
> example, try the following in shipping Safari and with this patch applied:
> 
> 1. Bring up the color picker
> 2. Enter 255 for each channel, in order: R, then, G, then B.
> 3. Note the "crosshairs" location, and color wheel appearance.
> 
> See attached screenshots.
I definitely should've added this to the ChangeLog, but this patch does definitively change the functionality of `WI.ColorWheel`.  Our previous implementation used a RGB-esque color space, whereas this version follows an HSL color space.  I personally find HSL to be easier to understand over RGB.  In the RGB version, there is no clear explanation as to how the individual R, G, and B values map onto the visible colors, except that there are three "hot corners" for each of red, green, and blue.  The HSL version, however, is much more straightforward to how HSL works, in that H is represented by the angle around the center, S is how far from the center, and L is the slider on the right.  Additionally, this vastly simplifies the logic for determining the color from a position inside `WI.ColorWheel`.  I personally think that this is a much clearer and simpler version.

[Blaze Burg]

+1 to replacing with HSL-based picker. However, you should retitle the bug, if not at least mention it in the ChangeLog. This is something we'll need to update documentation for.

[Matt Baker]

(In reply to Devin Rousso from comment #6)
> (In reply to Matt Baker from comment #3)
> > r-, because it looks like this patch changes the behavior of the wheel. For
> > example, try the following in shipping Safari and with this patch applied:
> > 
> > 1. Bring up the color picker
> > 2. Enter 255 for each channel, in order: R, then, G, then B.
> > 3. Note the "crosshairs" location, and color wheel appearance.
> > 
> > See attached screenshots.
> I definitely should've added this to the ChangeLog, but this patch does
> definitively change the functionality of `WI.ColorWheel`.  Our previous
> implementation used a RGB-esque color space, whereas this version follows an
> HSL color space.  I personally find HSL to be easier to understand over RGB.
> In the RGB version, there is no clear explanation as to how the individual
> R, G, and B values map onto the visible colors, except that there are three
> "hot corners" for each of red, green, and blue.  The HSL version, however,
> is much more straightforward to how HSL works, in that H is represented by
> the angle around the center, S is how far from the center, and L is the
> slider on the right.  Additionally, this vastly simplifies the logic for
> determining the color from a position inside `WI.ColorWheel`.  I personally
> think that this is a much clearer and simpler version.

I agree HSL is easier to conceptualize. Do the input labels need to be updated to H, S, L?

[Devin Rousso]

(In reply to Matt Baker from comment #8)
> I agree HSL is easier to conceptualize. Do the input labels need to be
> updated to H, S, L?
The input labels are based on the current format of the color text, not the `WI.ColorWheel` (e.g. if you click on `rgb(...)`, it'll show R, G, and B inputs, whereas clicking on `hsla()` will show H, S, L, and A inputs).

[Devin Rousso]

Created attachment 353115 [details]
Patch

[Blaze Burg]

Comment on attachment 353115 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:104
> +            WI.ColorPicker._supportsConicGradient = window.getComputedStyle(element).getPropertyValue(property).includes(conicGradient);

You are clever.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:234
> +            brightnessGradient = "linear-gradient(90deg, black, " + rawColor + ", white)";

Nit; Using template literals would be nice here

> Source/WebInspectorUI/UserInterface/Views/ColorWheel.js:41
> +        this._crosshair = this._element.appendChild(document.createElement("div"));

Nit: _crosshairElement

> Source/WebInspectorUI/UserInterface/Views/ColorWheel.js:157
> +        var point = window.webkitConvertPointFromPageToNode(this._gradientElement, new WebKitPoint(event.pageX, event.pageY));

I've never heard of this before.

> Source/WebInspectorUI/UserInterface/Views/ColorWheel.js:169
> +        if (center.distance(point) > radius) {

It would be nice to add a comment that this branch handles snapping to the edge when the event is outside of the picker circle.

[Devin Rousso]

Comment on attachment 353115 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ColorWheel.js:157
>> +        var point = window.webkitConvertPointFromPageToNode(this._gradientElement, new WebKitPoint(event.pageX, event.pageY));
> 
> I've never heard of this before.

¯\_(ツ)_/¯ <https://developer.apple.com/documentation/webkitjs/domwindow/1631559-webkitconvertpointfrompagetonode>

This was what was used before :P

[Devin Rousso]

Created attachment 353410 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 353410 [details]
Patch

Clearing flags on attachment: 353410

Committed r237605: <https://trac.webkit.org/changeset/237605>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45682730>