Bug 227098 – Web Inspector: Styles: should autocomplete `var()` and `attr()` values

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

# STEPS TO REPRODUCE
1. inspect any page
2. go to the Styles panel in the details sidebar in the Elements Tab
3. add a new CSS property
4. type `--xyz: red`
5. add a new CSS property
6. type `color: var(`

# EXPECTED
should show `--xyz` etc. in autocompletion

# ACTUAL
no autocompletions

[Radar WebKit Bug Importer]

<rdar://problem/79418247>

[Razvan Caliman]

Created attachment 431858 [details]
WIP

Request for feedback on general direction

[Razvan Caliman]

This WIP patch is building on the patch from bug 227097 which hasn't landed yet, therefore it's now only a request for feedback.

There is prior work for augmenting function completions in `Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:588`:

```
this._delegate.completionControllerCSSFunctionValuesNeeded(this, functionName, functionCompletions)
```

However, the delegate doesn't implement `.completionControllerCSSFunctionValuesNeeded()`. 
The delegate changed over time. It used to be `WI.CSSStyleDeclarationTextEditor`, bug got removed with the legacy style editor in bug 189808.

But the idea seems good. 

---

This patch implements `WI.SpreadsheetCSSStyleDeclarationEditor.completionControllerCSSFunctionValuesNeeded()`, delegated for `WI.SpreadsheetStyleProperty`.

Depending on the function name:
- for "var", return a list of CSS variable names collected in WI.DOMNodeStyles.allCSSVariables
- for "attr", return a list of node attribute names (restoring functionality lost when removing `WI.CSSStyleDeclarationTextEditor` in bug 189808)

Should we delegate a few more levels up to `WI.SpreadsheetRulesStyleDetailsPanel`? Or is it acceptable to stop at `WI.SpreadsheetCSSStyleDeclarationEditor`?

---

This does yet handle CSS variable completions in the Sources tab (i.e. implementing `WI.SourceCodeTextEditor.completionControllerCSSFunctionValuesNeeded()`). 
For that, we need to collect *all CSS Variables* declared in all stylesheets since the Sources code editor doesn't have a concept of a selected node nor does that make sense. Probably left to don in a follow-up.

[Razvan Caliman]

Comment on attachment 431858 [details]
WIP

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

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:264
> +    set values(values = [])

Doing this to ensure values can be added on-demand and the resulting list remains sorted.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:107
> +    if (functionName === "var" || functionName === "attr")

`attr(var(--something))` is not supported.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:456
> +            values = this.style.node.attributes().map(attribute => attribute.name);

Restoring functionality lost with bug 189808. Should this be in a separate bug/patch?

[Devin Rousso]

Comment on attachment 431858 [details]
WIP

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

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:264
>> +    set values(values = [])
> 
> Doing this to ensure values can be added on-demand and the resulting list remains sorted.

This is a bit odd as `WI.CSSCompletions` is really meant to be basically readonly.  Instead of allowing the `WI.CSSCompletion` to be entirely modifiable, I wonder if we could just have an `addAll` (i.e. can only extend, not replace) or really more ideally have a way for `WI.CSSKeywordCompletions.forFunction` to do this logic before construction.

Also, having a default parameter here isn't ideal as that's only going to be used if `undefined` is provided, and we generally avoid using `undefined` instead of `null`.  I think you're better off having a `values ||= [];` inside the function instead (FWIW this is our normal pattern).

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:107
>> +    if (functionName === "var" || functionName === "attr")
> 
> `attr(var(--something))` is not supported.

I wonder if we want to autocomplete `attr` on any property other than `content` given that it only works there 🤔

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:952
> +        this._allCSSVariables = new Set;

This isn't really great IMO as a function named `_collectUsedCSSVariables` has a side effect of modifying `_allCSSVariables`.  I'd recommend renaming this function to something more generic (e.g. `_collectCSSVariables`) so that this is more "acceptable".

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:453
> +            values = this.style.nodeStyles.allCSSVariables;

Style: change this into a `return`

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:456
>> +            values = this.style.node.attributes().map(attribute => attribute.name);
> 
> Restoring functionality lost with bug 189808. Should this be in a separate bug/patch?

Yeah this should probably be a different bug (or you could rename this bug to also include `attr()`.

Style: argument of arrow functions should always have parenthesis

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:988
> +            return { prefix: currentTokenValue, completions: completions.startsWith(currentTokenValue) };

Style: no space after `{` or before `}`

[Razvan Caliman]

Comment on attachment 431858 [details]
WIP

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

>>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:264
>>> +    set values(values = [])
>> 
>> Doing this to ensure values can be added on-demand and the resulting list remains sorted.
> 
> This is a bit odd as `WI.CSSCompletions` is really meant to be basically readonly.  Instead of allowing the `WI.CSSCompletion` to be entirely modifiable, I wonder if we could just have an `addAll` (i.e. can only extend, not replace) or really more ideally have a way for `WI.CSSKeywordCompletions.forFunction` to do this logic before construction.
> 
> Also, having a default parameter here isn't ideal as that's only going to be used if `undefined` is provided, and we generally avoid using `undefined` instead of `null`.  I think you're better off having a `values ||= [];` inside the function instead (FWIW this is our normal pattern).

> or really more ideally have a way for `WI.CSSKeywordCompletions.forFunction` to do this logic before construction.

Funny, I had a version of the patch doing this and thought it would get flagged during review :) 
Do you mean something like this?

```
-WI.CSSKeywordCompletions.forFunction = function(functionName)
+WI.CSSKeywordCompletions.forFunction = function(functionName, supplementalCompletions = {})
 {
     let suggestions = ["var()"];
 
     if (functionName === "var")
-        suggestions = [];
+        suggestions = supplementalCompletions?.var || [];
```

>>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:107
>>> +    if (functionName === "var" || functionName === "attr")
>> 
>> `attr(var(--something))` is not supported.
> 
> I wonder if we want to autocomplete `attr` on any property other than `content` given that it only works there 🤔

Improbable, but one could put it in a var and that still works.

```
--myvar: attr(class);
content: var(--myvar);
```

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:952
>> +        this._allCSSVariables = new Set;
> 
> This isn't really great IMO as a function named `_collectUsedCSSVariables` has a side effect of modifying `_allCSSVariables`.  I'd recommend renaming this function to something more generic (e.g. `_collectCSSVariables`) so that this is more "acceptable".

Yes, the naming is off indeed. I'll change.

I wanted to avoid doing another pass to grab all variables so I tried to leverage this loop here. 
Still scarred by the 1500 CSS variable scenarios of sites that dump all custom properties to the :root.

[Devin Rousso]

Comment on attachment 431858 [details]
WIP

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

>>>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:264
>>>> +    set values(values = [])
>>> 
>>> Doing this to ensure values can be added on-demand and the resulting list remains sorted.
>> 
>> This is a bit odd as `WI.CSSCompletions` is really meant to be basically readonly.  Instead of allowing the `WI.CSSCompletion` to be entirely modifiable, I wonder if we could just have an `addAll` (i.e. can only extend, not replace) or really more ideally have a way for `WI.CSSKeywordCompletions.forFunction` to do this logic before construction.
>> 
>> Also, having a default parameter here isn't ideal as that's only going to be used if `undefined` is provided, and we generally avoid using `undefined` instead of `null`.  I think you're better off having a `values ||= [];` inside the function instead (FWIW this is our normal pattern).
> 
> 

I think I'd either rename this to `addAll(values)` and keep mostly the same behavior (this is nice for extensibility in other scenarios) or modify `WI.CSSKeywordCompletions.forFunction` to also accept an optional `WI.CSSStyleDeclaration` which would instead take the place of the `completionControllerCSSFunctionValuesNeeded` delegate (this is also nice because then _every_ caller of `WI.CSSKeywordCompletions.forFunction` gets this behavior (assuming they provide a `WI.CSSStyleDeclaration`), instead of only the ones that have a delegate).

[Razvan Caliman]

Comment on attachment 431858 [details]
WIP

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

>>>>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:264
>>>>> +    set values(values = [])
>>>> 
>>>> Doing this to ensure values can be added on-demand and the resulting list remains sorted.
>>> 
>>> This is a bit odd as `WI.CSSCompletions` is really meant to be basically readonly.  Instead of allowing the `WI.CSSCompletion` to be entirely modifiable, I wonder if we could just have an `addAll` (i.e. can only extend, not replace) or really more ideally have a way for `WI.CSSKeywordCompletions.forFunction` to do this logic before construction.
>>> 
>>> Also, having a default parameter here isn't ideal as that's only going to be used if `undefined` is provided, and we generally avoid using `undefined` instead of `null`.  I think you're better off having a `values ||= [];` inside the function instead (FWIW this is our normal pattern).
>> 
>> 
> 
> I think I'd either rename this to `addAll(values)` and keep mostly the same behavior (this is nice for extensibility in other scenarios) or modify `WI.CSSKeywordCompletions.forFunction` to also accept an optional `WI.CSSStyleDeclaration` which would instead take the place of the `completionControllerCSSFunctionValuesNeeded` delegate (this is also nice because then _every_ caller of `WI.CSSKeywordCompletions.forFunction` gets this behavior (assuming they provide a `WI.CSSStyleDeclaration`), instead of only the ones that have a delegate).

I'd avoid passing in `WI.CSSStyleDeclaration` to `WI.CSSKeywordCompletions.forFunction()` because the Sources panel won't have one to pass in for context when getting all CSS variables. Its mechanism for getting CSS variables will be different because it's not constrained to the selected node.

Augmenting `WI.CSSKeywordCompletions.forFunction()` to get an optional list of extra items to pass to `WI.CSSCompletion` constructor will make it possible for Styles and Sources to pass in their own contextual data.

I'm thinking of something like:
```
WI.CSSKeywordCompletions.forFunction = function(functionName, extraFunctionCompletions ={})
{
    let suggestions = ["var()"];

    if (functionName === "var")
        suggestions = extraFunctionCompletions?.var || [];
...
   return new WI.CSSCompletions(suggestions, true);
}
```

[Razvan Caliman]

Created attachment 431894 [details]
WIP1.1

Address feedback

[Nikita Vasilyev]

Comment on attachment 431894 [details]
WIP1.1

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

Patch doesn't apply; looks like it needs to be rebaselined.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:267
> +        this._values = this._values.concat(values.slice());

Nit: No need to call `slice`; concat doesn't modify passed array.

[Razvan Caliman]

Created attachment 432065 [details]
Patch 1.0

Patch based on top of the one from bug 227097

[Patrick Angle]

Comment on attachment 432065 [details]
Patch 1.0

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

Awesome work, mostly just some thoughts on naming throughout.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:51
> +WI.CSSKeywordCompletions.forPartialPropertyValue = function(text, propertyName, {caretPosition, getFunctionValueCompletions} = {})

Nit: We might want to tweak this variable name, as currently it makes it sound like it is the only provider of function value completions, instead of an additional source of those completions. `additionalFunctionValueCompletionsProvider`?

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:50
> +        this._allCSSVariables = new Set;

Nit: If I understand correctly, this isn't really `all` CSS variables, but the `available` (not convinced this is precisely the right prefix either) CSS variables.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:342
> +    getFunctionValueCompletions(functionName)

See CSSKeywordCompletions:51

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:348
> +        if (functionName === "var")
> +            return Array.from(this._property.ownerStyle.nodeStyles.allCSSVariables);
> +
> +        if (functionName === "attr")
> +            return this._property.ownerStyle.node.attributes().map((attribute) => attribute.name);

Any reason not to express this as a switch statement? I know with two cases that's a bit of a wash, but I could see us needing to add to this someday. I'm fine either way, just something to consider.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:926
> +        return WI.CSSKeywordCompletions.forPartialPropertyValue(text, this._nameElement.textContent.trim(), {caretPosition, getFunctionValueCompletions: this.getFunctionValueCompletions.bind(this)});

See CSSKeywordCompletions:51

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:70
> +                const getFunctionValueCompletions = (functionName) => {

Can we declare this constant outside of compare expectations? Or, perhaps even better, could we make the additional `var` and `attr` completions optional parameters to `compareExpectations()`? In my opinion `compareExpectations` should do as little as possible to influence the outcome of what we are testing, and where it does it should do so with information from it's callers instead of inferring a non-empty set of these additional completions.

Also, I understand why you used this exact name (note from CSSKeywordCompletions:51 not withstanding), but would it be clearer that this is being somewhat fakes for these tests to have `mock` in the name somewhere?

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:146
> +            // attr(|

Nit: Can you put the value and caret inside backticks like the examples above, please. Ditto for below comments as well.

[Razvan Caliman]

Created attachment 432157 [details]
Patch 1.1

Address feedback; Patch based on top of the one from bug 227097

[Razvan Caliman]

Comment on attachment 432065 [details]
Patch 1.0

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

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:51
>> +WI.CSSKeywordCompletions.forPartialPropertyValue = function(text, propertyName, {caretPosition, getFunctionValueCompletions} = {})
> 
> Nit: We might want to tweak this variable name, as currently it makes it sound like it is the only provider of function value completions, instead of an additional source of those completions. `additionalFunctionValueCompletionsProvider`?

Yes, that's a good idea. Updated.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:50
>> +        this._allCSSVariables = new Set;
> 
> Nit: If I understand correctly, this isn't really `all` CSS variables, but the `available` (not convinced this is precisely the right prefix either) CSS variables.

The scope here is the styles for the selected DOM node. In that sense, these are all CSS variables applicable to it.
I'm not sure if the `available` prefix would clarify it any more. 

The case for using `_all` in the context of this file is to distinguish that `_usedCSSVariables` will be a subset of `_allCSSVariables`. 
Still open to a better naming suggestion. I considered dropping `_all` in favor of just `_CSSVariables` but that just adds to the ambiguity.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:348
>> +            return this._property.ownerStyle.node.attributes().map((attribute) => attribute.name);
> 
> Any reason not to express this as a switch statement? I know with two cases that's a bit of a wash, but I could see us needing to add to this someday. I'm fine either way, just something to consider.

I tried a `switch` statement before, but it looked needlessly complex for the two cases currently handled. 
When more cases are added, perhaps it will then make more sense to switch to `switch` :)

>> LayoutTests/inspector/unit-tests/css-keyword-completions.html:70
>> +                const getFunctionValueCompletions = (functionName) => {
> 
> Can we declare this constant outside of compare expectations? Or, perhaps even better, could we make the additional `var` and `attr` completions optional parameters to `compareExpectations()`? In my opinion `compareExpectations` should do as little as possible to influence the outcome of what we are testing, and where it does it should do so with information from it's callers instead of inferring a non-empty set of these additional completions.
> 
> Also, I understand why you used this exact name (note from CSSKeywordCompletions:51 not withstanding), but would it be clearer that this is being somewhat fakes for these tests to have `mock` in the name somewhere?

Extracted the mock provider and added a mock to its name.

Additionally, I configured `compareExpectations()` to accept an argument for a function that provides the contextual function values – the mock.
It did add verbosity to the test cases. I'm not convinced it was worthwhile given that this is a unit-test. But I don't have strong opinions either way.

[Razvan Caliman]

> Or, perhaps even better, could we make the additional `var` and `attr` completions optional parameters to `compareExpectations()`?

On closer reading of your comment, I think I understand what you meant. 

Essentially, the test is checking that the provided `additionalFunctionValueCompletionsProvider()` gets called by `WI.CSSKeywordCompletions.forPartialPropertyValue` and its results are appended to the list of completions and sorted.

Therefore, the mock itself doesn't have to contain any logic. The mock can just be a simple function that returns the list of additional completions. The test will check if these were appended, sorted and filtered accordingly.

Ex:

```
// `attr(|`
compareExpectations("attr(", "content", {
    expectedPrefix: "",
    expectedCompletions: ["class", "id", "var()"],
    additionalFunctionValueCompletionsProvider: () => { return ["class", "id"] },
})
```

Do let me know if I misread your suggestion.

[Razvan Caliman]

Created attachment 432158 [details]
Patch 1.2

Update test to add inline mocks instead of global mock; Patch based on top of the one from bug 227097

[Devin Rousso]

Comment on attachment 432158 [details]
Patch 1.2

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

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:266
> +        console.assert(Array.isArray(values), "Expected array of values for additional completion suggestions");

NIT: These kinds of comments are not useful IMO since this is basically redundant if one reads the code.  For simple assertions like this the code itself is enough.

NIT: For non-falsy checks please also include the argument in the assertion so that if a developer opens inspector^2 they can see what value was passed in
```
    console.assert(Array.isArray(values), values);
```

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:267
> +        this._values = this._values.concat(values);

Please use the utility added in r249301 :)
```
    this._values.pushAll(values);
```

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:122
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:344
> +        if (functionName === "var")

Personally I prefer using `switch` even for a small number of `case` as it makes future patches cleaner and keeps the blame very straightforward.  Up to you tho :)

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:68
> +                additionalFunctionValueCompletionsProvider ??= () => {}

Style: missing semicolon

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:140
> +                additionalFunctionValueCompletionsProvider: () => { return ["class", "id"] },

You can drop the `{ return` and `}` for these.  You only have to have that for multi-line arrow functions (in accordance with our style) or when the returned value is an object literal.

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:148
> +                additionalFunctionValueCompletionsProvider: () => { return ["class", "id"] },

ditto (:140)

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:155
> +                additionalFunctionValueCompletionsProvider: () => { return ["--one", "--two"] },

ditto (:140)

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:162
> +                additionalFunctionValueCompletionsProvider: () => { return ["--one", "--two"] },

ditto (:140)

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:170
> +                additionalFunctionValueCompletionsProvider: () => { return ["--one", "--two"] },

ditto (:140)

[Razvan Caliman]

Comment on attachment 432158 [details]
Patch 1.2

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

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:267
>> +        this._values = this._values.concat(values);
> 
> Please use the utility added in r249301 :)
> ```
>     this._values.pushAll(values);
> ```

Neat!

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:344
>> +        if (functionName === "var")
> 
> Personally I prefer using `switch` even for a small number of `case` as it makes future patches cleaner and keeps the blame very straightforward.  Up to you tho :)

Yeah, the argument about a cleaner `blame` is a good one.

[Razvan Caliman]

Created attachment 432696 [details]
Patch 1.3

Rebased onto master after Bug 227097 landed. Addressed code review feedback.

[Devin Rousso]

Comment on attachment 432696 [details]
Patch 1.3

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

r=me, nice work :)

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:114
> +        completions.addValues(contextualValueCompletions);

We should either not call this if `contextualValueCompletions` is empty or have a check inside `addValues` that bails if the provided array is empty so that we don't `sort()` unnecessarily.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:347
> +            return Array.from(this._property.ownerStyle.nodeStyles.allCSSVariables);
> +        case "attr":

Style: missing newline between

[Razvan Caliman]

Comment on attachment 432696 [details]
Patch 1.3

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

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:114
>> +        completions.addValues(contextualValueCompletions);
> 
> We should either not call this if `contextualValueCompletions` is empty or have a check inside `addValues` that bails if the provided array is empty so that we don't `sort()` unnecessarily.

Good idea! I'll add the check to `WI.Completions.addValues()`

[Razvan Caliman]

Created attachment 432784 [details]
Patch 1.4

Carry over R+; Address last nits

[EWS]

Committed r279502 (239354@main): <https://commits.webkit.org/239354@main>

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