Bug 194796 – Web Inspector: Styles: Control-Space should force completion

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

[Nikita Vasilyev]

Pressing Control-Space should show completion popover, even if the value is empty. This is how it works in Xcode, Chrome DevTools, and just about every code editor.

[Radar WebKit Bug Importer]

<rdar://problem/48180822>

[Nikita Vasilyev]

Created attachment 362349 [details]
Patch

[Nikita Vasilyev]

Created attachment 362350 [details]
[Video] With patch applied

[Matt Baker]

One thing I find really annoying is pressing Escape in the TextEditor shows the completion popover, for a file that is probably read-only, when it would be so much better to focus the Quick Console instead.

If control-space is a common shortcut for showing autocompletion, can we free up escape for this?

[Nikita Vasilyev]

(In reply to Matt Baker from comment #4)
> One thing I find really annoying is pressing Escape in the TextEditor shows
> the completion popover, for a file that is probably read-only, when it would
> be so much better to focus the Quick Console instead.

We shouldn't autocomplete for read-only resources, period. 

> 
> If control-space is a common shortcut for showing autocompletion, can we
> free up escape for this?

I think it makes sense. I'd prefer to do it as a separate patch since it's unrelated to the styles sidebar.

[Matt Baker]

(In reply to Matt Baker from comment #4)
> One thing I find really annoying is pressing Escape in the TextEditor shows
> the completion popover, for a file that is probably read-only, when it would
> be so much better to focus the Quick Console instead.

Actually it's only an issue for local files. Also I couldn't ever get the focus to jump to the Quick Console, so maybe that was never a thing.

> If control-space is a common shortcut for showing autocompletion, can we
> free up escape for this?

I think the case we actually want to consider is autocomplete in the Quick Console. Escape is the shortcut for both show/hide console drawer, and show/dismiss autocomplete. I think we should only use control-space to show autocomplete.

What do you think?

[Matt Baker]

(In reply to Nikita Vasilyev from comment #5)
> (In reply to Matt Baker from comment #4)
> > One thing I find really annoying is pressing Escape in the TextEditor shows
> > the completion popover, for a file that is probably read-only, when it would
> > be so much better to focus the Quick Console instead.
> 
> We shouldn't autocomplete for read-only resources, period. 
> 
> > 
> > If control-space is a common shortcut for showing autocompletion, can we
> > free up escape for this?
> 
> I think it makes sense. I'd prefer to do it as a separate patch since it's
> unrelated to the styles sidebar.

Makes sense!

[Matt Baker]

Comment on attachment 362349 [details]
Patch

r- for now, because completions for property values are currently broken, making this difficult to review.

The completions popover is no longer shown when tabbing to a new property value from a property name. This regressed in https://webkit.org/b/193605. I think it makes sense to fix the behavior as part of this patch, since they are so closely related.

[Matt Baker]

Comment on attachment 362349 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:372
> +    _updateCompletions(forceCompletions = false)

It would be great if we could avoid passing this flag across so many functions.

[Nikita Vasilyev]

Created attachment 362761 [details]
Patch

[Nikita Vasilyev]

(In reply to Matt Baker from comment #8)
> Comment on attachment 362349 [details]
> Patch
> 
> r- for now, because completions for property values are currently broken,
> making this difficult to review.
> 
> The completions popover is no longer shown when tabbing to a new property
> value from a property name. This regressed in https://webkit.org/b/193605. I
> think it makes sense to fix the behavior as part of this patch, since they
> are so closely related.

Fixed!

[Nikita Vasilyev]

(In reply to Matt Baker from comment #9)
> Comment on attachment 362349 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362349&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:372
> > +    _updateCompletions(forceCompletions = false)
> 
> It would be great if we could avoid passing this flag across so many
> functions.

I don't think I can avoid it.

We don't show completion popover for empty CSS property names (e.g. when creating a new property). I think it makes sense because at the top of the list are obscure -apple- and -epub- properties.

I'm fine with showing completion popover for empty property names after this bug lands: Bug 156271 - Web Inspector: CSS autocomplete: suggestion hint should be the most commonly used property and not the alphabetically first one.

[Devin Rousso]

Comment on attachment 362761 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:268
> +        if (prefix === "")

NIT: `!prefix`.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:712
> +    _nameCompletionDataProvider(prefix, forceCompletion = false)

I'd prefer you put this inside an optional `options` object and rename it to a positive.

    _nameCompletionDataProvider(prefix, options = {})
        let completions = null;
        if (!prefix && options.allowEmptyPrefix)
            completions = WI.CSSCompletions.cssNameCompletions.values;
        else
            completions = WI.CSSCompletions.cssNameCompletions.startsWith(prefix);

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:716
> +        if (!prefix && forceCompletion)
> +            completions = WI.CSSCompletions.cssNameCompletions.values;

If `forceCompletion` is `false`, both of these paths will have the same effect.  If we show all values when `prefix` is empty, all we really need to do is force the completion popover to appear when ⌃-Space is pressed.  `forceCompletion` is unnecessary.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:719
> +

NIT: unnecessary newline.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-750
> -            completions: WI.CSSKeywordCompletions.forProperty(propertyName).startsWith(prefix)

Why was this changed?  Having it inline is fine.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:281
> +        if (event.ctrlKey && event.code === "Space") {

We should decide on <https://webkit.org/b/192947> before doing this.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:378
> +        let {completions, prefix} = this._completionProvider(valueWithoutSuggestion, forceCompletions);

You also need to update `WI.SpreadsheetStyleProperty.prototype._valueCompletionDataProvider` for this.

[Nikita Vasilyev]

Comment on attachment 362761 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:716
>> +            completions = WI.CSSCompletions.cssNameCompletions.values;
> 
> If `forceCompletion` is `false`, both of these paths will have the same effect.  If we show all values when `prefix` is empty, all we really need to do is force the completion popover to appear when ⌃-Space is pressed.  `forceCompletion` is unnecessary.

"If `forceCompletion` is `false`, both of these paths will have the same effect." — no, they won't.
We don't show completion popover for empty CSS property names (e.g. when creating a new property).

[Devin Rousso]

Comment on attachment 362761 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:716
>>> +            completions = WI.CSSCompletions.cssNameCompletions.values;
>> 
>> If `forceCompletion` is `false`, both of these paths will have the same effect.  If we show all values when `prefix` is empty, all we really need to do is force the completion popover to appear when ⌃-Space is pressed.  `forceCompletion` is unnecessary.
> 
> "If `forceCompletion` is `false`, both of these paths will have the same effect." — no, they won't.
> We don't show completion popover for empty CSS property names (e.g. when creating a new property).

Ah, I missed the part where `_acceptEmptyPrefix` gets set.

Why is it that we don't want to show a completion for an empty property name?  Is it just because there are a lot of CSS properties?  If we're adding a keyboard shortcut to force completions, but only for values, that seems a bit strict.

[Nikita Vasilyev]

Because this is what it shows for empty property names:

    -apple-color-filter
    -apple-pay-button-style
    -apple-pay-button-type
    -apple-trailing-word
    -epub-caption-side
    -epub-hyphens
    -epub-text-combine
    -epub-text-emphasis
    -epub-text-emphasis-color
    -epub-text-emphasis-style
    -epub-text-orientation

I'm to show completions for empty property names once we land your patch to show the most commonly used properties first.

[Nikita Vasilyev]

Created attachment 363042 [details]
Patch

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #16)
> I'm to show completions for empty property names once we land your patch to
> show the most commonly used properties first.

*I'm fine with showing

[Matt Baker]

(In reply to Nikita Vasilyev from comment #16)
> Because this is what it shows for empty property names:
> 
>     -apple-color-filter
>     -apple-pay-button-style
>     -apple-pay-button-type
>     -apple-trailing-word
>     -epub-caption-side
>     -epub-hyphens
>     -epub-text-combine
>     -epub-text-emphasis
>     -epub-text-emphasis-color
>     -epub-text-emphasis-style
>     -epub-text-orientation
> 
> I'm to show completions for empty property names once we land your patch to
> show the most commonly used properties first.

I'm not sure we want to do this. The user probably isn't adding a new CSS property unless they have at least an idea of what the property is, even if they can't remember, say, how to spell it. Sorting the list by commonly used properties will surface useful properties some of the time, but it is still distracting at best IMO.

[Matt Baker]

Comment on attachment 363042 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:379
> +        const options = {allowEmptyPrefix: forceCompletions};

This can be inlined:

let {completions, prefix} = this._completionProvider(valueWithoutSuggestion, {allowEmptyPrefix: forceCompletions});

[Nikita Vasilyev]

Created attachment 363252 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 363252 [details]
Patch for landing

Clearing flags on attachment: 363252

Committed r242218: <https://trac.webkit.org/changeset/242218>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikita Vasilyev]

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