Bug 124356 – Web Inspector: color picker should feature an editable CSS value

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

[Antoine Quint]

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.

[Radar WebKit Bug Importer]

<rdar://problem/15469602>

[Timothy Hatcher]

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.

[Devin Rousso]

Created attachment 297875 [details]
Patch

[Devin Rousso]

Created attachment 297876 [details]
[Image] After Patch is applied

[Blaze Burg]

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.

[Devin Rousso]

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?

[Devin Rousso]

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")

[Devin Rousso]

Created attachment 297898 [details]
Patch

[Blaze Burg]

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

[Blaze Burg]

(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.

[Devin Rousso]

Created attachment 297915 [details]
Patch

[Devin Rousso]

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.

[Blaze Burg]

(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.

[Devin Rousso]

(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 😅

[Blaze Burg]

(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.

[Blaze Burg]

Comment on attachment 297915 [details]
Patch

r=me

We'll need to revisit this change for AX issues at a later time.

[WebKit Commit Bot]

Comment on attachment 297915 [details]
Patch

Clearing flags on attachment: 297915

Committed r210260: <http://trac.webkit.org/changeset/210260>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 166860

[Devin Rousso]

(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.

[Blaze Burg]

(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.

[Devin Rousso]

Created attachment 298495 [details]
Patch

[Devin Rousso]

(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.

[Antoine Quint]

(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?

[Devin Rousso]

(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?

[Blaze Burg]

(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.

[Devin Rousso]

Created attachment 299198 [details]
Patch

[Devin Rousso]

Created attachment 299200 [details]
Patch

[Timothy Hatcher]

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.

[Timothy Hatcher]

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.

[Devin Rousso]

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.

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 299535 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 299535 [details]
Patch

Clearing flags on attachment: 299535

Committed r211057: <http://trac.webkit.org/changeset/211057>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.