WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
203928
Web Inspector: Show RGBA input fields for p3 color picker
https://bugs.webkit.org/show_bug.cgi?id=203928
Summary
Web Inspector: Show RGBA input fields for p3 color picker
Nikita Vasilyev
Reported
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)`.
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2019-11-06 16:17:56 PST
<
rdar://problem/56963805
>
Nikita Vasilyev
Comment 2
2020-01-02 20:39:27 PST
Created
attachment 386655
[details]
Patch
Nikita Vasilyev
Comment 3
2020-01-02 20:54:23 PST
Created
attachment 386657
[details]
Patch
Nikita Vasilyev
Comment 4
2020-01-02 20:54:46 PST
Created
attachment 386658
[details]
[Image] With patch applied
Nikita Vasilyev
Comment 5
2020-01-03 12:07:50 PST
Created
attachment 386704
[details]
Patch
Blaze Burg
Comment 6
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
Devin Rousso
Comment 7
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?
Nikita Vasilyev
Comment 8
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.
Nikita Vasilyev
Comment 9
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 :/
Nikita Vasilyev
Comment 10
2020-01-08 17:21:59 PST
Created
attachment 387167
[details]
Patch
WebKit Commit Bot
Comment 11
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
>
WebKit Commit Bot
Comment 12
2020-01-08 18:00:14 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug