Bug 194724 - Web Inspector: Move CSS completion logic from SpreadsheetTextField to SpreadsheetStyleProperty
Summary: Web Inspector: Move CSS completion logic from SpreadsheetTextField to Spreads...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: DoNotImportToRadar
Depends on:
Blocks:
 
Reported: 2019-02-15 15:13 PST by Nikita Vasilyev
Modified: 2019-02-17 16:08 PST (History)
4 users (show)

See Also:


Attachments
Patch (6.25 KB, patch)
2019-02-15 15:21 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (6.73 KB, patch)
2019-02-16 18:56 PST, Nikita Vasilyev
hi: review+
Details | Formatted Diff | Diff
Patch for landing (6.76 KB, patch)
2019-02-17 15:42 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2019-02-15 15:13:02 PST
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.
Comment 1 Nikita Vasilyev 2019-02-15 15:21:31 PST
Created attachment 362166 [details]
Patch
Comment 2 Devin Rousso 2019-02-16 18:02:15 PST
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).
Comment 3 Nikita Vasilyev 2019-02-16 18:34:27 PST
(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.
Comment 4 Nikita Vasilyev 2019-02-16 18:56:04 PST
Created attachment 362224 [details]
Patch
Comment 5 Devin Rousso 2019-02-17 11:39:10 PST
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),
    };
Comment 6 Nikita Vasilyev 2019-02-17 15:42:21 PST
Created attachment 362259 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2019-02-17 16:08:58 PST
Comment on attachment 362259 [details]
Patch for landing

Clearing flags on attachment: 362259

Committed r241653: <https://trac.webkit.org/changeset/241653>
Comment 8 WebKit Commit Bot 2019-02-17 16:08:59 PST
All reviewed patches have been landed.  Closing bug.