Bug 177313 – Web Inspector: Styles Redesign: hook up autocompletion to property names and values

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

- Implement suggest hint

    col|or
        ^
        suggest hint


- Implement autocompletion popover

    col|
    color
    color-interpolation-filters
    color-interpolation
    color-profile
    ...

- Accept autocomplete suggestion by pressing Enter, Tab, or Arrow Right keys.

- Discard autocomplete suggestion by pressing Esc key, or by moving text caret left.

- Show autocomplete immediately when tabbing to an empty (e.g. new) value field.

<rdar://problem/33526579>

[Radar WebKit Bug Importer]

<rdar://problem/34577057>

[Nikita Vasilyev]

Created attachment 323054 [details]
[Animated GIF] WIP

Known issues:
- Suggest hint just selected text. It isn't gray.

[Nikita Vasilyev]

Created attachment 323055 [details]
WIP

Feel free to apply the patch and play around with it. Not ready for review yet.

[Nikita Vasilyev]

Created attachment 323444 [details]
Patch

[Nikita Vasilyev]

Created attachment 323445 [details]
[Animated GIF] With patch applied

[Joseph Pecoraro]

Comment on attachment 323444 [details]
Patch

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

Feels good some far. Here are my general comments on behavior after playing with it:

  1. When creating a new property and tabbing the value, there is no blinking caret, but there are autocompletion suggestions. Felt weird.

  2. It suddenly becomes hard to delete a property. For example, if there is a property "color: blue"
      - Click "blue" => Delete => Enter => Ends up with "color: aliceblue" and editing the next thing
      - Click "blue" => Delete => Tab => Ends up with "color: aliceblue" and editing the next thing
  The best ways I've found are:
      - Click "blue" => Delete => Escape => Enter. Which is quite involved.
      - Click "color" => Delete => Enter. I didn't immediately think to select the property, but maybe this is fine.

   3. For multi-value properties Tab / Enter unexpectedly moved to the next property
      - Accepting a suggestion with Tab/Enter like "solid" in: "border: 1px sol|" leaves me with "1px solid" and moved me to the next property.
      - I'd likely want to stay in this value so that I could input a color.
   The best way I've found to get what I want:
      - Right Arrow instead of Tab / Enter. But I wouldn't think to use right arrow as its not easily reachable for me on the keyboard
   I guess this comes down to:
      - I expect Tab to behave more like Right arrow when there are completions.

  4. When keying down through the Completion Suggestions quickly only the last is applied
      - Create a new property "color" and tab to the value
      - Keyboard down through the list quickly => Only when you stop or go slowly is the value applied

  5. When adding a new property I can't trigger a list of ALL suggestions.
     - It used to be that Esc would always "complete right here". Is there an equivalent to that?

> Source/WebInspectorUI/ChangeLog:12
> +        - Arrow Up selects the next completion item.
> +        - Arrow Down selects the previous completion item.

This is flipped. Down selects the next and Up the previous.

> Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:28
> +    constructor(delegate, options)

For options we've been using this style:

    constructor(delegate, {preventBlur} = {})

So inside the method you can just use `preventBlur` and not need to value check options.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:101
> +    completionSuggestionsSelectedCompletion(suggestionsView, selectedText = "")

Style: I've recently been add a section comment here like:

    // CompletionSuggestionsView delegate

    completionSuggestionsSelectedCompletion(suggestionsView, selectedText = "")
    ...

It may help us with code searching in the future, and differentiates this from the Public / Protected section above.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:109
> +        if (this._suggestionHintElement.parentElement !== this._element)
> +            this._element.append(this._suggestionHintElement);

You might be able to simplify this by just checking if there is a parent. You never expect it to be parented anywhere else:

    if (!this._suggestionHintElement.parentNode)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:238
> +            return;

Should this return true?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:277
> +

There is an expectation of a return value. Seem this should return false at the end.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:357
> +    _getCompletionPrefix(prefix)
> +    {
> +        // For "border: 1px so|", we want to suggest "solid" based on "so" prefix.
> +        let match = prefix.match(/\b[a-z()-]+$/i);
> +        if (match)
> +            return match[0];
> +
> +        return prefix;
> +    }

Hmm, so this will be completing always at the end of the field. It should probably care about where the caret is. For example if my cursor is:

    border: [1px so| black]

Then I'd like to get a `solid` completion. Right now it doesn't complete. But this could be a follow-up. This is important, but not as important as just getting autocompletion up and running at all.

[Nikita Vasilyev]

(In reply to Joseph Pecoraro from comment #6)
> Comment on attachment 323444 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323444&action=review
> 
> Feels good some far. Here are my general comments on behavior after playing
> with it:
> 
>   1. When creating a new property and tabbing the value, there is no
> blinking caret, but there are autocompletion suggestions. Felt weird.

Fixed.

> 
>   2. It suddenly becomes hard to delete a property. For example, if there is
> a property "color: blue"
>       - Click "blue" => Delete => Enter => Ends up with "color: aliceblue"
> and editing the next thing
>       - Click "blue" => Delete => Tab => Ends up with "color: aliceblue" and
> editing the next thing
>   The best ways I've found are:
>       - Click "blue" => Delete => Escape => Enter. Which is quite involved.
>       - Click "color" => Delete => Enter. I didn't immediately think to
> select the property, but maybe this is fine.

Fixed.

Click "blue" => Delete => Tab or Enter => Removes the property

> 
>    3. For multi-value properties Tab / Enter unexpectedly moved to the next
> property
>       - Accepting a suggestion with Tab/Enter like "solid" in: "border: 1px
> sol|" leaves me with "1px solid" and moved me to the next property.
>       - I'd likely want to stay in this value so that I could input a color.
>    The best way I've found to get what I want:
>       - Right Arrow instead of Tab / Enter. But I wouldn't think to use
> right arrow as its not easily reachable for me on the keyboard
>    I guess this comes down to:
>       - I expect Tab to behave more like Right arrow when there are
> completions.

For this patch, I'd like to keep the behavior that matches Chrome DevTools.
I want to collect feedback and experiment with different actions for Tab / Enter
later.

> 
>   4. When keying down through the Completion Suggestions quickly only the
> last is applied
>       - Create a new property "color" and tab to the value
>       - Keyboard down through the list quickly => Only when you stop or go
> slowly is the value applied

Will fix as a follow up.

> 
>   5. When adding a new property I can't trigger a list of ALL suggestions.
>      - It used to be that Esc would always "complete right here". Is there
> an equivalent to that?
> 

- Tabbing from name to an empty value shows all suggestions.
- Deleting value entirely shows all suggestion.

There's currently no keyboard shortcut to force showing all suggestions, but
I think this is fine.


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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:238
>> +            return;
> 
> Should this return true?

Yes, it should.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:357
>> +    }
> 
> Hmm, so this will be completing always at the end of the field. It should probably care about where the caret is. For example if my cursor is:
> 
>     border: [1px so| black]
> 
> Then I'd like to get a `solid` completion. Right now it doesn't complete. But this could be a follow-up. This is important, but not as important as just getting autocompletion up and running at all.

Yes, I'd like to fix this as a follow up.

[Nikita Vasilyev]

Created attachment 323487 [details]
Patch

[Joseph Pecoraro]

Created attachment 323537 [details]
[IMAGE] Issue - Caret is off center (3px on left, 2px on right)

[Joseph Pecoraro]

(In reply to Nikita Vasilyev from comment #7)
> There's currently no keyboard shortcut to force showing all suggestions, but
> I think this is fine.

As a user, I really want this. Just playing with the current patch its easy to get in a place where I get no completion suggestions but I want to see what is available.

    border: 1px solid |

I want to see what colors / keywords and I can't trigger that unless I start with a character.

[Nikita Vasilyev]

(In reply to Joseph Pecoraro from comment #10)
> (In reply to Nikita Vasilyev from comment #7)
> > There's currently no keyboard shortcut to force showing all suggestions, but
> > I think this is fine.
> 
> As a user, I really want this. Just playing with the current patch its easy
> to get in a place where I get no completion suggestions but I want to see
> what is available.
> 
>     border: 1px solid |
> 
> I want to see what colors / keywords and I can't trigger that unless I start
> with a character.

I can make the Escape key do that, if you think this's more important than discarding changes.

[Joseph Pecoraro]

Created attachment 323539 [details]
[IMAGE] Issue - Property completion with number

[Joseph Pecoraro]

Comment on attachment 323487 [details]
Patch

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

r- for the few smaller issues identified in screenshots and comments. Address them and this should be good to land and addressing other issues as follow-ups.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:49
> +    margin-right: 3px;

Based on the issue screenshot I attached maybe this should be 4px on the right? It was unbalanced with the left.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:154
> +    set _suggestionHint(value)

I really don't like getters/setters with leading underscores. I think its better to just make it a public.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:228
> +    _handleKeyDownForSuggestionView(event)

This appears to break ⇧⌥➞ or selecting a word to the right since ⇧⌥← still works.

Probably early on in _handleKeyDownForSuggestionView, or per-case, you may want to bail if either event.shiftKey or event.altKey are active.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:359
> +        let match = prefix.match(/[a-z()-]+$/i);

0-9 are allowed in property names:

    <style>
    :root { --color1--something: blue; }
    body { background: var(--color1--something); }
    </style>
    <body></body>

Numbers just can't be the first character but we don't have to be precise here. It seems there are visible issues right now with numbers in property names, should be easy to fix by allowing numbers here.

Technically a lot more is allowed (for unicode variable names):

    nonascii	        [\240-\377]
    unicode		\\{h}{1,6}(\r\n|[ \t\r\n\f])?
    escape		{unicode}|\\[^\r\n\f0-9a-f]
    nmstart		[_a-z]|{nonascii}|{escape}
    nmchar		[_a-z0-9-]|{nonascii}|{escape}
    ident		-?{nmstart}{nmchar}*

[Nikita Vasilyev]

Created attachment 323547 [details]
[Image] Issue - Caret placement

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:49
>> +    margin-right: 3px;
> 
> Based on the issue screenshot I attached maybe this should be 4px on the right? It was unbalanced with the left.

Changing margin-right didn't fix it.

I get different text caret placement depending on a property name (or its character count, rather). For "color" and "font" it looks different. I don't understand why it matters as all properties are monospace.

[Nikita Vasilyev]

Comment on attachment 323487 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:228
>> +    _handleKeyDownForSuggestionView(event)
> 
> This appears to break ⇧⌥➞ or selecting a word to the right since ⇧⌥← still works.
> 
> Probably early on in _handleKeyDownForSuggestionView, or per-case, you may want to bail if either event.shiftKey or event.altKey are active.

This isn't _handleKeyDownForSuggestionView fault. Commenting it out entirely didn't solve the problem.

This looks like a contentEditable issue. Once suggestHint is shown, ⇧⌥➞ stops working.

[Nikita Vasilyev]

Created attachment 323577 [details]
Patch

[Nikita Vasilyev]

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

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:228
>>> +    _handleKeyDownForSuggestionView(event)
>> 
>> This appears to break ⇧⌥➞ or selecting a word to the right since ⇧⌥← still works.
>> 
>> Probably early on in _handleKeyDownForSuggestionView, or per-case, you may want to bail if either event.shiftKey or event.altKey are active.
> 
> This isn't _handleKeyDownForSuggestionView fault. Commenting it out entirely didn't solve the problem.
> 
> This looks like a contentEditable issue. Once suggestHint is shown, ⇧⌥➞ stops working.

I resolved this by removing suggestionHintElement when it's empty.


(In reply to Nikita Vasilyev from comment #14)
> Created attachment 323547 [details]
> [Image] Issue - Caret placement
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323487&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:49
> >> +    margin-right: 3px;
> > 
> > Based on the issue screenshot I attached maybe this should be 4px on the right? It was unbalanced with the left.
> 
> Changing margin-right didn't fix it.
> 
> I get different text caret placement depending on a property name (or its
> character count, rather). For "color" and "font" it looks different. I don't
> understand why it matters as all properties are monospace.

After talking to Myles, this is likely to be a bug in WebKit. I don't have a workaround right now.

[Nikita Vasilyev]

> (In reply to Nikita Vasilyev from comment #14)
> > Created attachment 323547 [details]
> > [Image] Issue - Caret placement
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=323487&action=review
> > 
> > >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:49
> > >> +    margin-right: 3px;
> > > 
> > > Based on the issue screenshot I attached maybe this should be 4px on the right? It was unbalanced with the left.
> > 
> > Changing margin-right didn't fix it.
> > 
> > I get different text caret placement depending on a property name (or its
> > character count, rather). For "color" and "font" it looks different. I don't
> > understand why it matters as all properties are monospace.
> 
> After talking to Myles, this is likely to be a bug in WebKit. I don't have a
> workaround right now.

I filed a bug:
https://bugs.webkit.org/show_bug.cgi?id=178242
Bug 178242 - Inconsistent position of text caret when focusing on an empty element

[Joseph Pecoraro]

Comment on attachment 323577 [details]
Patch

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

r=me

> Source/WebInspectorUI/ChangeLog:19
> +        Add a preventBlur option so clicking on an completion item doesn't change the focus and doesn't cause "blur" event on the target text field.

Nit: We typically wrap ChangeLog sentences at some reasonable column (80-100). Nothing is enforced, we just avoid super long lines.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:72
> +                this._element.append(this._suggestionHintElement);

Nit: I'd still prefer appendChild if already have the element so we know thats what it would do.

[Nikita Vasilyev]

Created attachment 323683 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:19
>> +        Add a preventBlur option so clicking on an completion item doesn't change the focus and doesn't cause "blur" event on the target text field.
> 
> Nit: We typically wrap ChangeLog sentences at some reasonable column (80-100). Nothing is enforced, we just avoid super long lines.

Whoops, fixed.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:72
>> +                this._element.append(this._suggestionHintElement);
> 
> Nit: I'd still prefer appendChild if already have the element so we know thats what it would do.

I like brevity of "append".

[WebKit Commit Bot]

Comment on attachment 323683 [details]
Patch

Clearing flags on attachment: 323683

Committed r223283: <https://trac.webkit.org/changeset/223283>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikita Vasilyev]

*** Bug 178501 has been marked as a duplicate of this bug. ***