Bug 203169 – Web Inspector: Replace color wheel with square HSB color picker

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 381342 [details]
[Image] With patch applied

Add an experimental setting to replace the current color wheel with a rectangular HSL color picker.

[Nikita Vasilyev]

Created attachment 381397 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/56449832>

[Devin Rousso]

Comment on attachment 381397 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Setting.js:193
> +    experimentalColorPicker: new WI.Setting("experimental-color-picker", false),

Do we really need an experimental setting for this?  At the very least, I'd expect this to be on by default, otherwise nobody will end up using/testing it.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:58
> +    background-image: linear-gradient(to right, #f00 0%, #ff0 17%, #0f0 33%, #0ff 50%, #00f 67%, #f0f 83%, #f00 100%)

We should either use `hsl` or keyword color values.
```
   background-image: linear-gradient(to right, red 0%, yellow 16.6%, lime 33.2%, aqua 50%, blue 66.6%, fuchsia 83.2%, red 100%);
```

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:70
> +    left: calc(var(--color-picker-width) - var(--color-picker-brightness-offset-start));

We should either rename this variable to something more generic, or create another variable with the same value.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:71
> +            this._colorWheel = new WI.ColorSquare(this, 200);

It's no longer accurate to call this `_colorWheel`.  Perhaps we could use `_colorView`?  Alternatively, we could just have a separate variable for `_colorSquare`.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:113
> +        if (!WI.settings.experimentalColorPicker.value)

This only appears to be called from `sliderValueDidChange`, so I'd actually change this to an assertion so we can avoid an unnecessary check.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:184
>          else if (slider === this._brightnessSlider)

Let's change this to a switch.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:187
> +            this._colorWheel.hue = value * 360;

We could create a `set hue` that does this logic for us.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:30
> +.color-square .saturation-gradient {

Please use a direct descendant selector:
```
    .color-square > .crosshair
```

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:31
> +    background-image: linear-gradient(to right, white, rgba(255, 255, 255, 0));

Please use `hsl`.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:34
> +.color-square .lightness-gradient {

Ditto (30)

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:35
> +    background-image: linear-gradient(to top, black, rgba(0, 0, 0, 0));

`transparent`?

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:38
> +.color-square .fill {

Ditto (30)

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:46
> +.color-square .crosshair {

Ditto (30)

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:49
> +    top: calc(-1 * var(--crosshair-size) / 2);
> +    left: calc(-1 * var(--crosshair-size) / 2);

Is there any way we can avoid using negative coordinates?

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.css:53
> +    border: .5px solid black;

Style: `0.5px`

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:30
> +        console.assert(!isNaN(dimension));

Should this be moved inside `set dimension`?

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:42
> +        let baseColorElement = document.createElement("div");
> +        this._baseColorElement = baseColorElement;

We can simplify this onto one line:
```
    this._baseColorElement = this._element.appendChild(document.createElement("div"));
```

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:46
> +        let saturationGradientElement = document.createElement("div");

Style: we usually combine the `appendChild` onto the same line.
```
    let saturationGradientElement = baseColorElement.appendChild(document.createElement("div"));
```

Another idea, we could use `:before` and `:after` for these gradients, seeing as they're never changed.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:50
> +        let lightnessGradientElement = document.createElement("div");

Ditto (46)

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:56
> +        this._crosshairElement = document.createElement("div");

Ditto (46)

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:92
> +        if (this._crosshairPosition)

Style: this should be defined (even if `null`) in the constructor.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:97
> +    set tintedColor(tintedColor)

Let's add a `console.assert(tintedColor instanceof WI.Color)`;.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:115
> +        return new WI.Color(WI.Color.Format.HSLA, [0, 0, 0, 0]);

NIT: we can use `WI.Color.Keywords.transparent` instead of the hardcoded array.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:125
> +            break;

Let's make these `return;`s and add a not reached `console.assert();` after to make sure we don't use this unexpectedly.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:151
> +        return (200 - this._saturation) * (this._brightness / 100) / 2;

Can you explain this math?  Is this something you came up with?  Perhaps it should be using `_dimension` instead of `100`?

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:180
> +        if (this._delegate && typeof this._delegate.colorWheelColorDidChange === "function")

We should drop the `typeof` check.  If the `_delegate` has a `colorWheelColorDidChange` member, it should be a function or we have an error in our own code.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:189
> +        this._crosshairElement.style.setProperty("transform", "translate(" + this._x + "px, " + this._y + "px)");

`translate(${this._x}px, ${this._y}px)`?

[Nikita Vasilyev]

Comment on attachment 381397 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Setting.js:193
>> +    experimentalColorPicker: new WI.Setting("experimental-color-picker", false),
> 
> Do we really need an experimental setting for this?  At the very least, I'd expect this to be on by default, otherwise nobody will end up using/testing it.

I'd prefer to go without the experimental setting! The square color picker in this patch is very much usable, in my opinion, and I'm not aware of any bugs.

>> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:151
>> +        return (200 - this._saturation) * (this._brightness / 100) / 2;
> 
> Can you explain this math?  Is this something you came up with?  Perhaps it should be using `_dimension` instead of `100`?

No, I didn't come up with it. This is based on the HSV to HSL conversion equation https://en.wikipedia.org/wiki/HSL_and_HSV#HSV_to_HSL
I'll add a coment here and for line 102.

[Nikita Vasilyev]

Comment on attachment 381397 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:187
>> +            this._colorWheel.hue = value * 360;
> 
> We could create a `set hue` that does this logic for us.

This is the only place it would be used. I think it's fine as it is.

>> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:42
>> +        this._baseColorElement = baseColorElement;
> 
> We can simplify this onto one line:
> ```
>     this._baseColorElement = this._element.appendChild(document.createElement("div"));
> ```

It seems like I could use remove this._baseColorElement and use this._element instead.

>> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:125
>> +            break;
> 
> Let's make these `return;`s and add a not reached `console.assert();` after to make sure we don't use this unexpectedly.

I'm not sure how it would be better.

>>> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:151
>>> +        return (200 - this._saturation) * (this._brightness / 100) / 2;
>> 
>> Can you explain this math?  Is this something you came up with?  Perhaps it should be using `_dimension` instead of `100`?
> 
> No, I didn't come up with it. This is based on the HSV to HSL conversion equation https://en.wikipedia.org/wiki/HSL_and_HSV#HSV_to_HSL
> I'll add a coment here and for line 102.

Also, 100 is the maximum brightness value here. It isn't related to _dimension.

[Nikita Vasilyev]

Created attachment 381624 [details]
Patch

[Devin Rousso]

Comment on attachment 381624 [details]
Patch

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

r=me, nice work :)

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:34
> +        this._hueSlider = new WI.Slider;

It's odd that the `0` of the hue slider is at the bottom.  I'd almost expect it to be at the top.  Could we flip it vertically?

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:-103
> -        this._colorWheel.brightness = brightness;

I don't think `set brightness` is used anywhere anymore.  We should remove it.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:119
> +        return this._colorSquare;

Style: I'd make this (and `get element`) into one-line getters at the top of the Public section

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.cssSource/WebInspectorUI/UserInterface/Views/ColorWheel.css:53
> +    border: 0.5px solid black;

We should only do this if we are on a @2x device.  Please make this `1px` and add a `@media (-webkit-min-device-pixel-ratio: 2)` query for `border-width: 0.5px;`.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:53
> +        this._updateBaseColor();

This is already called by `set dimension`, so we can remove this.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:101
> +        y = -(y - 1) * this._dimension;

NIT: I'd personally use `-1 * (` instead of `-(` so it's less likely to be missed.

[Nikita Vasilyev]

Created attachment 381709 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 381709 [details]
Patch

Clearing flags on attachment: 381709

Committed r251487: <https://trac.webkit.org/changeset/251487>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.