Bug 227097 – Web Inspector: Styles: Autocomplete should support function completions

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

[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 `margin: env(`

# EXPECTED
should show `safe-area-inset-top` etc. in autocompletion

# ACTUAL
no autocompletions

[Radar WebKit Bug Importer]

<rdar://problem/79418160>

[Devin Rousso]

note that this works as expected for CodeMirror editors (e.g. Sources Tab)

this is possibly/probably a regression from when the Styles panel moved off using CodeMirror

[Patrick Angle]

Created attachment 431750 [details]
Patch v1.0

[Patrick Angle]

Comment on attachment 431750 [details]
Patch v1.0

The changes to `WI.SpreadsheetStyleProperty` should be testable. I'll prepare some tests for them on Monday.

[Razvan Caliman]

Comment on attachment 431750 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:933
> +        let indexOfTokenAtCursur = -1;

Typo: indexOfTokenAtCursur/indexOfTokenAtCursor

[Devin Rousso]

Comment on attachment 431750 [details]
Patch v1.0

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

Awesome work!  Mostly style/NIT with some other questions.  Haven't had a chance to test it myself, so I'd love a screen recording if possible :)

> Source/WebInspectorUI/ChangeLog:9
> +        function, any meaningful function completions, and a lack of support for multi-line CSS property values. This 

s/function/functions

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

NIT: I feel like `text` is a better name than `value`

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

ditto (:889)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:924
> +        let tokens = WI.tokenizeCSSValue(value);

I wonder what the performance cost of this is compared to a regex match?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:929
> +            return { prefix: "", completions };

Style: we don't add spaces after `{` and before `}`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:936
> +        for (let i = 0; i < tokens.length; i++) {

Style: `++i`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:940
> +                cursorIsAtEndOfToken = caretPosition === passedCharacters;

NIT: you could move this out of the `for` entirely and instead do it all in one line after

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:945
> +        console.assert(tokens[indexOfTokenAtCursur], "Could not find token for cursor.", value, caretPosition);

NIT: Instead of writing `tokens[indexOfTokenAtCursor]` over and over, I'd pull it out into a local `tokenAtCursor` (and maybe rename the `nextToken` below to `tokenAfterCursor`)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:947
> +            return { prefix: "", completions: [] };

ditto (:929)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:950
> +        // If the cursor was in middle of a token or the next token is entirely valid characters for a value, we are effectively mid-token.

I'm a bit confused by this.  Does this mean that a situation like `margin: au| auto` would not autocomplete?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:953
> +            return { prefix: "", completions: [] };

ditto (:929)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:956
> +            return { prefix: "", completions: [] };

ditto (:929)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:967
> +        for (let i = indexOfTokenAtCursur; i >= 0; i--) {

Style: `--i`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:971
> +            if (value === ")") {

Style: no `{` `}` for single line control flow

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:972
> +                preceedingFunctionDepth++;

Style: `++preceedingFunctionDepth`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:975
> +                    preceedingFunctionDepth--;

Style: `--preceedingFunctionDepth`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:977
> +                    functionName = tokens[i-1]?.value;

Style: `tokens[i - 1]`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:985
> +            return { prefix: currentTokenValue, completions: WI.CSSKeywordCompletions.forFunction(functionName).startsWith(currentTokenValue) };

ditto (:929)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:987
> +        return { prefix: currentTokenValue, completions: WI.CSSKeywordCompletions.forProperty(propertyName).startsWith(currentTokenValue) };

ditto (:929)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:67
> +        return Array.from(this._element.childNodes).filter((node) => {

Style: I'm not sure if the style guide completely covers a case like this, but I've done stuff like this in the past
```
    Array.from(this._element.childNodes)
        .filter((node) => node !== this._suggestionHintElement)
        .map((node) => node.textContent)
        .join("");
```

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:158
> +        let unprefixedCompletion = selectedText.slice(this._completionPrefix.length);
> +        this.suggestionHint = unprefixedCompletion;

NIT: while this is not a problem right now, if `set suggestionHint` changes in the future such that the `value` is further manipulated, `unprefixedCompletion` will not get any of those modifications, so I'd instead suggest to just immediately assign to `this.suggestionHint` and then use the `get` instead of `unprefixedCompletion` below

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:466
> +            range.collapse();

Why?

[Patrick Angle]

Comment on attachment 431750 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:889
>> +    _nameCompletionDataProvider(value, {caretPosition, allowEmptyPrefix} = {})
> 
> NIT: I feel like `text` is a better name than `value`

Agreed.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:924
>> +        let tokens = WI.tokenizeCSSValue(value);
> 
> I wonder what the performance cost of this is compared to a regex match?

If we used regex here, we would still need to scan through some number of preceding characters below while determining function name, as we could be multiple levels of parenthesis deep.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:950
>> +        // If the cursor was in middle of a token or the next token is entirely valid characters for a value, we are effectively mid-token.
> 
> I'm a bit confused by this.  Does this mean that a situation like `margin: au| auto` would not autocomplete?

No, in that case the token after the caret will be whitespace and not match the regex below.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:67
>> +        return Array.from(this._element.childNodes).filter((node) => {
> 
> Style: I'm not sure if the style guide completely covers a case like this, but I've done stuff like this in the past
> ```
>     Array.from(this._element.childNodes)
>         .filter((node) => node !== this._suggestionHintElement)
>         .map((node) => node.textContent)
>         .join("");
> ```

👍🏻

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:466
>> +            range.collapse();
> 
> Why?

Looking at this again today, this isn't correct, as it places the completion box at caret instead of aligned with the completion. I'll fix this up. Still needs updated to not assume the range will be in the current container for multi-line situations and mid-line suggestions.

[Nikita Vasilyev]

Created attachment 431890 [details]
[Video] Unexpected behavior

I found case with unexpected behavior. Pressing ArrowRight key and moving the text caret inside of the suggestion should accept the suggestion (and not keep it visible and gray).

(I have "Debug -> Enable style editing debug mode" checked; you can ignore it.)

Overall, it's looking good!

[Nikita Vasilyev]

Comment on attachment 431750 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:952
> +        if (!cursorIsAtEndOfToken || (nextToken && /^[a-z0-9-]+.*$/i.test(nextToken.value)))

Do you intent to only test the 1st character of `nextToken.value`? If that's the case, this should be faster:
/[a-z0-9-]/i.test(nextToken.value[0])

[Nikita Vasilyev]

Comment on attachment 431750 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:-164
> -        // Consider the following example:
> -        //
> -        //   border: 1px solid ro|
> -        //                     rosybrown
> -        //                     royalblue
> -        //
> -        // Clicking on "rosybrown" should replace "ro" with "rosybrown".
> -        //
> -        //           prefix:  1px solid ro
> -        // completionPrefix:            ro
> -        //        newPrefix:  1px solid
> -        //     selectedText:            rosybrown

The patch no longer works correctly with the example in this comment you removed. Now it looks like this:

    border: 1px solid ro|
                        rosybrown

[Nikita Vasilyev]

Comment on attachment 431750 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:174
> +        // Make sure the caret is placed just after the newly committed characters.
> +        let textChildNode = this._element.childNodes[0];
> +        if (textChildNode) {
> +            let newSelectionRange = document.createRange();
> +            newSelectionRange.setEnd(textChildNode, endOfCompletionCaretPosition);
> +            newSelectionRange.collapse();
> +
> +            let selection = window.getSelection();
> +            selection.removeAllRanges();
> +            selection.addRange(newSelectionRange);
> +        }

This is 10 lines vs 1 line of setBaseAndExtent. Did setBaseAndExtent not work properly?

[Nikita Vasilyev]

Comment on attachment 431750 [details]
Patch v1.0

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

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:924
>>> +        let tokens = WI.tokenizeCSSValue(value);
>> 
>> I wonder what the performance cost of this is compared to a regex match?
> 
> If we used regex here, we would still need to scan through some number of preceding characters below while determining function name, as we could be multiple levels of parenthesis deep.

Generally, CSS values are short enough that tokenization should take <1ms. One exception is base64 encoded background images — I wonder what happens with `background: no-repeat url(data:image/png;base64,1-MB-IMAGE-HERE)` 😅

[Patrick Angle]

Comment on attachment 431750 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:-164
>> -        //     selectedText:            rosybrown
> 
> The patch no longer works correctly with the example in this comment you removed. Now it looks like this:
> 
>     border: 1px solid ro|
>                         rosybrown

Yes, this is a bug introduced below in _getCaretRect (:466) when working to fix it to work for multi-line situations and mid-line suggestions... I'll move the relevant chunk of this comment down there, but the second half of this comment no longer applies with the new logic and variables being used here, which is why I removed it.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:174
>> +        }
> 
> This is 10 lines vs 1 line of setBaseAndExtent. Did setBaseAndExtent not work properly?

`setBaseAndExtent` uses a number of nodes, not a number of characters for the offsets (https://developer.mozilla.org/en-US/docs/Web/API/Selection/setBaseAndExtent).

[Nikita Vasilyev]

Comment on attachment 431750 [details]
Patch v1.0

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

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:174
>>> +        }
>> 
>> This is 10 lines vs 1 line of setBaseAndExtent. Did setBaseAndExtent not work properly?
> 
> `setBaseAndExtent` uses a number of nodes, not a number of characters for the offsets (https://developer.mozilla.org/en-US/docs/Web/API/Selection/setBaseAndExtent).

It uses a number of characters when you provide it a text node.
selection.setBaseAndExtent(textNode, 0, textNode, 10) — this selects the 1st 10 characters, for example.

[Patrick Angle]

Created attachment 431936 [details]
Patch v1.1 - Addressed (hopefully all current) review feedback, resolved discovered regressions

[Patrick Angle]

Created attachment 431937 [details]
Video of Patch v1.1

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #9)
> Comment on attachment 431750 [details]
> Patch v1.0
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431750&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:952
> > +        if (!cursorIsAtEndOfToken || (nextToken && /^[a-z0-9-]+.*$/i.test(nextToken.value)))
> 
> Do you intent to only test the 1st character of `nextToken.value`? If that's
> the case, this should be faster:
> /[a-z0-9-]/i.test(nextToken.value[0])

This hasn't been addressed or justified, hasn't it?

[Patrick Angle]

Comment on attachment 431750 [details]
Patch v1.0

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

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:952
>>> +        if (!cursorIsAtEndOfToken || (nextToken && /^[a-z0-9-]+.*$/i.test(nextToken.value)))
>> 
>> Do you intent to only test the 1st character of `nextToken.value`? If that's the case, this should be faster:
>> /[a-z0-9-]/i.test(nextToken.value[0])
> 
> This hasn't been addressed or justified, hasn't it?

It hasn't, and that's on me - I missed this review note when I moved this code. I'll address it - you are correct in that only the first character should matter here.

[Nikita Vasilyev]

Comment on attachment 431936 [details]
Patch v1.1 - Addressed (hopefully all current) review feedback, resolved discovered regressions

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

Really nice to see tests for this ☺️

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:439
> +        multilineRange.selectNodeContents(this._element);
> +        multilineRange.setEnd(lineRange.endContainer, lineRange.endOffset);

Nit: it's not entirely clear what's the start of the range? Can we use `setStart`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:452
> +        if (range) {

Nit: This is never falsy, isn't it?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:487
> +        range.selectNodeContents(this._element);
> +        range.collapse();

Nit: any way we can be more explicit with what's happening here? It isn't entirely clear if `collapse()` collapses to start, end, focus, or anchor. Similar with `collapse` used elsewhere in the patch.

[Patrick Angle]

Created attachment 431983 [details]
Patch v1.2 - Review notes, address issue with adding empty suggestion hint element

[Devin Rousso]

Comment on attachment 431983 [details]
Patch v1.2 - Review notes, address issue with adding empty suggestion hint element

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

This is looking really good!  Awesome work adding tests :)

Given how complex and varying editing CSS is, though, I think some live-on might be a good idea for this.  I'd suggest committing this patch locally while working on other bugs and kinda trying to do more unusual things when editing CSS to see if maybe you can find other edge-cases like I did :)

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:55
> +    console.assert(caretPosition >= 0 && caretPosition <= text.length, "`caretPosition` must be an index within the `value`.", text, caretPosition);

NIT: I think the explanation here is unnecessary since the code is pretty self explanatory.  Also, it's technically not true since you allow for `caretPosition === text.length`, which is not "within" it :P

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:78
> +    console.assert(tokenAtCaret, "Could not find token for cursor.", text, caretPosition);

ditto (:55)

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:79
> +    if (!tokens[indexOfTokenAtCaret])

```
    if (!tokenAtCaret)
```

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:90
> +    if ((caretIsMidToken && !currentTokenIsEmpty) || (!caretIsMidToken && tokenAfterCaret && /[a-z0-9-]/i.test(tokenAfterCaret.value[0])))

Aside: I wonder if `/a-z/i` is better or worse for performance than `[a-zA-Z]`.  Personally I prefer the latter as it's more explicit and requires less thinking :P

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:446
> +

Style: I'd remove this newline

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:486
> +        const toStart = false;

I think you can drop this?  Usually optional arguments default to falsy anyways.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:498
>          this._element.textContent = this._element.textContent;

Aside: this kind of code is worthy of a comment IMO, as to any unfamiliar reader it seems like an identity operation and therefore possibly kinda pointless

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:501
> +            let textChildNode = this._element.childNodes[0];

`this._element.firstChild`?

NIT: The naming of this assumes that the first child is actually a text node.  I'd either rename this or add a `console.assert(textChildNode instanceof Text, textChildNode)`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:522
> +        this._element.insertBefore(this._suggestionHintElement, range.endContainer.splitText(range.endOffset));

I hit an exception on this line.

```
TypeError:​ range.endContainer.splitText is not a function. (In 'range.endContainer.splitText(range.endOffset)​', 'range.endContainer.splitText' is undefined)​ (at SpreadsheetTextField.js:​522:​93)​
    _reAttachSuggestionHint @ SpreadsheetTextField.js:​522:​93
    suggestionHint @ SpreadsheetTextField.js:​83:​41
    completionSuggestionsSelectedCompletion @ SpreadsheetTextField.js:​146:​13
    selectNext @ CompletionSuggestionsView.js:​94:​67
    _handleKeyDownForSuggestionView @ SpreadsheetTextField.js:​307:​49
    _handleKeyDown @ SpreadsheetTextField.js:​220:​64
    _handleKeyDown @ [native code]​
```

# STEPS TO REPRODUCE
1. inspect any page
2. add a new CSS property
3. type `margin` in for the name
4. type `auto auto` for the value
5. delete the first `auto` (empty completion should be shown)
6. type `i` (which will show `initial` as the completion)
7. delete the `i`
8. press the down key

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:8
> +    let suite = InspectorTest.createSyncSuite("CSSKeywordCompletions");

`WI.CSSKeywordCompletions`

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:11
> +        name: "CSSKeywordCompletions.forPartialPropertyName",

ditto (:8)

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:19
> +                InspectorTest.log(`- Testing "${text}" with "allowEmptyPrefix: ${allowEmptyPrefix}"`);

NIT: Instead of having sub-tests within each test case, perhaps just create a separate test case for each scenario?  This way you can also add a description for each so future readers can better understand what each scenario is for.

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:61
> +        name: "CSSKeywordCompletions.forPartialPropertyValue",

ditto (:8)

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:142
> +    <p>Testing CSSKeywordCompletions.</p>

ditto (:8)

[Patrick Angle]

Comment on attachment 431983 [details]
Patch v1.2 - Review notes, address issue with adding empty suggestion hint element

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

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:55
>> +    console.assert(caretPosition >= 0 && caretPosition <= text.length, "`caretPosition` must be an index within the `value`.", text, caretPosition);
> 
> NIT: I think the explanation here is unnecessary since the code is pretty self explanatory.  Also, it's technically not true since you allow for `caretPosition === text.length`, which is not "within" it :P

Good point.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:522
>> +        this._element.insertBefore(this._suggestionHintElement, range.endContainer.splitText(range.endOffset));
> 
> I hit an exception on this line.
> 
> ```
> TypeError:​ range.endContainer.splitText is not a function. (In 'range.endContainer.splitText(range.endOffset)​', 'range.endContainer.splitText' is undefined)​ (at SpreadsheetTextField.js:​522:​93)​
>     _reAttachSuggestionHint @ SpreadsheetTextField.js:​522:​93
>     suggestionHint @ SpreadsheetTextField.js:​83:​41
>     completionSuggestionsSelectedCompletion @ SpreadsheetTextField.js:​146:​13
>     selectNext @ CompletionSuggestionsView.js:​94:​67
>     _handleKeyDownForSuggestionView @ SpreadsheetTextField.js:​307:​49
>     _handleKeyDown @ SpreadsheetTextField.js:​220:​64
>     _handleKeyDown @ [native code]​
> ```
> 
> # STEPS TO REPRODUCE
> 1. inspect any page
> 2. add a new CSS property
> 3. type `margin` in for the name
> 4. type `auto auto` for the value
> 5. delete the first `auto` (empty completion should be shown)
> 6. type `i` (which will show `initial` as the completion)
> 7. delete the `i`
> 8. press the down key

Investigating this. I've found and fixed several other bugs living on this patch for the last week, but seem to have not run into this one yet 😅

>> LayoutTests/inspector/unit-tests/css-keyword-completions.html:19
>> +                InspectorTest.log(`- Testing "${text}" with "allowEmptyPrefix: ${allowEmptyPrefix}"`);
> 
> NIT: Instead of having sub-tests within each test case, perhaps just create a separate test case for each scenario?  This way you can also add a description for each so future readers can better understand what each scenario is for.

Good idea!

[Patrick Angle]

Comment on attachment 431983 [details]
Patch v1.2 - Review notes, address issue with adding empty suggestion hint element

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

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:522
>>> +        this._element.insertBefore(this._suggestionHintElement, range.endContainer.splitText(range.endOffset));
>> 
>> I hit an exception on this line.
>> 
>> ```
>> TypeError:​ range.endContainer.splitText is not a function. (In 'range.endContainer.splitText(range.endOffset)​', 'range.endContainer.splitText' is undefined)​ (at SpreadsheetTextField.js:​522:​93)​
>>     _reAttachSuggestionHint @ SpreadsheetTextField.js:​522:​93
>>     suggestionHint @ SpreadsheetTextField.js:​83:​41
>>     completionSuggestionsSelectedCompletion @ SpreadsheetTextField.js:​146:​13
>>     selectNext @ CompletionSuggestionsView.js:​94:​67
>>     _handleKeyDownForSuggestionView @ SpreadsheetTextField.js:​307:​49
>>     _handleKeyDown @ SpreadsheetTextField.js:​220:​64
>>     _handleKeyDown @ [native code]​
>> ```
>> 
>> # STEPS TO REPRODUCE
>> 1. inspect any page
>> 2. add a new CSS property
>> 3. type `margin` in for the name
>> 4. type `auto auto` for the value
>> 5. delete the first `auto` (empty completion should be shown)
>> 6. type `i` (which will show `initial` as the completion)
>> 7. delete the `i`
>> 8. press the down key
> 
> Investigating this. I've found and fixed several other bugs living on this patch for the last week, but seem to have not run into this one yet 😅

Found the underlying issue and will also add an assert/return here to ensure that the range's endContainer is a text node. Underlying issue was that setting the suggestion hint to an empty string removed the suggestion hint element, but did not defragment the remaining text nodes which were created to be able to wedge the completion hint in the correct place. This meant every time the suggestion was cleared without us committing it, the contents of `_element` just got slice again next time the suggestion hint was needed.

Relatedly, I've found another issue (however this isn't a regression because it also occurs without this patch) where pressing the up or down arrow keys with a suggestion hint visible, but no suggestion list (because there is only one possible completion), leaves the suggestion hint, but the cursor is no longer where you would expect, resulting in some strange situations. This also reproduces for Control+A (and friends). I'm working on a solution that makes sure we discard completions when the caret moves but there was no text input or other reason to keep completions, without having to define every single possible keyboard chord that can move the caret.

I'll continue living on this through the weekend as well to see if I can find any other issues.

[Patrick Angle]

Created attachment 432235 [details]
Patch v1.3 - Address review feedback

[Patrick Angle]

Created attachment 432310 [details]
Patch v1.4 - Roll back changes to SpreadsheetTextField to handle in a future patch

[Devin Rousso]

Comment on attachment 432310 [details]
Patch v1.4 - Roll back changes to SpreadsheetTextField to handle in a future patch

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

r=me, nice.  I'd suggest living on this a bit before landing tho, as some of the logic inside `WI.CSSKeywordCompletions.forPartialPropertyValue` is maybe slightly scary and/or different behavior from what we had (though this is probably desirable).

NIT: It's a bit odd that `margin: env(` shows a completion hint for `margin: env(safe-area-inset-bottom;` WITHOUT a `)`.  Could we make it so that the `)` is added when doing completions for functions, or is that a super intense change (I have a feeling it is)?

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:40
> +    if (text.length !== caretPosition)

NIT: I'd flip this

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:48
> +    return {prefix: text, completions};

NIT: I'd restructure this to use early returns
```
    if (!text && allowEmptyPrefix)
        return {prefix: text, completions: WI.CSSCompletions.cssNameCompletions.values};
    return {prefix: text, completions: WI.CSSCompletions.cssNameCompletions.startsWith(text)};
```

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

So is `caretPosition` here mainly so that you can test the behavior of this, even if you can't actually use it in `WI.SpreadsheetStyleProperty`?  That seems mostly fine, but it is a bit odd that `WI.CSSKeywordCompletions.forPartialPropertyName` explicitly bails if `caretPosition !== text.length`. I'd expect both of these functions to either do or not do that, not one of each.

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

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:81
> +    // If the current token is only whitespace characters, treat it as if we are at the start of a new token.

I'd move this to be next to the comment below since that's where this logic actually is.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:83
> +    let currentTokenIsEmpty = !currentTokenValue.length;

I'd just inline this.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:84
> +    let caretIsMidToken = caretPosition !== passedCharacters;

NIT: `caretIsInMiddleOfToken`

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:93
> +    if (tokenAtCaret.type && /\b(comment|string)\b/.test(tokenAtCaret.type))
> +        return {prefix: "", completions: []};

Could you move this above `let currentTokenValue = ...` so that we avoid doing any of that work in this case?

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

It doesn't look like `allowEmptyPrefix` is used right now.

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:115
> +        expectedCompletions: ["papayawhip"],

PAPAYAWHIP 🔥🔥🔥

[Patrick Angle]

Comment on attachment 432310 [details]
Patch v1.4 - Roll back changes to SpreadsheetTextField to handle in a future patch

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

To the NIT regarding closing parenthesis: I agree it could be a bit odd, however this patch isn't really regressing behavior there. Because we don't provide mid-line completions yet do to other issues in the hopper, the closing parentheses situation currently would require us to append the paren to each completion value, which means it would appear in the list that way. Also would cause issues with some other function types like repeat() which take multiple params. The common case of pressing enter to get the completion will automatically add it anyways, since there is existing logic to add missing parens to the value on commit.

I'll get the rest of this fixed up and lived on for another day or two (this logic hasn't really changed in the last week as I worked through bugs related to the text fields).

So, if the user types `env(` arrows down the completion list to `safe-area-inset-top` and presses the return key, they will automatically get the the closing paren added. I hope we can figure out the bugs related to Bug 227411 and finish improving out completion situation soon.

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:51
>> +WI.CSSKeywordCompletions.forPartialPropertyValue = function(text, propertyName, {caretPosition} = {})
> 
> So is `caretPosition` here mainly so that you can test the behavior of this, even if you can't actually use it in `WI.SpreadsheetStyleProperty`?  That seems mostly fine, but it is a bit odd that `WI.CSSKeywordCompletions.forPartialPropertyName` explicitly bails if `caretPosition !== text.length`. I'd expect both of these functions to either do or not do that, not one of each.

Yes, currently `caretPosition` is tested but not used by `SpreadsheetStyleProperty`, as that is tied up in Bug 227411. The reason only the `*Value` takes a caret position is that we don't support completions in middle of tokens, which means for property names, the only valid place for the caret to be while getting completions is at the end of the text. The early bail for `caretPosition !== text.length` in `*Name` means that for the Bug 227411 patch (WIP patch based on the code removed from previous versions of this patch coming later today), providing the actual caret position will make sure we only provide completions at the end of the line, since a property name should only be a single token, or is otherwise invalid anyways

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:913
>> +    _valueCompletionDataProvider(text, {allowEmptyPrefix} = {})
> 
> It doesn't look like `allowEmptyPrefix` is used right now.

It isn't, but we will always be provided this value by SpreadsheetTextField:387, which provides allowEmptyPrefix to both this function and `_nameCompletionDataProvider` I call it out here much like we do unused `event` parameters to make clear what variables are being provided, even if we don't end up using it. I can remove the parameter, no problem. This just felt more in line with our style (unless our style is different for optional parameters, in which case this is as good as gone).

>> LayoutTests/inspector/unit-tests/css-keyword-completions.html:115
>> +        expectedCompletions: ["papayawhip"],
> 
> PAPAYAWHIP 🔥🔥🔥

🍦

[Patrick Angle]

Created attachment 432405 [details]
Video of Patch v1.4

[Patrick Angle]

Created attachment 432433 [details]
Patch v1.5 - Review notes, will live on for a bit longer to verify

[EWS]

Committed r279422 (239283@main): <https://commits.webkit.org/239283@main>

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