Bug 194724 – Web Inspector: Move CSS completion logic from SpreadsheetTextField to SpreadsheetStyleProperty

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

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 362166 [details]
Patch

[Devin Rousso]

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).

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

Created attachment 362224 [details]
Patch

[Devin Rousso]

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),
    };

[Nikita Vasilyev]

Created attachment 362259 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 362259 [details]
Patch for landing

Clearing flags on attachment: 362259

Committed r241653: <https://trac.webkit.org/changeset/241653>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.