We should RGBA input fields for `rgba(0,255,0,0.5)` and HSL fields for `hsl(240, 30%, 40%)`. We should show RGBA input fields for `color(display-p3 0 1 0 / 0.5)`.
<rdar://problem/56963805>
Created attachment 386655 [details] Patch
Created attachment 386657 [details] Patch
Created attachment 386658 [details] [Image] With patch applied
Created attachment 386704 [details] Patch
Comment on attachment 386704 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386704&action=review r=me, nice cleanup. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:199 > + Nit: extra newline
Comment on attachment 386704 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386704&action=review > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:44 > + this._colorInputsContainerElement.addEventListener("input", this._handleColorInputInput.bind(this)); Please rename the handler function so we know it’s being added to the container and not each input individually. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:-88 > - this._color = WI.Color.fromString("white"); Why was this changed? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:232 > + let [h, s, l] = this._color.hsl; This will have problems since the scope is to the `switch`. Either use `var` or put them outside or add braces. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:233 > + createColorInput("H", h.maxDecimals(1), {max: 360}); Why did this go from 2 to 1? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:260 > + let inputs = this._colorInputsContainerElement.querySelectorAll("input"); Can we avoid this? It seems unnecessary when we could just have a `_colorInputs` array that we populate. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:262 > + this._color = new WI.Color(this._color.format, components, this._color.gamut); Should we assert for a valid color? What happens if they input a non-number?
Comment on attachment 386704 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386704&action=review >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:-88 >> - this._color = WI.Color.fromString("white"); > > Why was this changed? `_handleFormatChange` would not get called for color keywords (such as "transparent") when showing the color picker initially. It would get called for any other format. Now that I look more into this, it doesn't seem to matter. WI.ColorPicker.Event.FormatChanged event is only used to call `popover.update()`. I don't think we need to do this. We show input fields for all color formats now; the popover doesn't need to be resized. >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:233 >> + createColorInput("H", h.maxDecimals(1), {max: 360}); > > Why did this go from 2 to 1? For HSLA values, less than 3 digits fully fit in the field. I don't think my change makes much difference; I don't think it's a good UI either way. >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:262 >> + this._color = new WI.Color(this._color.format, components, this._color.gamut); > > Should we assert for a valid color? What happens if they input a non-number? Good catch. We used to fallback to 0 for non-numbers. My patch displays NaN. I'll make it fallback to 0.
Comment on attachment 386704 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386704&action=review > Source/WebInspectorUI/ChangeLog:25 > + (WI.ColorPicker.prototype._showColorComponentInputs): > + Remove `updateColorInput` function and rebuild DOM every time color changes. > + This simplifies the logic and it isn't expensive - `_showColorComponentInputs` > + takes only 0.5ms on my 2017 MacBook Pro. This has issues. If I'm focused on the input field, rebuilding DOM loses the focus and text selection :/
Created attachment 387167 [details] Patch
Comment on attachment 387167 [details] Patch Clearing flags on attachment: 387167 Committed r254243: <https://trac.webkit.org/changeset/254243>
All reviewed patches have been landed. Closing bug.