Bug 237676 – Web Inspector: Styles panel: Unwanted extra dash when autocompleting CSS variable names

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

[Razvan Caliman]

Created attachment 454288 [details]
Video recording of bug

# Steps to Reproduce:

- Ensure "Use fuzzy matching for completion suggestions" is enabled in Settings > Experimental
- Inspect any element on https://webkit.org 
- In the Styles panel, add a new CSS declaration: `color: var(-color` (notice the missing second dash)
- Pick any suggestion offered for a CSS variable name, ex `--link-color`

# Result

The applied CSS variable name includes an extra dash, ex: `var(---link-color)`

[Razvan Caliman]

Created attachment 454290 [details]
Patch 1.0

[Patrick Angle]

Comment on attachment 454290 [details]
Patch 1.0

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

env

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:124
> +    // Expand the current token value to mitigate a bug in the tokenizer for CSS variable names:
> +    // 1) in `var(-name)`: splitting `-name` into two tokens, `-` and `name`.
> +    // 2) in `var(--)`: splitting `--` into two tokens, `-` and `-`.

Can we reference a bug/ticket in CodeMirror and rephrase to present the bug info first e.g. "Due to <BUG>, <explanation of what the bug is causing>"?

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:128
> +            if (value === "(" || value === " ")

Is it correct to do this for spaces? What if the current value is something like `var(- -|` or some other partial value that has a space in it?

Although I think that there is also a pre-existing bug in handling functions. For example if I have `env(foo safe|` I get completions for `safe-area-inset-*` values, but they won't be valid when I commit any of them. I'm not sure of the best way to handle that, given something like `calc` will often have space-separated (or for authors that don't use space in math expressions, a bunch of operator-separated) values. This is a separate bug, but at least in this case let's avoid perpetuating that bug into this workaround.

The more I think about this, should we apply this same logic to `env` as well, since now with fuzzy matching multiple possible values when typing `--` into `env` providing results as well, and mid-text spaces between the caret and the opening paren would indicate an invalid value where a completion won't work anyways?

> LayoutTests/ChangeLog:11
> +        * inspector/unit-tests/css-keyword-completions-expected.txt:
> +        * inspector/unit-tests/css-keyword-completions.html:

Nice!

[Radar WebKit Bug Importer]

<rdar://problem/90090000>

[Razvan Caliman]

Comment on attachment 454290 [details]
Patch 1.0

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

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:124
>> +    // 2) in `var(--)`: splitting `--` into two tokens, `-` and `-`.
> 
> Can we reference a bug/ticket in CodeMirror and rephrase to present the bug info first e.g. "Due to <BUG>, <explanation of what the bug is causing>"?

After further investigation, this isn't a bug per se in CodeMirror. It does its job well and does not tokenize a non-conforming CSS variable name.
With fuzzy matching, we expect someone to type incomplete strings and still match. So it's our special case, not CodeMirror's bug.

I have updated the comments and Changelog.

Aside, there'a an extra special case that's fixed upstream in CodeMirror: correctly tokenizing *just* `--` as a valid variable name. Yes, that's legal according to spec o_O:
https://github.com/codemirror/CodeMirror/pull/5687

I pulled in just this fix, but not CodeMirror tip-of-tree or even the updated rules for the CSS mode to avoid introducing side-effects.
If and when we do update CodeMirror, there's no risk of losing our local overwrite. It will come from upstream.

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:128
>> +            if (value === "(" || value === " ")
> 
> Is it correct to do this for spaces? What if the current value is something like `var(- -|` or some other partial value that has a space in it?
> 
> Although I think that there is also a pre-existing bug in handling functions. For example if I have `env(foo safe|` I get completions for `safe-area-inset-*` values, but they won't be valid when I commit any of them. I'm not sure of the best way to handle that, given something like `calc` will often have space-separated (or for authors that don't use space in math expressions, a bunch of operator-separated) values. This is a separate bug, but at least in this case let's avoid perpetuating that bug into this workaround.
> 
> The more I think about this, should we apply this same logic to `env` as well, since now with fuzzy matching multiple possible values when typing `--` into `env` providing results as well, and mid-text spaces between the caret and the opening paren would indicate an invalid value where a completion won't work anyways?

I'd keep the space as a separator for the current token. It's true that any suggestion accepted would yield an invalid CSS declaration by concatenation with the whitespace-separate string. 
But doing so we avoid "eating" more preceding tokens than necessary in edge cases we don't foresee yet. If a user gets into a scenario of `var(foo --name|`, the resulting value would be invalid regardless if typed or autocompleted. I think that's more predictable and recoverable for a user.

> The more I think about this, should we apply this same logic to `env` as well 
Is something like `env(--name)` even valid?

[Razvan Caliman]

Created attachment 454867 [details]
Patch 1.1

Update comments, pull in fix from CodeMirror upstream for tokenizing double-dash as valid variable name

[Razvan Caliman]

Comment on attachment 454867 [details]
Patch 1.1

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

> Source/WebInspectorUI/UserInterface/External/CodeMirror/css.js:65
> +      } else if (stream.match(/^-[\w\\\-]*/)) {

This comes from CodeMirror upstream: https://github.com/codemirror/CodeMirror/pull/5687/commits/4d0164220e21131af5f4598467ee61f93e52c5eb

[Patrick Angle]

Comment on attachment 454867 [details]
Patch 1.1

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

>> Source/WebInspectorUI/UserInterface/External/CodeMirror/css.js:65
>> +      } else if (stream.match(/^-[\w\\\-]*/)) {
> 
> This comes from CodeMirror upstream: https://github.com/codemirror/CodeMirror/pull/5687/commits/4d0164220e21131af5f4598467ee61f93e52c5eb

In general I think we'd like to update CodeMirror to upstream versions, not patch in specific bits from upstream. Is there anything blocking us from adopting a more recent version of CodeMirror that includes this fix?

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:126
> +    if (functionName === "var" && currentTokenValue.length) {

I thought I had commented this in reply to your question on the previous patch, but I can find no proof I did:
We should include `env` here has well, since `env(--` should fuzzily match the `--` to `safe-area-inset-*`. It's perhaps an edge case, but I think the solution is the same, so we should include it here.

[Razvan Caliman]

Created attachment 455403 [details]
Video of pre-existing bug with vendor prefixed CSS values

It turns out this CodeMirror tokenizer limitation was already affecting autocompletion before turning on fuzzy matching: it was separating `-` from `webkit` when typing a value and thus searching just for the prefix `webkit`. This returned no results and the suggestions list was empty.

See attached video for demo.

It only occurs when autocompleting CSS values because that's where the tokenizer is used.
It isn't an issue with CSS property names because in that case the tokenizer isn't used; the whole string is passed in as a prefix, no dropping `-`.

I have a solution coming up which addressed this bug too.

[Razvan Caliman]

Created attachment 455405 [details]
Patch 1.2

Simplified solution to only avoid splitting first dash.
Removed manual changes to CodeMirror CSS mode

[Razvan Caliman]

Comment on attachment 454867 [details]
Patch 1.1

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

>>> Source/WebInspectorUI/UserInterface/External/CodeMirror/css.js:65
>>> +      } else if (stream.match(/^-[\w\\\-]*/)) {
>> 
>> This comes from CodeMirror upstream: https://github.com/codemirror/CodeMirror/pull/5687/commits/4d0164220e21131af5f4598467ee61f93e52c5eb
> 
> In general I think we'd like to update CodeMirror to upstream versions, not patch in specific bits from upstream. Is there anything blocking us from adopting a more recent version of CodeMirror that includes this fix?

I pulled this out because it's not needed.
Updating CodeMirror is out of scope for this patch.

Aside: CodeMirror is vendored-in to Web Inspector. It looks like only a few bits of it were pulled in, not the entire project. So an update isn't straightforward.
A look at the latest CodeMirror repo structure (https://github.com/codemirror/CodeMirror) shows a very different one from the flat structure we currently have in Web Inspector (https://sourcegraph.com/github.com/WebKit/WebKit@aa4287c6b8d6c6f90ba3c416e219c0c533d59727/-/tree/Source/WebInspectorUI/UserInterface/External/CodeMirror)

I'm not even sure which version of CodeMirror we're currently using.
Updating looks more complex than simply a drop-in replacement and will require substantial manual testing. Maybe some other time :)

>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:126
>> +    if (functionName === "var" && currentTokenValue.length) {
> 
> I thought I had commented this in reply to your question on the previous patch, but I can find no proof I did:
> We should include `env` here has well, since `env(--` should fuzzily match the `--` to `safe-area-inset-*`. It's perhaps an edge case, but I think the solution is the same, so we should include it here.

I turns out the issue with the tokenizer had effects elsewhere too: when completing vendor-prefixed CSS values. Explained in https://bugs.webkit.org/show_bug.cgi?id=237676#c8

I added a precise solution which addresses the issue punctually: do not drop the immediately preceding dash if one exists. That's all.
It addresses all of these use cases and more:
- `var(--|`
- `var(-name|`
- `env(--|`
- `env(-safe|
- `-webkit|

CodeMirror's tokenizer works well after encountering the second dash without any changes necessary. It keeps the token in one piece. For example: `env(---` //=> currentTokenValue === `---`.

I didn't guard for `calc()` because a minus/dash without whitespace around it isn't valid CSS. So `calc(1px-something|` isn't going to return any completions and it wouldn't be valid if it did anyway.

```
CSS.supports('margin: calc(1px - 1px)') // => true
CSS.supports('margin: calc(1px-1px)')   // => false
```

I added a few more tests to cover the additional use cases.

[Patrick Angle]

Comment on attachment 455405 [details]
Patch 1.2

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

r=me - Nice work!

> LayoutTests/inspector/unit-tests/css-keyword-completions.html:271
>      // `calc(1 + var(|))`

Just for completeness can we add a test case along the lines of `calc(1 - |)` to ensure that we get an empty prefix here? We should currently, but I could see a mistake being made in a future change to `WI.CSSKeywordCompletions.forPartialPropertyValue`, for example if someone removes the `trim` on :86, which would make the token that is just space appear to have content and get merged with the preceding `-`.

[Razvan Caliman]

Created attachment 455418 [details]
[fast-cq] Patch 1.2

[Razvan Caliman]

Comment on attachment 455405 [details]
Patch 1.2

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

>> LayoutTests/inspector/unit-tests/css-keyword-completions.html:271
>>      // `calc(1 + var(|))`
> 
> Just for completeness can we add a test case along the lines of `calc(1 - |)` to ensure that we get an empty prefix here? We should currently, but I could see a mistake being made in a future change to `WI.CSSKeywordCompletions.forPartialPropertyValue`, for example if someone removes the `trim` on :86, which would make the token that is just space appear to have content and get merged with the preceding `-`.

Done!
Thanks for the careful review!

[EWS]

Committed r291740 (248772@main): <https://commits.webkit.org/248772@main>

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