Bug 177313 - Web Inspector: Styles Redesign: hook up autocompletion to property names and values
Summary: Web Inspector: Styles Redesign: hook up autocompletion to property names and ...
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: InRadar
: 178501 (view as bug list)
Depends on: 177208
Blocks:
  Show dependency treegraph
 
Reported: 2017-09-21 11:32 PDT by Nikita Vasilyev
Modified: 2017-10-18 19:29 PDT (History)
5 users (show)

See Also:


Attachments
[Animated GIF] WIP (491.96 KB, image/gif)
2017-10-06 15:20 PDT, Nikita Vasilyev
no flags Details
WIP (8.88 KB, patch)
2017-10-06 15:23 PDT, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (20.50 KB, patch)
2017-10-11 12:21 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Animated GIF] With patch applied (841.93 KB, image/gif)
2017-10-11 12:22 PDT, Nikita Vasilyev
no flags Details
Patch (20.90 KB, patch)
2017-10-11 16:43 PDT, Nikita Vasilyev
joepeck: review-
Details | Formatted Diff | Diff
[IMAGE] Issue - Caret is off center (3px on left, 2px on right) (31.95 KB, image/png)
2017-10-12 11:04 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Issue - Property completion with number (49.57 KB, image/png)
2017-10-12 11:24 PDT, Joseph Pecoraro
no flags Details
[Image] Issue - Caret placement (29.07 KB, image/png)
2017-10-12 12:28 PDT, Nikita Vasilyev
no flags Details
Patch (21.25 KB, patch)
2017-10-12 15:15 PDT, Nikita Vasilyev
joepeck: review+
Details | Formatted Diff | Diff
Patch (21.26 KB, patch)
2017-10-13 09:33 PDT, 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 2017-09-21 11:32:51 PDT
- 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>
Comment 1 Radar WebKit Bug Importer 2017-09-21 11:33:10 PDT
<rdar://problem/34577057>
Comment 2 Nikita Vasilyev 2017-10-06 15:20:46 PDT
Created attachment 323054 [details]
[Animated GIF] WIP

Known issues:
- Suggest hint just selected text. It isn't gray.
Comment 3 Nikita Vasilyev 2017-10-06 15:23:34 PDT
Created attachment 323055 [details]
WIP

Feel free to apply the patch and play around with it. Not ready for review yet.
Comment 4 Nikita Vasilyev 2017-10-11 12:21:44 PDT
Created attachment 323444 [details]
Patch
Comment 5 Nikita Vasilyev 2017-10-11 12:22:41 PDT
Created attachment 323445 [details]
[Animated GIF] With patch applied
Comment 6 Joseph Pecoraro 2017-10-11 15:17:34 PDT
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.
Comment 7 Nikita Vasilyev 2017-10-11 16:35:15 PDT
(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.
Comment 8 Nikita Vasilyev 2017-10-11 16:43:14 PDT
Created attachment 323487 [details]
Patch
Comment 9 Joseph Pecoraro 2017-10-12 11:04:22 PDT
Created attachment 323537 [details]
[IMAGE] Issue - Caret is off center (3px on left, 2px on right)
Comment 10 Joseph Pecoraro 2017-10-12 11:07:10 PDT
(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.
Comment 11 Nikita Vasilyev 2017-10-12 11:17:36 PDT
(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.
Comment 12 Joseph Pecoraro 2017-10-12 11:24:15 PDT
Created attachment 323539 [details]
[IMAGE] Issue - Property completion with number
Comment 13 Joseph Pecoraro 2017-10-12 11:27:36 PDT
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}*
Comment 14 Nikita Vasilyev 2017-10-12 12:28:18 PDT
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.
Comment 15 Nikita Vasilyev 2017-10-12 13:07:45 PDT
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.
Comment 16 Nikita Vasilyev 2017-10-12 15:15:44 PDT
Created attachment 323577 [details]
Patch
Comment 17 Nikita Vasilyev 2017-10-12 15:21:04 PDT
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.
Comment 18 Nikita Vasilyev 2017-10-12 16:09:44 PDT
> (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
Comment 19 Joseph Pecoraro 2017-10-12 21:27:08 PDT
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.
Comment 20 Nikita Vasilyev 2017-10-13 09:33:17 PDT
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".
Comment 21 WebKit Commit Bot 2017-10-13 10:14:26 PDT
Comment on attachment 323683 [details]
Patch

Clearing flags on attachment: 323683

Committed r223283: <https://trac.webkit.org/changeset/223283>
Comment 22 WebKit Commit Bot 2017-10-13 10:14:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Nikita Vasilyev 2017-10-18 19:29:31 PDT
*** Bug 178501 has been marked as a duplicate of this bug. ***