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.
<rdar://problem/15469602>
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.
Created attachment 297875 [details] Patch
Created attachment 297876 [details] [Image] After Patch is applied
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 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 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")
Created attachment 297898 [details] Patch
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.)
(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.
Created attachment 297915 [details] Patch
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.
(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.
(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 😅
(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 on attachment 297915 [details] Patch r=me We'll need to revisit this change for AX issues at a later time.
Comment on attachment 297915 [details] Patch Clearing flags on attachment: 297915 Committed r210260: <http://trac.webkit.org/changeset/210260>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 166860
(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.
(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.
Created attachment 298495 [details] Patch
(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.
(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?
(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?
(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.
Created attachment 299198 [details] Patch
Created attachment 299200 [details] Patch
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 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 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 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.
Created attachment 299535 [details] Patch
Comment on attachment 299535 [details] Patch Clearing flags on attachment: 299535 Committed r211057: <http://trac.webkit.org/changeset/211057>