Bug 203928 - Web Inspector: Show RGBA input fields for p3 color picker
Summary: Web Inspector: Show RGBA input fields for p3 color picker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on: 203436
Blocks:
  Show dependency treegraph
 
Reported: 2019-11-06 16:17 PST by Nikita Vasilyev
Modified: 2020-01-08 18:00 PST (History)
5 users (show)

See Also:


Attachments
Patch (11.56 KB, patch)
2020-01-02 20:39 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (11.84 KB, patch)
2020-01-02 20:54 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Image] With patch applied (271.48 KB, image/png)
2020-01-02 20:54 PST, Nikita Vasilyev
no flags Details
Patch (11.78 KB, patch)
2020-01-03 12:07 PST, Nikita Vasilyev
bburg: review+
Details | Formatted Diff | Diff
Patch (15.78 KB, patch)
2020-01-08 17:21 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 Blaze 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.