Bug 233372 – Web Inspector: Add CSS variable names to property name completion list

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

[Razvan Caliman]

<rdar://83205968>

[Razvan Caliman]

Created attachment 444827 [details]
Patch 1.0

Depends on Bug 233369

[Razvan Caliman]

Created attachment 444829 [details]
Screenshot of variable autocompletion

Behavior with patch applied.

[Devin Rousso]

Comment on attachment 444827 [details]
Patch 1.0

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

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:153
> +            this._queryController.addValues(values);

Should we use `this._values` instead since it's been sorted?

> Source/WebInspectorUI/UserInterface/Models/CSSNameCompletions.js:52
> +        this._cachedValues = values.slice(0);

ditto (Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:153)

> Source/WebInspectorUI/UserInterface/Models/CSSNameCompletions.js:53
> +        this._isStaleListOfCSSVariables = true;

Why would we start out as stale?  Perhaps we should reword this to something like `this._needsVariablesFromInspectedNode = true;` so it's more clear what it's used for.

> Source/WebInspectorUI/UserInterface/Models/CSSNameCompletions.js:91
> +        this.values = this._cachedValues.concat(variables);

Is there a better way we can do this?  `Array.prototype.concat` involves creating a copy of the entire array.  See r249301 for more info.

[Razvan Caliman]

Comment on attachment 444827 [details]
Patch 1.0

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

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:153
>> +            this._queryController.addValues(values);
> 
> Should we use `this._values` instead since it's been sorted?

Doesn't really matter. `WI.CSSQueryController` will rank results based on match heuristics, not sort order. It doesn't sort the array again.

>> Source/WebInspectorUI/UserInterface/Models/CSSNameCompletions.js:52
>> +        this._cachedValues = values.slice(0);
> 
> ditto (Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:153)

Done. Though, after concatenation with the list of variables, the resulting array will be sorted anyway when setting `this.values` so it doesn't really help that much.

>> Source/WebInspectorUI/UserInterface/Models/CSSNameCompletions.js:53
>> +        this._isStaleListOfCSSVariables = true;
> 
> Why would we start out as stale?  Perhaps we should reword this to something like `this._needsVariablesFromInspectedNode = true;` so it's more clear what it's used for.

> Why would we start out as stale?
To avoid doing potentially unnecessary work.

`WI.CSSPropertyNameCompletions` gets instantiated in `WI.CSSManager.initializeCSSPropertyNameCompletions()`. At that point it's not guaranteed that the user has selected the desired node, nor has the intention to edit a CSS property name. We need to do the work to merge the CSS property names list with applicable CSS variables for the inspected node only when completions are requested (i.e. the user is typing).

> Perhaps we should reword this to something like `this._needsVariablesFromInspectedNode = true;` so it's more clear what it's used for.
Good idea. Done!

>> Source/WebInspectorUI/UserInterface/Models/CSSNameCompletions.js:91
>> +        this.values = this._cachedValues.concat(variables);
> 
> Is there a better way we can do this?  `Array.prototype.concat` involves creating a copy of the entire array.  See r249301 for more info.

Yes, we can push the cached original values onto the fresh `variables` array; no harm mutating that:
```
variables.pushAll(this._cachedValues)
```

I didn't know concat was slow. I _assume_ the spread operator suffers from the same, right? 
```
[...variables, ...this._cachedVariables]
```

[Razvan Caliman]

Created attachment 446599 [details]
Patch 1.1

Address code review feedback:
- update name of flag for indicating list of css variables is out of date
- avoid copying arrays with Array.concat()

Depends on Bug 233369

[Patrick Angle]

Comment on attachment 446599 [details]
Patch 1.1

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

> Source/WebInspectorUI/ChangeLog:18
> +        be stale whenever the inspected node changes. Only when a query completions comes in do we check
> +        this flag and augment the list of CSS property names with the latest list of CSS variables.

s/query completions/completion query

Also, it might be worth rephrasing this since it initially would lead my to believe that only when executing a query do we rebuild a stale list of completions, but it appears we also do so for `startsWith`. Both of these are kinda like executing a query, but only the first one actually has it in the name.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:150
> +        if (this._queryController) {
> +            this._queryController.reset();
> +            this._queryController.addValues(values);
> +        }

Nit: This is probably better expressed as an early return. e.g.
```
if (!this._queryController)
    return;

[rest of the code]
```

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:47
> +        WI.domManager.addEventListener(WI.DOMManager.Event.InspectedNodeChanged, this._handleInspectedNodeChanged, this);

What about new variables that may be declared while editing the currently inspected DOM node. If I add a `--fancy-new-variable` to a rule for the inspected node, will that variable name also be included when I get completions for another new property in the node's style attribute? I suspect not currently, and the patch doesn't apply cleanly for me to check :(
Probably need to set `this._needsVariablesFromInspectedNode = true` when `WI.DOMNodeStyles.Event.NeedsRefresh` is dispatched for the inspected DOM node's style. The `WI.DOMManager.Event.InspectedNodeChanged` event provides the previously inspected node, so it should be possible to hook up the event listener/tear it down below in `_handleInspectedNodeChanged`.

[Devin Rousso]

Comment on attachment 446599 [details]
Patch 1.1

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

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:150
>> +        }
> 
> Nit: This is probably better expressed as an early return. e.g.
> ```
> if (!this._queryController)
>     return;
> 
> [rest of the code]
> ```

you also could use `?.` for both and not have an `if`
```
    this._queryController?.reset();
    this._queryController?.addValues(values);
```
your choice :)

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:44
> +        this._cachedSortedPropertyNames = this.values.slice();

Does this also need to be updated in `WI.CSSCompletions.prototype.addValues`?  This approach of using `WI.CSSCompletions.prototype.set values` to override the result of `WI.CSSCompletions.prototype.get values` may quickly get out of sync if some other callsite were to call `WI.CSSCompletions.prototype.set values` with a new set of property names, which this would not have any knowledge of.  Perhaps instead of adding a `WI.CSSCompletions.prototype.set values` we could instead make a protected `WI.CSSCompletions.prototype.get additionalValues` that's combined with `this._values` inside `WI.CSSCompletions.prototype.get values` and overridden by this class to return a cached list of CSS variable property names?

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:78
> +        console.assert(nodeStyles instanceof WI.DOMNodeStyles, nodeStyles);

I don't think this adds anything since `stylesForNode` is guaranteed to return a `WI.DOMNodeStyles` (and it'll even create one if it didn't have one yet).

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:81
> +        console.assert(Array.isArray(variables), variables);

this is redundant because `Array.from` is guaranteed to return an array

[Razvan Caliman]

Comment on attachment 446599 [details]
Patch 1.1

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

>> Source/WebInspectorUI/ChangeLog:18
>> +        this flag and augment the list of CSS property names with the latest list of CSS variables.
> 
> s/query completions/completion query
> 
> Also, it might be worth rephrasing this since it initially would lead my to believe that only when executing a query do we rebuild a stale list of completions, but it appears we also do so for `startsWith`. Both of these are kinda like executing a query, but only the first one actually has it in the name.

Slight rewording to remove the ambiguity of when the list of matching variables is refreshed.

>>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:150
>>> +        }
>> 
>> Nit: This is probably better expressed as an early return. e.g.
>> ```
>> if (!this._queryController)
>>     return;
>> 
>> [rest of the code]
>> ```
> 
> you also could use `?.` for both and not have an `if`
> ```
>     this._queryController?.reset();
>     this._queryController?.addValues(values);
> ```
> your choice :)

Oooh, I like `?.` Done

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:44
>> +        this._cachedSortedPropertyNames = this.values.slice();
> 
> Does this also need to be updated in `WI.CSSCompletions.prototype.addValues`?  This approach of using `WI.CSSCompletions.prototype.set values` to override the result of `WI.CSSCompletions.prototype.get values` may quickly get out of sync if some other callsite were to call `WI.CSSCompletions.prototype.set values` with a new set of property names, which this would not have any knowledge of.  Perhaps instead of adding a `WI.CSSCompletions.prototype.set values` we could instead make a protected `WI.CSSCompletions.prototype.get additionalValues` that's combined with `this._values` inside `WI.CSSCompletions.prototype.get values` and overridden by this class to return a cached list of CSS variable property names?

I don't think this is an issue. There is at most only one instance of `WI.CSSPropertyNameCompletions` created in `WI.CSSManager.initializeCSSPropertyNameCompletions()` with the payload of supported CSS properties from the backend call. There is no outside change to this list of properties for the lifetime of the Web Inspector session.

The reason for splitting and subclassing `WI.CSSPropertyNameCompletions` from `WI.CSSCompletions` is to be able to handle mutations to this list of values internally, like we do here by augmenting with applicable CSS variable names. 
`WI.CSSCompletions` shouldn't have to know or care how the list of values it is given was created.

Since we don't update `WI.CSSPropertyNameCompletions.values` from the outside, I think this is safe from side-effects.

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:47
>> +        WI.domManager.addEventListener(WI.DOMManager.Event.InspectedNodeChanged, this._handleInspectedNodeChanged, this);
> 
> What about new variables that may be declared while editing the currently inspected DOM node. If I add a `--fancy-new-variable` to a rule for the inspected node, will that variable name also be included when I get completions for another new property in the node's style attribute? I suspect not currently, and the patch doesn't apply cleanly for me to check :(
> Probably need to set `this._needsVariablesFromInspectedNode = true` when `WI.DOMNodeStyles.Event.NeedsRefresh` is dispatched for the inspected DOM node's style. The `WI.DOMManager.Event.InspectedNodeChanged` event provides the previously inspected node, so it should be possible to hook up the event listener/tear it down below in `_handleInspectedNodeChanged`.

That use case did not work. Thank you for the suggestion! Done.

The latest patch applies cleanly because it no longer depends on unlanded code. (Bug 233369 landed)

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:78
>> +        console.assert(nodeStyles instanceof WI.DOMNodeStyles, nodeStyles);
> 
> I don't think this adds anything since `stylesForNode` is guaranteed to return a `WI.DOMNodeStyles` (and it'll even create one if it didn't have one yet).

Removed.

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:81
>> +        console.assert(Array.isArray(variables), variables);
> 
> this is redundant because `Array.from` is guaranteed to return an array

Removed.

[Razvan Caliman]

Created attachment 446735 [details]
Patch 1.2

Address code review feedback:
- remove unnecessary asserts
- refresh list of CSS variables when styles change
- address nits & typos

[Devin Rousso]

Comment on attachment 446599 [details]
Patch 1.1

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

>>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:44
>>> +        this._cachedSortedPropertyNames = this.values.slice();
>> 
>> Does this also need to be updated in `WI.CSSCompletions.prototype.addValues`?  This approach of using `WI.CSSCompletions.prototype.set values` to override the result of `WI.CSSCompletions.prototype.get values` may quickly get out of sync if some other callsite were to call `WI.CSSCompletions.prototype.set values` with a new set of property names, which this would not have any knowledge of.  Perhaps instead of adding a `WI.CSSCompletions.prototype.set values` we could instead make a protected `WI.CSSCompletions.prototype.get additionalValues` that's combined with `this._values` inside `WI.CSSCompletions.prototype.get values` and overridden by this class to return a cached list of CSS variable property names?
> 
> I don't think this is an issue. There is at most only one instance of `WI.CSSPropertyNameCompletions` created in `WI.CSSManager.initializeCSSPropertyNameCompletions()` with the payload of supported CSS properties from the backend call. There is no outside change to this list of properties for the lifetime of the Web Inspector session.
> 
> The reason for splitting and subclassing `WI.CSSPropertyNameCompletions` from `WI.CSSCompletions` is to be able to handle mutations to this list of values internally, like we do here by augmenting with applicable CSS variable names. 
> `WI.CSSCompletions` shouldn't have to know or care how the list of values it is given was created.
> 
> Since we don't update `WI.CSSPropertyNameCompletions.values` from the outside, I think this is safe from side-effects.

This may be the case right now, but a future engineer who looks at `WI.cssManager.propertyNameCompletions` and `WI.CSSCompletions` might think that they can add new things via `addValues` and/or `set values`, which would cause this hidden bug to surface.  Also, you've put `set values` inside the "// Public" section so again future engineers will look at that and think it's allowed by anyone.  I think if you want to do as you describe we need to rename `set values` to `updateValues` in "// Protected" (so that only subclasses are allowed call it) and override `addValues` to either throw an error or also add to `_cachedSortedPropertyNames` (or re-initialize `_cachedSortedPropertyNames` with the new `this.value`).

[Devin Rousso]

Comment on attachment 446735 [details]
Patch 1.2

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

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:59
> +        if (this._needsVariablesFromInspectedNode)

NIT: you could move this `if` inside `_updateValuesWithLatestCSSVariables` and rename the latter to `_updateValuesWithLatestCSSVariablesIfNeeded` to avoid having to have every callsite do this check

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:92
> +        if (!event.data.lastInspectedNode?.id)

I think you should really just check for `!event.data.lastInspectedNode` as I don't think it's possible for a `WI.DOMNode` to have a falsy `id`.

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:96
> +        nodeStyles.removeEventListener(WI.DOMNodeStyles.Event.NeedsRefresh, this._handleNodesStylesNeedsRefresh, this);

This could be problematic if the user changes the inspected node multiple times before `_updateValuesWithLatestCSSVariables` is called.  I think you should add an early return if `this._needsVariablesFromInspectedNode` is already truthy as in that case we won't have added the event listener yet, meaning there's also no need to remove it.

[Razvan Caliman]

Comment on attachment 446735 [details]
Patch 1.2

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

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:59
>> +        if (this._needsVariablesFromInspectedNode)
> 
> NIT: you could move this `if` inside `_updateValuesWithLatestCSSVariables` and rename the latter to `_updateValuesWithLatestCSSVariablesIfNeeded` to avoid having to have every callsite do this check

Done

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:92
>> +        if (!event.data.lastInspectedNode?.id)
> 
> I think you should really just check for `!event.data.lastInspectedNode` as I don't think it's possible for a `WI.DOMNode` to have a falsy `id`.

Indeed, there is the case `lastInspectedNode` is `null` at start-up, but I didn't encounter a scenario where DOMNode exists without an id.

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:96
>> +        nodeStyles.removeEventListener(WI.DOMNodeStyles.Event.NeedsRefresh, this._handleNodesStylesNeedsRefresh, this);
> 
> This could be problematic if the user changes the inspected node multiple times before `_updateValuesWithLatestCSSVariables` is called.  I think you should add an early return if `this._needsVariablesFromInspectedNode` is already truthy as in that case we won't have added the event listener yet, meaning there's also no need to remove it.

Good catch. Done!

[Razvan Caliman]

Comment on attachment 446599 [details]
Patch 1.1

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

>>>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:44
>>>> +        this._cachedSortedPropertyNames = this.values.slice();
>>> 
>>> Does this also need to be updated in `WI.CSSCompletions.prototype.addValues`?  This approach of using `WI.CSSCompletions.prototype.set values` to override the result of `WI.CSSCompletions.prototype.get values` may quickly get out of sync if some other callsite were to call `WI.CSSCompletions.prototype.set values` with a new set of property names, which this would not have any knowledge of.  Perhaps instead of adding a `WI.CSSCompletions.prototype.set values` we could instead make a protected `WI.CSSCompletions.prototype.get additionalValues` that's combined with `this._values` inside `WI.CSSCompletions.prototype.get values` and overridden by this class to return a cached list of CSS variable property names?
>> 
>> I don't think this is an issue. There is at most only one instance of `WI.CSSPropertyNameCompletions` created in `WI.CSSManager.initializeCSSPropertyNameCompletions()` with the payload of supported CSS properties from the backend call. There is no outside change to this list of properties for the lifetime of the Web Inspector session.
>> 
>> The reason for splitting and subclassing `WI.CSSPropertyNameCompletions` from `WI.CSSCompletions` is to be able to handle mutations to this list of values internally, like we do here by augmenting with applicable CSS variable names. 
>> `WI.CSSCompletions` shouldn't have to know or care how the list of values it is given was created.
>> 
>> Since we don't update `WI.CSSPropertyNameCompletions.values` from the outside, I think this is safe from side-effects.
> 
> This may be the case right now, but a future engineer who looks at `WI.cssManager.propertyNameCompletions` and `WI.CSSCompletions` might think that they can add new things via `addValues` and/or `set values`, which would cause this hidden bug to surface.  Also, you've put `set values` inside the "// Public" section so again future engineers will look at that and think it's allowed by anyone.  I think if you want to do as you describe we need to rename `set values` to `updateValues` in "// Protected" (so that only subclasses are allowed call it) and override `addValues` to either throw an error or also add to `_cachedSortedPropertyNames` (or re-initialize `_cachedSortedPropertyNames` with the new `this.value`).

Ok, I replaced the public setter with a `WI.CSSCompletions.replaceValues()`. It's a bit of a "beware of dog" sign, but I guess it helps by requiring a more intentional reading than just setting a value.

`WI.CSSPropertyNames.addValues()` will throw an error to prevent unintentional overwriting. 

Factoring in logic to clone or replace `_cachedSortedPropertyNames` feels premature. We don't have this use case yet. It can be done when necessary.

[Razvan Caliman]

Created attachment 446782 [details]
Patch 1.3

Address code review

[Devin Rousso]

Comment on attachment 446782 [details]
Patch 1.3

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

r=me, nice work!

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:179
> +        console.assert(Array.isArray(values), values);

NIT: Do we also want the other assertion in the `constructor` too?  In fact, you could just call this in the `constructor` and avoid some repeated code :)

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:182
> +        this._queryController?.reset();

Style: I'd add a newline before this

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:60
> +        return super.executeQuery(query);

Style: I'd add a newline before this

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:66
> +        return super.startsWith(prefix);

ditto (:60)

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:71
> +        throw new Error("Adding values will overwrite the list of supported CSS property names");

NIT: I'd just make this into a `console.assert(false, "Adding values will overwrite the list of supported CSS property names")` so that this doesn't cause issues in production.  You've already made sure it's not an issue by NOT calling `super.addValues(...)`.

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:91
> +        let variables = Array.from(nodeStyles.allCSSVariables);
> +        let values = variables;

NIT: I'd just do `let values = Array.from(nodeStyles.allCSSVariables);` and avoid the extra `variables` variable that's not used otherwise.

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:93
> +

Style: I'd remove this newline

> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:95
> +        this._needsVariablesFromInspectedNode = false;

Style: I'd add a newline before this

[Razvan Caliman]

Comment on attachment 446782 [details]
Patch 1.3

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

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:179
>> +        console.assert(Array.isArray(values), values);
> 
> NIT: Do we also want the other assertion in the `constructor` too?  In fact, you could just call this in the `constructor` and avoid some repeated code :)

Copied over the assertion.

I'll avoid calling this in the constructor. That's a recipe for trouble when we add more stateful logic and the unwittingly run it by accident.

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:182
>> +        this._queryController?.reset();
> 
> Style: I'd add a newline before this

Done

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:60
>> +        return super.executeQuery(query);
> 
> Style: I'd add a newline before this

Done

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:66
>> +        return super.startsWith(prefix);
> 
> ditto (:60)

Done

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:71
>> +        throw new Error("Adding values will overwrite the list of supported CSS property names");
> 
> NIT: I'd just make this into a `console.assert(false, "Adding values will overwrite the list of supported CSS property names")` so that this doesn't cause issues in production.  You've already made sure it's not an issue by NOT calling `super.addValues(...)`.

Good idea.

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:91
>> +        let values = variables;
> 
> NIT: I'd just do `let values = Array.from(nodeStyles.allCSSVariables);` and avoid the extra `variables` variable that's not used otherwise.

Done.

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:93
>> +
> 
> Style: I'd remove this newline

Done.

>> Source/WebInspectorUI/UserInterface/Models/CSSPropertyNameCompletions.js:95
>> +        this._needsVariablesFromInspectedNode = false;
> 
> Style: I'd add a newline before this

Done

[Razvan Caliman]

Created attachment 446788 [details]
Patch 1.4

Carry over R+; Address final nits

[EWS]

Committed r286890 (245118@main): <https://commits.webkit.org/245118@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446788 [details].