This CSS value completion logic shouldn't be in SpreadsheetTextField: _getCompletionPrefix(prefix) { // For "border: 1px so|", we want to suggest "solid" based on "so" prefix. let match = prefix.match(/[a-z0-9()-]+$/i); 1. It doesn't make sense for CSS property names. 2. I plan to use SpreadsheetTextField for CSS selectors as well, e.g. remove SpreadsheetSelectorField and use SpreadsheetTextField instead.
Created attachment 362166 [details] Patch
Comment on attachment 362166 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362166&action=review Why is this patch necessary? Is it purely a design decision (e.g. we should keep CSS related "things" inside CSS classes), or is something going to build on this? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:715 > + completionPrefix: prefix, NIT: I think just `prefix` is enough. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:756 > + return {completions: [], completionPrefix: ""}; Ditto (>715). > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:758 > + let completionPrefix = match[0]; Ditto (>715). > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:761 > + return {completionPrefix, completions}; NIT: you can inline the values. let prefix = match[0]; return { prefix, completions: WI.CSSKeywordCompletions.forProperty(propertyName).startsWith(prefix), }; > Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:125 > + this._completionPrefix = ""; I don't think we should set this here. We should only clear this when we update completions, as technically `hide` doesn't clear the completions from the `_suggestionsView`. We should only do this when `_suggestionsView.update` is called. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:369 > + let {completions, completionPrefix} = this._completionProvider(prefix); > + this._completionPrefix = completionPrefix; Ditto (>SpreadsheetStyleProperty.js:715).
(In reply to Devin Rousso from comment #2) > Comment on attachment 362166 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362166&action=review > > Why is this patch necessary? Is it purely a design decision (e.g. we should > keep CSS related "things" inside CSS classes), or is something going to > build on this? It's mostly what I mentioned in the description: "2. I plan to use SpreadsheetTextField for CSS selectors as well, e.g. remove SpreadsheetSelectorField and use SpreadsheetTextField instead." Unifying SpreadsheetTextField and SpreadsheetSelectorField would resolve a few inconsistencies between the two. For example, CSS selector fields start editing on mousedown but CSS name/value fields on mouse up.
Created attachment 362224 [details] Patch
Comment on attachment 362224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362224&action=review r=me, nice work :) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:714 > + return {prefix, completions: WI.CSSCompletions.cssNameCompletions.startsWith(prefix)}; Style: when there are non-simple keys/values, please put all keys/values on separate lines. return { prefix, completions: WI.CSSCompletions.cssNameCompletions.startsWith(prefix), };
Created attachment 362259 [details] Patch for landing
Comment on attachment 362259 [details] Patch for landing Clearing flags on attachment: 362259 Committed r241653: <https://trac.webkit.org/changeset/241653>
All reviewed patches have been landed. Closing bug.