Bug 203928 – Web Inspector: Show RGBA input fields for p3 color picker

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

[Nikita Vasilyev]

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

[Radar WebKit Bug Importer]

<rdar://problem/56963805>

[Nikita Vasilyev]

Created attachment 386655 [details]
Patch

[Nikita Vasilyev]

Created attachment 386657 [details]
Patch

[Nikita Vasilyev]

Created attachment 386658 [details]
[Image] With patch applied

[Nikita Vasilyev]

Created attachment 386704 [details]
Patch

[Blaze Burg]

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

[Devin Rousso]

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?

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

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 :/

[Nikita Vasilyev]

Created attachment 387167 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 387167 [details]
Patch

Clearing flags on attachment: 387167

Committed r254243: <https://trac.webkit.org/changeset/254243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.