[webkit-reviews] review denied: [Bug 205482] Web Inspector: Color picker: add color palettes with colors from CSS variables : [Attachment 388355] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 205482: Web Inspector: Color picker: add color palettes with colors from
CSS variables
https://bugs.webkit.org/show_bug.cgi?id=205482

Attachment 388355: Patch

https://bugs.webkit.org/attachment.cgi?id=388355&action=review




--- Comment #10 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 388355
  --> https://bugs.webkit.org/attachment.cgi?id=388355
Patch

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

r-, for reasons described in the ChangeLog

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:81
> +    /* `flex-shrink: 0;` prevents scrolling. */
> +    flex-shrink: revert;

Is this still needed?

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

Is there really no way to avoid a negative margin?  They're pretty awful for
performance :(

Also, it should be `-webkit-margin-start: -1px;`.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:323
> +	   let expandableView = new
WI.ExpandableView("color-picker-css-variables", headerElement, contentElement);
> +	   expandableView.element.classList.add("color-palette");
> +	   this._element.replaceChild(expandableView.element,
this._colorPalette);

Rather than create a new `WI.ExpandableView` each time we get new
`cssProperties`, can we create this in the `constructor` and hide it until it's
asked to be shown, and then save references for `this._headerElement` and
`this._childElement` and just modify them instead?  This could help decrease
the number of times we re-fetch the value of the `this._expandedSetting` from
`localStorage` as well.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:352
> +	       let itemElement = contentElement.createChild("div",
"color-palette-item");

It seems like the only difference between this `for` loop and the above `for`
loop is `"div"` vs `"span"` (and the `title`).	Perhaps you could make a local
function that takes a `WI.CSSProperty` and an `isInline` boolean so we don't
duplicate this logic.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:372
> +	   if (element)
> +	       element.classList.add("selected");

Please use `this._selectedColorPaletteElement`.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:400
> +    SizeChanged: "css-color-picker-size-changed",

Can we use a more specific/indicative name for this?  Perhaps
`VariablesViewToggled`?

> Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:41
> +	       headerElement.addEventListener("click",
this._onTitleClick.bind(this));

Wouldn't it be more accurate to say `_handleHeaderClick`?

> Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:75
> +	   let focusNode = selection && !selection.isCollapsed &&
selection.focusNode;

This is very odd, as it results in either `false` or a `Node`, which are not
the same type.	Please rework this.
```
    if (selection && !selection.isCollapsed &&
this._headerElement.contains(selection.focusNode))
```

> Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:87
> +	   this.dispatchEventToListeners(WI.ExpandableView.Event.Toggled,
{expanded: shouldExpand});

Why do we need to return `shouldExpand` instead of having callers just call
`get expanded`?

Also, we should probably put this inside `_update` (which you could make `set
expanded` if you want) so it's never missed.

> Source/WebInspectorUI/UserInterface/Views/GradientEditor.css:61
> +    --color-picker-padding: 0px;

Style: you can drop the `px`

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:301
> +	       console.assert(value instanceof WI.Color);

This assertion already exists inside `WI.ColorPicker.prototype.set color`.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:319
> +	       let resolvedValue =
this.delegate.inlineSwatchResolveVariable(value);

We should assert that this is set if `this._type ===
WI.InlineSwatch.Type.Variable` inside the constructor.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:345
> +	       this._valueText = "";

Why?

> Source/WebInspectorUI/UserInterface/Views/Popover.js:202
> +    setContentPadding(padding)

Why not `set contentPadding`?

> Source/WebInspectorUI/UserInterface/Views/Popover.js:324
> +	   this._container.style.padding = `${this._contentPadding}px`;

Style: I would prefer `this._container.style.setProperty("padding",
this._contentPadding + "px");` as it is easier to search (`"padding"` vs
`.padding`) in the code and in general it matches the CSS syntax (e.g.
`"padding-left"` instead of `.paddingLeft`), but in this case it's not a big
deal

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-531
> -	       swatch.value = () => {
> -		   return
this._property.ownerStyle.nodeStyles.computedStyle.resolveVariableValue(innerEl
ement.textContent);
> -	       };

I don't think we should remove this.  The reason I made it this way when it was
first added was because a CSS variable's value can change and the Web Inspector
frontend, which won't get picked up by this, meaning that the swatch/color will
be inaccurate.	We either need to have better instrumentation around when CSS
variables update or have the clicking of the swatch be the point when we
determine the actual resolved value.

# STEPS TO REPRODUCE:
1. inspect any page
2. add `color: var(--DoesNotExistVariable);` to the inline style of any node
 => no swatch is shown next to `var(--DoesNotExistVariable)`
3. add `--DoesNotExistVariable: red;` to some _other_ CSS rule that is already
being applied to the current node
 => still no swatch next to `var(--DoesNotExistVariable)`

This issue is even more evident if the CSS variable _does_ exist.

# STEPS TO REPRODUCE
1. inspect any page
2. add `--foo: red;` to the inline style of any node
3. add `color: var(--foo);` to the inline style of the same node as step #2
 => red color swatch is shown next to `var(--foo)`
3. add `--foo: blue !important;` to some _other_ CSS rule that is already being
applied to the current node
 => still a red color swatch next to `var(--foo)` which _should_  be blue

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:536
> +	       swatch.delegate = this;

Is there a reason to not always set the delegate, like in the constructor?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:539
> +	       swatch.addEventListener(WI.InlineSwatch.Event.Activated,
this._handleColorSwatchActivated.bind(this));

Can we merge this with the event listener that is added below, so we don't add
it twice?

Also, you don't need to `bind` when `addEventListener` to an `WI.Object`.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:948
> +	   let colorPicker = event.target.valueEditor;

I don't think this is a good approach, as it reveals an internal mechanism of
`WI.InlineSwatch` and now requires that any changes that are made also update
this code.  Please either add a check (instead of assuming) that the
`valueEditor` is a `WI.ColorPicker` or have a different way to access the
`WI.ColorPicker` (e.g. `event.data.colorPicker`).