Web Inspector: Gradient editor: opacity slider is too close to the right edge of the popover

[Nikita Vasilyev]

Created attachment 382386 [details]
[Image] Bug

This is not a recent regression. It's been like this for over a year.

The hue and the opacity sliders should move 10px to the left.

[Radar WebKit Bug Importer]

<rdar://problem/56762879>

[Nikita Vasilyev]

We should use static layout (e.g. flexbox) instead of positioning everything absolute.

The sliders are currently rotated 90 degrees by CSS transform to be vertical. This doesn't work well with flexbox; flexbox doesn't understand that width and height are now flipped.

This is the only place where the sliders are used. We should convert WI.Slider to be vertical (and remove CSS transforms from ColorPicker.css).

[Nikita Vasilyev]

Created attachment 386164 [details]
Patch

[Nikita Vasilyev]

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

[Devin Rousso]

Comment on attachment 386164 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:46
> +.color-picker .color-square,
> +.color-picker .slider {

`.color-picker :matches(.color-square, .slider)`

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:51
> +    height: 200px;

Can we just make this `height: 100%;`?

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:52
> +    margin-left: 10px;

`-webkit-margin-start`

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:91
> +    margin: 0;

Why not just drop this then?

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:98
> +.color-picker > .color-inputs > div:not([hidden]) + div {

I think a more accurate selector would be `.color-picker > .color-inputs > div:nth-child(n + 2 of :not([hidden]))`

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:99
> +    margin-left: 4px;

`-webkit-margin-start`

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:78
> +        wrapper.className = "wrapper";

Why bother adding a class if it's not used?

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:81
> +        wrapper.append(this._colorSquare.element);
> +        wrapper.append(this._hueSlider.element);
> +        wrapper.append(this._opacitySlider.element);

Please use `appendChild`.

> Source/WebInspectorUI/UserInterface/Views/Slider.css:34
> +    left: 2px;

Do we need to do anything special for this for RTL?

> Source/WebInspectorUI/UserInterface/Views/Slider.js:73
> +        this._knob.style.webkitTransform = `translateY(${this._knobY}px) rotate(270deg)`;

Rather than have to repeat the `rotate(270deg)`, why not set a `--translate-y` variable and use that in the CSS?
```
    this._knob.style.webkitTransform = `--translate-y: ${this._knobY}px`;

    translate: translateY(var(--translate-y, 0)) rotate(270deg);
```

[Nikita Vasilyev]

Comment on attachment 386164 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:51
>> +    height: 200px;
> 
> Can we just make this `height: 100%;`?

This doesn't work, unfortunatelly.

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:52
>> +    margin-left: 10px;
> 
> `-webkit-margin-start`

Thanks.

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:91
>> +    margin: 0;
> 
> Why not just drop this then?

Oh, right :)

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:98
>> +.color-picker > .color-inputs > div:not([hidden]) + div {
> 
> I think a more accurate selector would be `.color-picker > .color-inputs > div:nth-child(n + 2 of :not([hidden]))`

I find it confusing that you have to write "+ 2" when you want to skip ONE (first) element.

P.S. I'm planning to remove this selector as I'm planning to only render visible elements after Bug 203928: Web Inspector: Show RGBA input fields for p3 color picker.
P.P.S. https://developer.mozilla.org/en-US/docs/Web/CSS/column-gap should be used for this specific case. In WebKit, it currently only works for CSS grids and doesn't work for flexbox.

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:99
>> +    margin-left: 4px;
> 
> `-webkit-margin-start`

Thanks.

>> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:78
>> +        wrapper.className = "wrapper";
> 
> Why bother adding a class if it's not used?

It's used in `.color-picker .wrapper` rule.

>> Source/WebInspectorUI/UserInterface/Views/Slider.css:34
>> +    left: 2px;
> 
> Do we need to do anything special for this for RTL?

I don't think it's particularly matters here.

[Nikita Vasilyev]

Created attachment 386185 [details]
Patch

[Devin Rousso]

Comment on attachment 386185 [details]
Patch

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

r=me, with one additional change

> Source/WebInspectorUI/UserInterface/Views/Slider.css:-34
> -    left: -5px;

This doesn't look very good in RTL.  The slider thumb is on the wrong side of the slider (it's on the right side in LTR and in RTL, whereas it should be on the left side in RTL).
```
    body[dir=ltr] .slider > img {
        left: 2px;
        transform: translateY(var(--translate-y, 0px)) rotate(270deg);
    }

    body[dir=rtl] .slider > img {
        left: -6px;
        transform: translateY(var(--translate-y, 0px)) rotate(90deg);
    }
```

[Nikita Vasilyev]

Created attachment 386282 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 386282 [details]
Patch

Clearing flags on attachment: 386282

Committed r253859: <https://trac.webkit.org/changeset/253859>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.