Bug 124356

Summary: Web Inspector: color picker should feature an editable CSS value
Product: WebKit Reporter: Antoine Quint <graouts>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, chrisjshull, commit-queue, drousso, graouts, inspector-bugzilla-changes, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 166860    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
[Image] After Patch is applied
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
timothy: review+
Patch none

Description Antoine Quint 2013-11-14 08:06:46 PST
The new color picker introduced with https://bugs.webkit.org/show_bug.cgi?id=124354 does not feature a label showing the generated CSS value. We should not only have this, but also ensure we use the same format as the edited color by default, and make the values editable by click-and-drag.
Comment 1 Radar WebKit Bug Importer 2013-11-14 08:07:11 PST
<rdar://problem/15469602>
Comment 2 Timothy Hatcher 2013-11-14 08:46:49 PST
To be clear, this means separate text fields for the color components (RGBA or HSLA) with each field independently editable. Full text editing of the color is in the rule's text editor, and a non-goal for the popover.
Comment 3 Devin Rousso 2017-01-01 13:51:26 PST
Created attachment 297875 [details]
Patch
Comment 4 Devin Rousso 2017-01-01 14:20:32 PST
Created attachment 297876 [details]
[Image] After Patch is applied
Comment 5 Brian Burg 2017-01-01 22:55:05 PST
Comment on attachment 297875 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297875&action=review

No major issues, but I want to see a v2.

Can you think up some basic tests we would want to write for this? UI tests are not yet in trunk, but in preparation for that, I'd like everyone to start seeding some ideas so we can pick them up easily and write tests when it is possible given our infrastructure.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:44
> +        this._colorInputsContainer = document.createElement("div");

Nit: should end in *Element as it stores a DOM element.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:47
> +        function createColorInput(label, {min, max, step, units} = {min: 0, max: 1, step: 0.01}) {

Neat!

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:52
> +            let input = container.createChild("input");

Nit: ditto

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:60
> +                container.append(units);

It's clearer to append a text node.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:62
> +            return {

No need to add newlines here.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:178
> +        if (format === WebInspector.Color.Format.HSL || format === WebInspector.Color.Format.HSLA) {

Please use a switch on the format.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:207
> +        function updateColorInput(key, value) {

(applies multiple places)

Please call this TextField instead of Input. The latter is a bit too generic.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:208
> +            let {input, container} = this._colorInputs[key];

Please use a Map instead of an object.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:213
> +        for (let {container} of Object.values(this._colorInputs))

Erm, yeah, this will be simpler with a map.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:219
> +            var [r, g, b] = this._color.rgb;

Nit: let

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:227
> +            var [h, s, l] = this._color.hsl;

Ditto

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:271
> +            return;

This should call WebInspector.reportInternalError or something.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:276
> +        this.dispatchEventToListeners(WebInspector.ColorPicker.Event.ColorChanged, {color: this._color});

It seems very strange to me that we use events here, when this widget is only ever used by one client at a time. A delegate would make more sense, IMO. You can leave it like this for now since it's an existing design, though.
Comment 6 Devin Rousso 2017-01-02 07:52:31 PST
Comment on attachment 297875 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297875&action=review

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:60
>> +                container.append(units);
> 
> It's clearer to append a text node.

We do this all over WebInspector code <https://bugs.webkit.org/show_bug.cgi?id=147301>.

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:62
>> +            return {
> 
> No need to add newlines here.

I've been doing this recently because it makes a diff much cleaner if new items are added.  I'll change it.

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:178
>> +        if (format === WebInspector.Color.Format.HSL || format === WebInspector.Color.Format.HSLA) {
> 
> Please use a switch on the format.

I'd actually recommend keeping this as is, because all WI.Color formats (except HSL) use RGB to store the color components (e.g. if the color given is in a HEX format, it will still use RGB to hold the color components).  It would be a bit weird to have a switch with HSL being its own case and then every other format having the same logic.

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:208
>> +            let {input, container} = this._colorInputs[key];
> 
> Please use a Map instead of an object.

So I actually talked with Joe about this, and we both agreed that using an object is clearer since it doesn't ever add/remove after creation.  I'll message you about this next time I am free.

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:213
>> +        for (let {container} of Object.values(this._colorInputs))
> 
> Erm, yeah, this will be simpler with a map.

If I were to do this with a Map, the code would be:

for (let {container} of this._colorInputs.values())
    container.hidden = true;

Personally, I don't see a huge difference between the two and the intentions of both are obvious.  I'll message you about this next time I am free.

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:219
>> +            var [r, g, b] = this._color.rgb;
> 
> Nit: let

I've been told to not use `let` inside switch because of the fact that a switch only has one underlying block.  <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Temporal_dead_zone_and_errors_with_let>

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:271
>> +            return;
> 
> This should call WebInspector.reportInternalError or something.

Really?  Can you point me to an example so I can better understand what you mean?
Comment 7 Devin Rousso 2017-01-02 07:56:42 PST
Comment on attachment 297875 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297875&action=review

I forgot to comment this earlier.  Sorry.

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:207
>> +        function updateColorInput(key, value) {
> 
> (applies multiple places)
> 
> Please call this TextField instead of Input. The latter is a bit too generic.

So I just tried changing this, and found myself typing "textArea" instead of "textField".  I think that using a name so close to another type of DOM element <textarea> may be confusing, since they have different attributes (such as "textarea" having ".textContent")
Comment 8 Devin Rousso 2017-01-02 07:59:12 PST
Created attachment 297898 [details]
Patch
Comment 9 Brian Burg 2017-01-02 10:01:54 PST
Comment on attachment 297875 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297875&action=review

>>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:207
>>> +        function updateColorInput(key, value) {
>> 
>> (applies multiple places)
>> 
>> Please call this TextField instead of Input. The latter is a bit too generic.
> 
> So I just tried changing this, and found myself typing "textArea" instead of "textField".  I think that using a name so close to another type of DOM element <textarea> may be confusing, since they have different attributes (such as "textarea" having ".textContent")

The problem is that an 'input' could refer to a text field, slider, gradient, whatever. I had to look at the screenshot to tell what's going on. Maybe NumberField or NumberInput is an okay compromise.

>>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:208
>>> +            let {input, container} = this._colorInputs[key];
>> 
>> Please use a Map instead of an object.
> 
> So I actually talked with Joe about this, and we both agreed that using an object is clearer since it doesn't ever add/remove after creation.  I'll message you about this next time I am free.

I don't follow your argument. Please elaborate why that's clearer. An object is a bag of properties that can be used as a struct, class, string-key map, or be proxied, whereas a Map is an ADT.

>>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:219
>>> +            var [r, g, b] = this._color.rgb;
>> 
>> Nit: let
> 
> I've been told to not use `let` inside switch because of the fact that a switch only has one underlying block.  <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Temporal_dead_zone_and_errors_with_let>

Oh, interesting. I've

>>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:271
>>> +            return;
>> 
>> This should call WebInspector.reportInternalError or something.
> 
> Really?  Can you point me to an example so I can better understand what you mean?

In most Inspector code we would write console.assert(something), but WebInspector.reportInternalError allows you to include more key/values and will trigger the Uncaught Exception sheet. So I prefer we use that in new code.

(Of course, only do this if this is an unexpected error. It wasn't clear here as there's no assert, but we bail out.)
Comment 10 Brian Burg 2017-01-02 13:21:23 PST
(In reply to comment #9)
> Comment on attachment 297875 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297875&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:207
> >>> +        function updateColorInput(key, value) {
> >> 
> >> (applies multiple places)
> >> 
> >> Please call this TextField instead of Input. The latter is a bit too generic.
> > 
> > So I just tried changing this, and found myself typing "textArea" instead of "textField".  I think that using a name so close to another type of DOM element <textarea> may be confusing, since they have different attributes (such as "textarea" having ".textContent")
> 
> The problem is that an 'input' could refer to a text field, slider,
> gradient, whatever. I had to look at the screenshot to tell what's going on.
> Maybe NumberField or NumberInput is an okay compromise.
> 
> >>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:208
> >>> +            let {input, container} = this._colorInputs[key];
> >> 
> >> Please use a Map instead of an object.
> > 
> > So I actually talked with Joe about this, and we both agreed that using an object is clearer since it doesn't ever add/remove after creation.  I'll message you about this next time I am free.
> 
> I don't follow your argument. Please elaborate why that's clearer. An object
> is a bag of properties that can be used as a struct, class, string-key map,
> or be proxied, whereas a Map is an ADT.
> 
> >>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:219
> >>> +            var [r, g, b] = this._color.rgb;
> >> 
> >> Nit: let
> > 
> > I've been told to not use `let` inside switch because of the fact that a switch only has one underlying block.  <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Temporal_dead_zone_and_errors_with_let>
> 
> Oh, interesting. I've

... updated the style guide with a note about this. Although, you can probably just add an explicit block scope inside the relevant case.
Comment 11 Devin Rousso 2017-01-02 15:46:58 PST
Created attachment 297915 [details]
Patch
Comment 12 Devin Rousso 2017-01-02 15:51:13 PST
Comment on attachment 297875 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297875&action=review

>>>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:208
>>>> +            let {input, container} = this._colorInputs[key];
>>> 
>>> Please use a Map instead of an object.
>> 
>> So I actually talked with Joe about this, and we both agreed that using an object is clearer since it doesn't ever add/remove after creation.  I'll message you about this next time I am free.
> 
> I don't follow your argument. Please elaborate why that's clearer. An object is a bag of properties that can be used as a struct, class, string-key map, or be proxied, whereas a Map is an ADT.

I just realized that you can construct a Map with items, instead of having to call `set` for each.  That, as well as the fact that the function names are slightly more simple, was the main reason why I wanted to use an object over a Map.

Joe also mentioned that an object, especially one like this that doesn't ever change its keys/values after creation, is able to be optimized more than a Map, but I understand that that won't really matter here since it isn't used that often.
Comment 13 Brian Burg 2017-01-03 00:24:24 PST
(In reply to comment #12)
>
> Joe also mentioned that an object, especially one like this that doesn't
> ever change its keys/values after creation, is able to be optimized more
> than a Map, but I understand that that won't really matter here since it
> isn't used that often.

Yeah. It might be true in a vacuum, but we should favor Map instances instead of objects in ordinary places that don't have a performance problem.
Comment 14 Devin Rousso 2017-01-03 11:39:56 PST
(In reply to comment #13)
> Yeah. It might be true in a vacuum, but we should favor Map instances
> instead of objects in ordinary places that don't have a performance problem.

Is there a reason that you favor Map over object?  I realize that it's an ADT instead of a struct/dictionary/class/etc, but is there a specific reason as to why Map is preferred?  Is there a general "rule-of-thumb" as to when to use a Map vs an object?  Trying to avoid this in the future 😅
Comment 15 Brian Burg 2017-01-03 14:52:30 PST
(In reply to comment #14)
> (In reply to comment #13)
> > Yeah. It might be true in a vacuum, but we should favor Map instances
> > instead of objects in ordinary places that don't have a performance problem.
> 
> Is there a reason that you favor Map over object?  I realize that it's an
> ADT instead of a struct/dictionary/class/etc, but is there a specific reason
> as to why Map is preferred?  Is there a general "rule-of-thumb" as to when
> to use a Map vs an object?  Trying to avoid this in the future 😅

- You can use more things as keys.
- No need to guess or read code to find out what the field does, it gets initialized to a Map.
- Defined iteration order.
Comment 16 Brian Burg 2017-01-03 15:10:33 PST
Comment on attachment 297915 [details]
Patch

r=me

We'll need to revisit this change for AX issues at a later time.
Comment 17 WebKit Commit Bot 2017-01-03 17:27:53 PST
Comment on attachment 297915 [details]
Patch

Clearing flags on attachment: 297915

Committed r210260: <http://trac.webkit.org/changeset/210260>
Comment 18 WebKit Commit Bot 2017-01-03 17:27:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Commit Bot 2017-01-09 13:30:19 PST
Re-opened since this is blocked by bug 166860
Comment 20 Devin Rousso 2017-01-09 14:17:28 PST
(In reply to comment #19)
> Re-opened since this is blocked by bug 166860

What exactly was the issue with this change?  I tried it locally and it worked fine for me.
Comment 21 Brian Burg 2017-01-10 06:23:59 PST
(In reply to comment #20)
> (In reply to comment #19)
> > Re-opened since this is blocked by bug 166860
> 
> What exactly was the issue with this change?  I tried it locally and it
> worked fine for me.

None of the controls below the color wheel are visible, the space where they should exist is empty. Both Antoine and myself confirmed locally and via an internal nightly.
Comment 22 Devin Rousso 2017-01-10 11:39:08 PST
Created attachment 298495 [details]
Patch
Comment 23 Devin Rousso 2017-01-10 11:39:38 PST
(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > Re-opened since this is blocked by bug 166860
> > 
> > What exactly was the issue with this change?  I tried it locally and it
> > worked fine for me.
> 
> None of the controls below the color wheel are visible, the space where they
> should exist is empty. Both Antoine and myself confirmed locally and via an
> internal nightly.

The controls are only supposed to be visible for RGB and HSL color formats, so if you click on a hex it shouldn't display.  I've added some more logic to only show that extra space for valid formats.
Comment 24 Antoine Quint 2017-01-11 00:55:01 PST
(In reply to comment #23)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > Re-opened since this is blocked by bug 166860
> > > 
> > > What exactly was the issue with this change?  I tried it locally and it
> > > worked fine for me.
> > 
> > None of the controls below the color wheel are visible, the space where they
> > should exist is empty. Both Antoine and myself confirmed locally and via an
> > internal nightly.
> 
> The controls are only supposed to be visible for RGB and HSL color formats,
> so if you click on a hex it shouldn't display.  I've added some more logic
> to only show that extra space for valid formats.

Good to know. I was probably testing with a hex value myself and the empty space led me to believe that something was supposed to be there.

But why limit this to RGB and HSL? What about CSS keywords and hex?
Comment 25 Devin Rousso 2017-01-11 09:48:35 PST
(In reply to comment #24)
> Good to know. I was probably testing with a hex value myself and the empty
> space led me to believe that something was supposed to be there.
> 
> But why limit this to RGB and HSL? What about CSS keywords and hex?

Adding inputs for keywords would duplicate from the rule's text editor (see comment #2).  I could add hex editors, but I'm not really sure what to call the individual components.  Should it be [H1, H2, H3] or do each of the 3 (or 4 in the case of hex with alpha) components have a specific name?
Comment 26 Brian Burg 2017-01-11 09:58:40 PST
(In reply to comment #25)
> (In reply to comment #24)
> > Good to know. I was probably testing with a hex value myself and the empty
> > space led me to believe that something was supposed to be there.
> > 
> > But why limit this to RGB and HSL? What about CSS keywords and hex?
> 
> Adding inputs for keywords would duplicate from the rule's text editor (see
> comment #2).  I could add hex editors, but I'm not really sure what to call
> the individual components.  Should it be [H1, H2, H3] or do each of the 3
> (or 4 in the case of hex with alpha) components have a specific name?

Can it use RGBA fields in the editor, but preserve the original format (in the case for hex)? If it's a color keyword, then keep the keyword if the value doesn't change, but if it does change, then probably OK to switch to rgb() or rgba() for non-1 alpha.
Comment 27 Devin Rousso 2017-01-18 16:07:40 PST
Created attachment 299198 [details]
Patch
Comment 28 Devin Rousso 2017-01-18 16:16:44 PST
Created attachment 299200 [details]
Patch
Comment 29 Timothy Hatcher 2017-01-20 20:49:30 PST
Comment on attachment 299200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299200&action=review

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:221
> +        this._element.classList.toggle("hide-inputs", this._color.format !== WebInspector.Color.Format.Keyword

Hmm. Maybe a switch that has cases that set a bool true or false would read better.
Comment 30 Timothy Hatcher 2017-01-20 20:51:02 PST
Comment on attachment 299200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299200&action=review

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:274
> +        if (this._color.format === WebInspector.Color.Format.RGBA || this._color.format === WebInspector.Color.Format.HSLA

Maybe the switch or another switch should set a local bool this if block can use. That would read better.
Comment 31 Devin Rousso 2017-01-20 23:27:18 PST
Comment on attachment 299200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299200&action=review

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:221
>> +        this._element.classList.toggle("hide-inputs", this._color.format !== WebInspector.Color.Format.Keyword
> 
> Hmm. Maybe a switch that has cases that set a bool true or false would read better.

I don't think it makes much sense to have a switch where every case except default has the same effect.  I personally don't have any issues with this, but if someone has strong objections I can "switch" it to a switch (pun intended).

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:274
>> +        if (this._color.format === WebInspector.Color.Format.RGBA || this._color.format === WebInspector.Color.Format.HSLA
> 
> Maybe the switch or another switch should set a local bool this if block can use. That would read better.

If I were to merge this with the above switch statement, all that would really happen is the splitting of the conditions into two smaller ones, both of which would still be long enough to warrant splitting it into multiple lines.  See above comment about creating a second switch.  I think one other alternative may be to alias WebInspector.Color.Format to something more manageable (like Formats) so it has less repeated text.
Comment 32 Joseph Pecoraro 2017-01-23 11:29:14 PST
Comment on attachment 299200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299200&action=review

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:133
> +        if (!color)
> +            return;

Does this happen? Maybe we can change this to:

    console.assert(color instanceof WebInspector.Color);

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:276
> +        || this._color.format === WebInspector.Color.Format.HEXAlpha || this._color.format === WebInspector.Color.Format.ShortHEXAlpha
> +        || (this._color.format === WebInspector.Color.Format.Keyword && this._color.alpha !== 1)) {

Style: these lines should be indented. I know its not your preferred style but it is WebKit style.
Comment 33 Devin Rousso 2017-01-23 12:49:54 PST
Created attachment 299535 [details]
Patch
Comment 34 WebKit Commit Bot 2017-01-23 13:28:43 PST
Comment on attachment 299535 [details]
Patch

Clearing flags on attachment: 299535

Committed r211057: <http://trac.webkit.org/changeset/211057>
Comment 35 WebKit Commit Bot 2017-01-23 13:28:48 PST
All reviewed patches have been landed.  Closing bug.