Bug 230351 – Web Inspector: Support fuzzy matching in CSS completions

• Component: Web Inspector
• Status: RESOLVED FIXED

[Razvan Caliman]

<rdar://82976292>

[Razvan Caliman]

Created attachment 438354 [details]
Prototype

Messy and buggy prototype.

[Razvan Caliman]

Created attachment 440968 [details]
WIP Patch

Work In Progress: extend CSSCompletions to support fuzzy matching. Update consumers in SpreadsheetTextProperty and SpreadsheetTextField

[Razvan Caliman]

Created attachment 444271 [details]
WIP Patch

Work In Progress: add CSSQueryController, add fuzzyMatching setting, divorce CSS name and value text field value from their text contents

[Razvan Caliman]

Created attachment 444582 [details]
Patch 1.0

Implementation of fuzzy matching for completions of CSS properties and values in the Styles details sidebar.

[Devin Rousso]

Comment on attachment 444582 [details]
Patch 1.0

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

> Source/WebInspectorUI/ChangeLog:9
> +        Use fuzzy matching for identifying CSS completions in the Styles detalils sidebar.

detalils

> Source/WebInspectorUI/UserInterface/Base/Setting.js:234
> +    experimentalFuzzyMatchingStyles: new WI.Setting("experimental-fuzzy-matching-styles", false),

This should have "completion" somewhere in the name since that's what it controls.  Maybe `experimentalCSSCompletionFuzzyMatching`?

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:30
> +        console.assert(Array.isArray(values), values);

Style: I'd add a newline after this

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:33
> +        this._values = values ?? [];

We normally only use `??` when we care about falsy values other than `null`/`undefined` (e.g. if the value is a number, we may want to include `0` as a valid input).  Since this is an array, we should ignore all falsy values and use `||` instead.

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:92
> +        // 2. Uppercase characters that follow a lowercase letter.

Is this used for CSS variables (e.g. `--myColor`)?  Or was this just a copypasta from `WI.ResourceQueryController`?

Speaking of, is it even possible for CSS variables to reach this point?

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:101
> +                isSpecial = true;

It seems a bit odd to me that "-"is considered a special character when it exists _so_ prevalently in CSS.  Perhaps this is trying to get something like `border--right-` to match `border-top-right-radius`?

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

this._queryController?.addValues(values);

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:265
> +        if (!this._queryController)
> +            this._queryController = new WI.CSSQueryController(this._values);

this._queryController ||= new WI.CSSQueryController(this._values);

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:97
> +    if (currentTokenValue === "(" || currentTokenValue === ")" || currentTokenValue === ",")

What about spaces (e.g. `border: 1px solid red`)?

> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.css:73
> +.completion-suggestions-container > .item > .highlighted {

NIT: Normally the word "highlighted" implies a different background color, not text styling.

Regardless, can we use a more specific/explanatory name for this (e.g. "matched" which is also used for a similar thing with CSS selectors)?

> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:191
> +                let highlightedResultFragment = this._createHighlightedCompletionFragment(completion.value, completion.matchingTextRanges);
> +                itemElement.appendChild(highlightedResultFragment);

NIT: I'd inline this

> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:208
> +    }

We should have some sort of
```
    console.assert(false, "not reached");
    return "";
```
at the end just in case.

> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:231
> +            highlightSpan.classList.add("highlighted");

ditto (Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.css:73)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:989
> +    _valueCompletionDataProvider(text, {caretPosition, allowEmptyPrefix, fuzzyMatching} = {})

Frankly at this point it'd probably be easier to turn this into a `options = {}` so that we don't have to keep updating all the various callsites that're just a passthrough.
```
    _valueCompletionDataProvider(text, options = {})
    {
        options.additionalFunctionValueCompletionsProvider = this.additionalFunctionValueCompletionsProvider.bind(this);
        return WI.CSSKeywordCompletions.forPartialPropertyValue(text, this._nameElement.textContent.trim(), options);
    }
```

ditto for `_nameCompletionDataProvider` above

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:66
> +    get value() { return this._pendingValue ?? this._element.textContent; }

Style: This is no longer a simple `get`, so it (and the corresponding `set`) should be expanded onto multiple lines.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:67
>      set value(value) { this._element.textContent = value; }

Should this have any consideration for `_pendingValue`?  Maybe a `console.assert(!this._pendingValue)`?  Or a `this._pendingValue = null;`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:425
> +        const fuzzyMatching = WI.settings.experimentalFuzzyMatchingStyles.value;

Style: We only use `const` for things that do not change between Web Inspector sessions (i.e. actual constants).  Please use `let`.

> LayoutTests/inspector/unit-tests/css-query-controller.html:38
> +                    padvalue: "a-bc_de.f",
> +                    expected: "^^^ ^  ^",

I thought `_` and `.` are NOT considered special characters?

> LayoutTests/inspector/unit-tests/css-query-controller.html:46
> +                    padvalue: "ab-c_d.ef",
> +                    expected: "^ ^^^ ^",

ditto (:37)

> LayoutTests/inspector/unit-tests/css-query-controller.html:70
> +        name: "ExecuteQueryAgainstNoResources",

oops?  It seems like this may have been a mostly-copypasta from test for `WI.ResourceQueryController` :/

[Patrick Angle]

Comment on attachment 444582 [details]
Patch 1.0

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

> Source/WebInspectorUI/ChangeLog:77
> +        * UserInterface/Views/SpreadsheetTextField.js:

It would probably be worthwhile to explain the class variables that do /almost/ the same thing as they previous did. (e.g. what became what, and why.) For example suggestionHint -> _completionText. Also particularly for this class it would be helpful to be a bit more verbose about the changes you've made here with details in the key methods below, instead of just a list of general changes underneath the list of methods.

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:73
> +        // Results are sorted in descending order by rank.
> +        // Results of equal rank are sorted alphabetically by value.

NIT: Comments are redundant with code below.

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:83
> +    _findSpecialCharacterIndices(string)

Is it necessary to duplicate this logic both here and in `WI.ResourceQueryController`? Could this function instead be a static method of `WI.QueryController` that takes both a string to search as well as a string/list of separators that each subclass keeps a `const` for?

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:34
> +WI.CSSKeywordCompletions.forPartialPropertyName = function(text, {caretPosition, allowEmptyPrefix, fuzzyMatching} = {})

NIT: `useFuzzyMatching`?

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

Ditto :34?

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:97
>> +    // If the current token value is a comma or parenthesis, treat it as if we are at the start of a new token.
>> +    if (currentTokenValue === "(" || currentTokenValue === ")" || currentTokenValue === ",")
> 
> What about spaces (e.g. `border: 1px solid red`)?

Is this actually correct? Shouldn't we need to make sure there is a space between the `)` and the cursor in order to consider ourselves at the start of a new token, since as far as I understand it isn't valid to run two functions right up against each other without a space in between? e.g. `border: var(--awesome-color)|` shouldn't offer completions, but `border: var(--awesome-color) |` should?

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:123
>      }
> +    else

Style: This should be on a single line (e.g. `} else`)

> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:197
>              if (this._delegate && typeof this._delegate.completionSuggestionsViewCustomizeCompletionElement === "function")
> -                this._delegate.completionSuggestionsViewCustomizeCompletionElement(this, itemElement, completions[i]);
> +                this._delegate.completionSuggestionsViewCustomizeCompletionElement(this, itemElement, completion);

Nit: While you are here can we update this to use optional chaining like you did on :94 and :105

> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:232
> +            highlightSpan.append(completionText.substring(textRange.startColumn, textRange.endColumn));

I know I can be a bit assertion-happy at times, but I think we may want to consider asserting that the start and end points are within the completion text, since this substring call makes that assumption.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:965
> +    _nameCompletionDataProvider(text, {caretPosition, allowEmptyPrefix, fuzzyMatching} = {})

Nit: `useFuzzyMatching`?

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:989
>> +    _valueCompletionDataProvider(text, {caretPosition, allowEmptyPrefix, fuzzyMatching} = {})
> 
> Frankly at this point it'd probably be easier to turn this into a `options = {}` so that we don't have to keep updating all the various callsites that're just a passthrough.
> ```
>     _valueCompletionDataProvider(text, options = {})
>     {
>         options.additionalFunctionValueCompletionsProvider = this.additionalFunctionValueCompletionsProvider.bind(this);
>         return WI.CSSKeywordCompletions.forPartialPropertyValue(text, this._nameElement.textContent.trim(), options);
>     }
> ```
> 
> ditto for `_nameCompletionDataProvider` above

Ditto :965?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:32
> +        this._value = element.textContent;

This appears to be unused?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:-99
> -
> -        // Removing the suggestion hint element may leave the contents of `_element` fragmented into multiple text nodes.
> -        this._combineEditorElementChildren();

Why remove this? Is it no longer possible for multiple dismissed completions to result in an increasingly fragmented list of children elements?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:133
> +        this._completionText = "";

This variable should be defined in the constructor as well.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:546
> +        let caretPosition = this._getCaretPosition();
> +        let newCaretPosition = moveCaretToEndOfCompletion ? caretPosition - this._completionPrefix.length + this._completionText.length : null;

I don't think this is right - having a null caret position shouldn't work when we use it on :553 with `setBaseAndExtent()`. I think you need to replace `null` with `caretPosition` here.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:548
> +        // Setting the value collapses the text selection. Get the caret position before doing this.

Nit: Odd that this comment applies to both this line as well as the preceding lines as well. I'd just drop the bit about caret position.

> LayoutTests/inspector/unit-tests/css-query-controller.html:17
> +                    padvalue: "Abcd",

Nit: Can we just call this value and add three more spaces between `value:` and `"Abcd"` instead? Same for all cases below.

>> LayoutTests/inspector/unit-tests/css-query-controller.html:70
>> +        name: "ExecuteQueryAgainstNoResources",
> 
> oops?  It seems like this may have been a mostly-copypasta from test for `WI.ResourceQueryController` :/

`ExecuteQueryAgainstNoValues`?

[Razvan Caliman]

Comment on attachment 444582 [details]
Patch 1.0

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

>> Source/WebInspectorUI/UserInterface/Base/Setting.js:234
>> +    experimentalFuzzyMatchingStyles: new WI.Setting("experimental-fuzzy-matching-styles", false),
> 
> This should have "completion" somewhere in the name since that's what it controls.  Maybe `experimentalCSSCompletionFuzzyMatching`?

Done

>> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:30
>> +        console.assert(Array.isArray(values), values);
> 
> Style: I'd add a newline after this

Done.

>> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:33
>> +        this._values = values ?? [];
> 
> We normally only use `??` when we care about falsy values other than `null`/`undefined` (e.g. if the value is a number, we may want to include `0` as a valid input).  Since this is an array, we should ignore all falsy values and use `||` instead.

Done

>> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:73
>> +        // Results of equal rank are sorted alphabetically by value.
> 
> NIT: Comments are redundant with code below.

Done

>> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:83
>> +    _findSpecialCharacterIndices(string)
> 
> Is it necessary to duplicate this logic both here and in `WI.ResourceQueryController`? Could this function instead be a static method of `WI.QueryController` that takes both a string to search as well as a string/list of separators that each subclass keeps a `const` for?

The logic for identifying special characters is very likely to diverge between matching file paths and CSS properties and variables. 
Also, see below comment.

>> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:92
>> +        // 2. Uppercase characters that follow a lowercase letter.
> 
> Is this used for CSS variables (e.g. `--myColor`)?  Or was this just a copypasta from `WI.ResourceQueryController`?
> 
> Speaking of, is it even possible for CSS variables to reach this point?

For now, it's copied from `WI.ResourceQueryController` verbatim, but I expect to make changes here in an upcoming patch.

Yes, CSS variables are already supported as-is. I haven't yet made particular tweaks for them, but I might refine condition, either dropping it all together or expanding it to consider all uppercase characters special given that CSS variables are case-sensitive.

>> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:101
>> +                isSpecial = true;
> 
> It seems a bit odd to me that "-"is considered a special character when it exists _so_ prevalently in CSS.  Perhaps this is trying to get something like `border--right-` to match `border-top-right-radius`?

Special characters have the role of boosting the rank of a result when matched or boosting the weight of a character adjacent to it when not matched. `See WI.QueryResult._calculateRank()`

For example:
query = "r"
results in ranked order: [r]esize, clip-[r]ule, g[r]id

> Perhaps this is trying to get something like `border--right-` to match `border-top-right-radius`?
Yes, that too.

There's more tweaking we need to do to the ranking algorithm. Right now it's copied verbatim from `WI.ResourceQueryController` where special characters in filenames have more meaning than the humble dash in CSS. Adjustments will happen in follow-ups.

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

Done

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:265
>> +            this._queryController = new WI.CSSQueryController(this._values);
> 
> this._queryController ||= new WI.CSSQueryController(this._values);

Done.

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:34
>> +WI.CSSKeywordCompletions.forPartialPropertyName = function(text, {caretPosition, allowEmptyPrefix, fuzzyMatching} = {})
> 
> NIT: `useFuzzyMatching`?

Yes, sounds better.

>>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:97
>>> +    if (currentTokenValue === "(" || currentTokenValue === ")" || currentTokenValue === ",")
>> 
>> What about spaces (e.g. `border: 1px solid red`)?
> 
> Is this actually correct? Shouldn't we need to make sure there is a space between the `)` and the cursor in order to consider ourselves at the start of a new token, since as far as I understand it isn't valid to run two functions right up against each other without a space in between? e.g. `border: var(--awesome-color)|` shouldn't offer completions, but `border: var(--awesome-color) |` should?

> What about spaces (e.g. `border: 1px solid red`)?
This worked before and still works. 
The completion is offered for the token at the cursor position when the cursor is at the end of the token.
There's an open FIXME for mid-token: webkit.org/b/227157 Styles: Support completions mid-token. 

> I understand it isn't valid to run two functions right up against each other without a space in between? e.g. `border: var(--awesome-color)|` shouldn't offer completions, but `border: var(--awesome-color) |` should?
You are right. The change doesn't guard against that. 
It guards against showing suggestions for the query ")" which would match var(), env(), etc. I moved it to a separate check to return no completions at all.

STP 135 also show completions for characters immediately following a closing parenthesis. So this isn't a new issue. I'd investigate + fix that in a follow-up.

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:123
>> +    else
> 
> Style: This should be on a single line (e.g. `} else`)

Done

>> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.css:73
>> +.completion-suggestions-container > .item > .highlighted {
> 
> NIT: Normally the word "highlighted" implies a different background color, not text styling.
> 
> Regardless, can we use a more specific/explanatory name for this (e.g. "matched" which is also used for a similar thing with CSS selectors)?

Done. Also renamed `_createHighlightedCompletionFragment()` -> `_createMatchedCompletionFragment()`

>> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:191
>> +                itemElement.appendChild(highlightedResultFragment);
> 
> NIT: I'd inline this

Done

>> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:197
>> +                this._delegate.completionSuggestionsViewCustomizeCompletionElement(this, itemElement, completion);
> 
> Nit: While you are here can we update this to use optional chaining like you did on :94 and :105

Good idea! Done. 

I wanted to update other similar sequences around this part of the code, but the patch is already sprawling. May happen in a follow-up. Optional chaining is beautiful

>> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:208
>> +    }
> 
> We should have some sort of
> ```
>     console.assert(false, "not reached");
>     return "";
> ```
> at the end just in case.

Should never get here, but sure. Makes the linter happy too.
Also added an assert, because if we ever get a completion of a different type, somethings wrong.

>> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:231
>> +            highlightSpan.classList.add("highlighted");
> 
> ditto (Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.css:73)

Done

>> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:232
>> +            highlightSpan.append(completionText.substring(textRange.startColumn, textRange.endColumn));
> 
> I know I can be a bit assertion-happy at times, but I think we may want to consider asserting that the start and end points are within the completion text, since this substring call makes that assumption.

Good idea. Done.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:965
>> +    _nameCompletionDataProvider(text, {caretPosition, allowEmptyPrefix, fuzzyMatching} = {})
> 
> Nit: `useFuzzyMatching`?

Done

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:989
>>> +    _valueCompletionDataProvider(text, {caretPosition, allowEmptyPrefix, fuzzyMatching} = {})
>> 
>> Frankly at this point it'd probably be easier to turn this into a `options = {}` so that we don't have to keep updating all the various callsites that're just a passthrough.
>> ```
>>     _valueCompletionDataProvider(text, options = {})
>>     {
>>         options.additionalFunctionValueCompletionsProvider = this.additionalFunctionValueCompletionsProvider.bind(this);
>>         return WI.CSSKeywordCompletions.forPartialPropertyValue(text, this._nameElement.textContent.trim(), options);
>>     }
>> ```
>> 
>> ditto for `_nameCompletionDataProvider` above
> 
> Ditto :965?

Done

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:32
>> +        this._value = element.textContent;
> 
> This appears to be unused?

Indeed. Removed accidental leftover.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:67
>>      set value(value) { this._element.textContent = value; }
> 
> Should this have any consideration for `_pendingValue`?  Maybe a `console.assert(!this._pendingValue)`?  Or a `this._pendingValue = null;`?

Good idea resetting `this._pendingValue = null;`

I'm hesitant to assert whether it's null beforehand. 

I have a patch in mind for CSS property name + value completion at the same time (rdar://82976526) where the CSS property value `SpreadsheetTextField` can be changed from the outside (`SpreadsheetStyleProperty`) when the completion includes the value.

For example "display: block" -> apply "display" to name, "block" to value

In that case, there won't be a pending value for the CSS value `SpreadsheetTextField` and the assert will trip.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:-99
>> -        this._combineEditorElementChildren();
> 
> Why remove this? Is it no longer possible for multiple dismissed completions to result in an increasingly fragmented list of children elements?

I believe so. If you have a set of steps to reproduce, please share. I tried dismissing multiple completions and did not get fragmentation.

When completions are dismissed, the pending value is reset to the current value without the suggestion hint:
`if (hadCompletionText) {
            this._pendingValue = this.valueWithoutSuggestion();
`

On blur, Tab, click or ArrowRight, the pending value replaces the `textContent` of the element. That's supposed to collapse all text nodes into one.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:133
>> +        this._completionText = "";
> 
> This variable should be defined in the constructor as well.

Done

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:425
>> +        const fuzzyMatching = WI.settings.experimentalFuzzyMatchingStyles.value;
> 
> Style: We only use `const` for things that do not change between Web Inspector sessions (i.e. actual constants).  Please use `let`.

Ok. Subtle, but fair.
I got used to the pattern where `const` is used for a self-explanatory argument name, usually booleans, passed to methods.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:546
>> +        let newCaretPosition = moveCaretToEndOfCompletion ? caretPosition - this._completionPrefix.length + this._completionText.length : null;
> 
> I don't think this is right - having a null caret position shouldn't work when we use it on :553 with `setBaseAndExtent()`. I think you need to replace `null` with `caretPosition` here.

Good catch!

I missed that the now removed `_combineEditorElementChildren()` handled the `null` caret position by requesting it again.
As I moved logic around, I forgot about that aspect.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:548
>> +        // Setting the value collapses the text selection. Get the caret position before doing this.
> 
> Nit: Odd that this comment applies to both this line as well as the preceding lines as well. I'd just drop the bit about caret position.

I got tripped by this subtle behavior and considered the comment warranted. 
If you don't feel strongly about it, I'd like to keep it to warn future me/ future others.
I can see an enthusiastic soul trying to reorder these lines and be smitten with strange errors.

>> LayoutTests/inspector/unit-tests/css-query-controller.html:17
>> +                    padvalue: "Abcd",
> 
> Nit: Can we just call this value and add three more spaces between `value:` and `"Abcd"` instead? Same for all cases below.

I tossed-and-turned about this for a while. It looks _strange_ both ways. 
TBH, it's cleaner like this than having to deal with keys with whitespace in the iterator below.

If you don't feel strongly about it, I'd keep it as-is.

>> LayoutTests/inspector/unit-tests/css-query-controller.html:38
>> +                    expected: "^^^ ^  ^",
> 
> I thought `_` and `.` are NOT considered special characters?

Indeed, they are not.

TIL: It's mistakenly considered uppercase in `WI.CSSQueryController._findSpecialCharacterIndices()`:

`Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:105`
```
else if (character.isUpperCase() && previousCharacter.isLowerCase())
    isSpecial = true;
```

It's the `String.isUppercase()` utility in:

`Source/WebInspectorUI/UserInterface/Base/Utilities.js:780`
```
Object.defineProperty(String.prototype, "isUpperCase",
{
    value()
    {
        return String(this) === this.toUpperCase();
    }
});
```

This means the check is mistaken for all non-alpha characters both in `WI.CSSQueryController` and `WI.ResourceQueryController`.

I could fix it with a regex test either in `_findSpecialCharacterIndices()` or directl in the utilities:

```
Object.defineProperty(String.prototype, "isLowerCase",
{
    value()
    {
        return new RegExp(`[a-z]{${this.length}}`).test(this);
    }
});

Object.defineProperty(String.prototype, "isUpperCase",
{
    value()
    {
        return new RegExp(`[A-Z]{${this.length}}`).test(this);
    }
});
```

WDYT?

>>> LayoutTests/inspector/unit-tests/css-query-controller.html:70
>>> +        name: "ExecuteQueryAgainstNoResources",
>> 
>> oops?  It seems like this may have been a mostly-copypasta from test for `WI.ResourceQueryController` :/
> 
> `ExecuteQueryAgainstNoValues`?

Yup, copypasta.  Fixed.

[Razvan Caliman]

Created attachment 444692 [details]
Patch 1.1

Address code review feedback.

[Devin Rousso]

Comment on attachment 444692 [details]
Patch 1.1

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:776
> +        return new RegExp(`[a-z]{${this.length}}`).test(this);

NIT: Why not just `/[a-z]*/.test(this)`?

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:784
> +        return new RegExp(`[A-Z]{${this.length}}`).test(this);

NIT: Why not just `/[A-Z]*/.test(this)`?

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:100
> +    // It's not valid CSS to append completions immediately after a closing parenthesis.

Can we add some tests for this?

> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:199
> +        console.assert(typeof completion === "string" || completion instanceof WI.QueryResult);

NIT: I'd add `completion` as another argument so that if this fails we can see what the value is in the inspector^2 console.

> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:228
> +            console.assert(textRange.startColumn >= 0 && textRange.startColumn < completionText.length, "Match start out of bounds.");
> +            console.assert(textRange.endColumn > 0 && textRange.endColumn <= completionText.length, "Match end out of bounds.");

ditto (:199)

NIT: Also, these messages are redundant if one just looks at the conditions.  I'd remove them.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:73
> +        this._pendingValue = null;

Style: I'd add a newline before this

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:154
> +        let hadCompletionText = !!this._completionText;

Since `_completionText` is always reset to `""`, I think it'd be better to check `.length` instead of `!!`.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:155
> +        // Resetting the suggestion hint removes any suggestion hint element that is attached.

Style: I'd add a newline before this

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:383
> +        if (event.key === "ArrowLeft" && (this._completionText || this._suggestionsView.visible)) {

ditto (:154)

> LayoutTests/inspector/unit-tests/css-query-controller.html:37
> +                    padvalue: "a-bc_de.f",

These really look more like filenames than CSS values.  Can we replace these with things one is more like to see in CSS, ideally even actual values (e.g. `margin-right`, `--test-kebab`, `--test_snake`, `--testCamel`, `var(--foo)`, `rgb(255, 255, 255)`, etc.).

[Razvan Caliman]

Comment on attachment 444692 [details]
Patch 1.1

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

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:776
>> +        return new RegExp(`[a-z]{${this.length}}`).test(this);
> 
> NIT: Why not just `/[a-z]*/.test(this)`?

[a-z]* means zero or more matches. That will return false positives when checking if ALL characters are lowercase for:
/[a-z]*/.test("")
/[a-z]*/.test("A")
/[a-z]*/.test("Aa")
/[a-z]*/.test("-a")

The `String.prototype.isLowerCase` is used now just on single character strings. But it's an extension of String and it's reasonable to expect it will be used for longer strings. Checking that ALL characters in the string pass the regex test makes this method versatile.

Ditto for `String.prototype.isUpperCase()`

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:100
>> +    // It's not valid CSS to append completions immediately after a closing parenthesis.
> 
> Can we add some tests for this?

Done.

>> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:199
>> +        console.assert(typeof completion === "string" || completion instanceof WI.QueryResult);
> 
> NIT: I'd add `completion` as another argument so that if this fails we can see what the value is in the inspector^2 console.

Good idea. Done.

>> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:228
>> +            console.assert(textRange.endColumn > 0 && textRange.endColumn <= completionText.length, "Match end out of bounds.");
> 
> ditto (:199)
> 
> NIT: Also, these messages are redundant if one just looks at the conditions.  I'd remove them.

Done.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:73
>> +        this._pendingValue = null;
> 
> Style: I'd add a newline before this

Done.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:154
>> +        let hadCompletionText = !!this._completionText;
> 
> Since `_completionText` is always reset to `""`, I think it'd be better to check `.length` instead of `!!`.

Done.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:155
>> +        // Resetting the suggestion hint removes any suggestion hint element that is attached.
> 
> Style: I'd add a newline before this

Done.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:383
>> +        if (event.key === "ArrowLeft" && (this._completionText || this._suggestionsView.visible)) {
> 
> ditto (:154)

Fixed.

>> LayoutTests/inspector/unit-tests/css-query-controller.html:37
>> +                    padvalue: "a-bc_de.f",
> 
> These really look more like filenames than CSS values.  Can we replace these with things one is more like to see in CSS, ideally even actual values (e.g. `margin-right`, `--test-kebab`, `--test_snake`, `--testCamel`, `var(--foo)`, `rgb(255, 255, 255)`, etc.).

Updated the list of tests to check strings usually found in CSS property names and values.

Added underscore "_" as a special character because it's possible to find it in variable names. Thanks for the tip!

I didn't mess with the matching logic just yet. CSS variable names are exceptionally permissive. 
We can decide in a later patch if we want to add special meaning for case in variables and/or additional special characters.

For now, special characters serve just as a booster when ranking results if the query includes them or finds matches adjacent to them. 
Results are not dropped if their special characters are not yet handled (for example parentheses, dots or commas). They're just ranked on equal terms as for regular characters.

[Razvan Caliman]

Created attachment 445027 [details]
Patch 1.2

Address review feedback: improve tests, fix nits

[Devin Rousso]

Comment on attachment 444692 [details]
Patch 1.1

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

>>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:776
>>> +        return new RegExp(`[a-z]{${this.length}}`).test(this);
>> 
>> NIT: Why not just `/[a-z]*/.test(this)`?
> 
> [a-z]* means zero or more matches. That will return false positives when checking if ALL characters are lowercase for:
> /[a-z]*/.test("")
> /[a-z]*/.test("A")
> /[a-z]*/.test("Aa")
> /[a-z]*/.test("-a")
> 
> The `String.prototype.isLowerCase` is used now just on single character strings. But it's an extension of String and it's reasonable to expect it will be used for longer strings. Checking that ALL characters in the string pass the regex test makes this method versatile.
> 
> Ditto for `String.prototype.isUpperCase()`

Oh oops I totally forgot the `$` and `^` in the regex.  The overarching point I was trying to make is "can we do this with a declared regex instead of a dynamic one?".  It just seems odd to me that we care about `this.length` at all when we really just want to know "are all the characters lowercase?".

[Devin Rousso]

Comment on attachment 445027 [details]
Patch 1.2

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

> Source/WebInspectorUI/ChangeLog:211
> -
> +        

oops?

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:776
> +        return new RegExp(`[a-z]{${this.length}}`).test(this);

please see comment #12

also, please add tests for this

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:784
> +        return new RegExp(`[A-Z]{${this.length}}`).test(this);

ditto (:776)

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:102
> +    if (currentTokenValue === ")" || tokenBeforeCaret?.value.trim() === ")")

Does `CodeMirror` include whitespace in tokens?If not, the `trim()` is unnecessary.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:71
> +    set value(value) {

Style: `{` should be on a separate line

> LayoutTests/inspector/unit-tests/css-query-controller.html:141
> +                    value: "abcde",
> +                    expected: "abcd"

NIT: It'd be nice to align these horizontally (e.g. do `padvalue` like above) so one can more easily map the query to the expected matches :)

[Razvan Caliman]

Comment on attachment 445027 [details]
Patch 1.2

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

>> Source/WebInspectorUI/ChangeLog:211
>> +        
> 
> oops?

Fixed.

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:776
>> +        return new RegExp(`[a-z]{${this.length}}`).test(this);
> 
> please see comment #12
> 
> also, please add tests for this

Done. Added tests in `LayoutTests/inspector/unit-tests/string-utilities.html`

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:784
>> +        return new RegExp(`[A-Z]{${this.length}}`).test(this);
> 
> ditto (:776)

Done.

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:102
>> +    if (currentTokenValue === ")" || tokenBeforeCaret?.value.trim() === ")")
> 
> Does `CodeMirror` include whitespace in tokens?If not, the `trim()` is unnecessary.

The `trim()` is indeed unnecessary.
`CodeMirror` doesn't use `CSSKeywordCompletions.forPartialPropertyValue()`. It has its own matching logic.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:71
>> +    set value(value) {
> 
> Style: `{` should be on a separate line

Fixed.

[Razvan Caliman]

(In reply to Devin Rousso from comment #12)

> Oh oops I totally forgot the `$` and `^` in the regex.

Great idea! It's nicer without the added complexity of string length.

I added the constraint for one or more characters, though: `/^[a-z]+$/.test(this)`
That's so empty strings don't match unnecessarily.

[Razvan Caliman]

Created attachment 446169 [details]
Patch 1.3

Address code review feedback

[Devin Rousso]

Comment on attachment 446169 [details]
Patch 1.3

r=me, nice work!

[Razvan Caliman]

Thank you!

[EWS]

Committed r286611 (244935@trunk): <https://commits.webkit.org/244935@trunk>

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

[Razvan Caliman]

Created attachment 449384 [details]
Screenshot with patch applied