Bug 203928

Summary: Web Inspector: Show RGBA input fields for p3 color picker
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, hi, inspector-bugzilla-changes, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 203436    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
[Image] With patch applied
none
Patch
bburg: review+
Patch none

Description Nikita Vasilyev 2019-11-06 16:17:34 PST
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)`.
Comment 1 Radar WebKit Bug Importer 2019-11-06 16:17:56 PST
<rdar://problem/56963805>
Comment 2 Nikita Vasilyev 2020-01-02 20:39:27 PST
Created attachment 386655 [details]
Patch
Comment 3 Nikita Vasilyev 2020-01-02 20:54:23 PST
Created attachment 386657 [details]
Patch
Comment 4 Nikita Vasilyev 2020-01-02 20:54:46 PST
Created attachment 386658 [details]
[Image] With patch applied
Comment 5 Nikita Vasilyev 2020-01-03 12:07:50 PST
Created attachment 386704 [details]
Patch
Comment 6 BJ Burg 2020-01-06 09:20:53 PST
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 7 Devin Rousso 2020-01-06 10:36:58 PST
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 8 Nikita Vasilyev 2020-01-06 18:03:01 PST
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 9 Nikita Vasilyev 2020-01-06 18:19:29 PST
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 :/
Comment 10 Nikita Vasilyev 2020-01-08 17:21:59 PST
Created attachment 387167 [details]
Patch
Comment 11 WebKit Commit Bot 2020-01-08 18:00:12 PST
Comment on attachment 387167 [details]
Patch

Clearing flags on attachment: 387167

Committed r254243: <https://trac.webkit.org/changeset/254243>
Comment 12 WebKit Commit Bot 2020-01-08 18:00:14 PST
All reviewed patches have been landed.  Closing bug.