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)`
Created attachment 454290 [details] Patch 1.0
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!
<rdar://problem/90090000>
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?
Created attachment 454867 [details] Patch 1.1 Update comments, pull in fix from CodeMirror upstream for tokenizing double-dash as valid variable name
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
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.
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.
Created attachment 455405 [details] Patch 1.2 Simplified solution to only avoid splitting first dash. Removed manual changes to CodeMirror CSS mode
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.
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 `-`.
Created attachment 455418 [details] [fast-cq] Patch 1.2
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!
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].