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.
Created attachment 349338 [details] Patch
Created attachment 349339 [details] [Image] After Patch is applied
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.
Created attachment 349364 [details] [Image] ColorWheel in shipping Safari
Created attachment 349365 [details] [Image] ColorWheel w/ patch applied
(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.
+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.
(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?
(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).
Created attachment 353115 [details] Patch
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.
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
Created attachment 353410 [details] Patch
Comment on attachment 353410 [details] Patch Clearing flags on attachment: 353410 Committed r237605: <https://trac.webkit.org/changeset/237605>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45682730>