Web Inspector: When autocompleting, pressing tab twice shouldn't insert a tab character

[Nikita Vasilyev]

Created attachment 254732 [details]
Animated GIF of the problem

It is very unlikely that someone would want to insert a tab character not for indentation.

[Radar WebKit Bug Importer]

<rdar://problem/21342410>

[Chris Chiera]

To add, this bug also occurs in the styles panel. If you type for instance, "mar tab" it auto completes margin: and jumps to the value properly. You enter a value such as 10px then click tab again but rather than now jumping to add a new rule, it simply adds a tab which would serve 0 purpose in css. Occurring in Safari 8.1. Working in Chrome.

[Devin Rousso]

Created attachment 255263 [details]
Patch

[Nikita Vasilyev]

https://bug-145885-attachments.webkit.org/attachment.cgi?id=254732 shows the problem in the console. This patch tackles the problem only in the styles panel; it should go into a new separate bug.

[Devin Rousso]

Created attachment 255311 [details]
Patch

[Devin Rousso]

Sorry Nikita, I didn't see your last comment.  I just created a new ticket for the styles sidebar tabbing functionality.  I'll remove that code from this patch.

[Devin Rousso]

Created attachment 255312 [details]
Patch

[Timothy Hatcher]

Comment on attachment 255312 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ConsolePrompt.js:169
> +    _handleShiftTabKey: function(codeMirror)

How does this differ from the built-in goWordLeft, goWordRight support in CodeMirror? (Control-Option-F & Control-Option-B) Maybe we should just wire up to those commands.

That would just be:
"Shift-Tab": "goWordLeft",
"Tab": "goWordRight"

Also, how does this handle autocompletion use of the Tab key?

[Nikita Vasilyev]

Created attachment 255393 [details]
[Video] Expected behavior

Let me explain what this issue is all about.

Steps to reproduce:
1. Open Console
2. Type "wind", suggestion popover with only one item, window, should show up.
3. Press Tab key, it should autocomplete
4. Press Tab again

Actual:
A tab character gets inserted after window, e.g. "window\t|"

Expected:
Nothing happens on the seconds Tab key press, e.g. "window|"

This would match Firefox DevTools' (video attached) and Chrome DevTools' behavior.


Regarding the patch: once again, there should a separate issue for that. I’m not against it, but it’s outside of the scope of this issue.

[Devin Rousso]

Created attachment 255785 [details]
Patch

[Timothy Hatcher]

Comment on attachment 255785 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:11
> +        (WebInspector.ConsolePrompt.prototype._handleOnKeyEvent): Pressing any key other than tab will remove
> +        the "<- no results" if it exists.

I don't think this is needed. You could just call InspectorFrontendHost.beep() if tab is pressed and does nothing, that is more inline with what happens on Mac for invalid keypresses.

> Source/WebInspectorUI/UserInterface/Views/ConsolePrompt.js:194
> +        if (trimmedLine.endsWith(".")) {
> +            this._completionController._completeAtCurrentPosition(true);
> +            return;
> +        }

This is using private API and is making assumptions (endsWith(".")) about the completion controller that might not always be correct.

It would be best to have a public API like "completeAtCurrentPositionIfNeeded".

[Nikita Vasilyev]

This patch works as expected.

Showing Firefox's style "<- no results" might be a bit excessive.

On a slightly related topic, I have found an another autocomplete bug: https://bugs.webkit.org/show_bug.cgi?id=146496

[Devin Rousso]

Created attachment 255986 [details]
Patch

[Timothy Hatcher]

Comment on attachment 255986 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/WrappedPromise.js:26
> +WebInspector.WrappedPromise = class WrappedPromise

Neat!

> Source/WebInspectorUI/UserInterface/Views/ConsolePrompt.js:170
> +        this._completionController.completeAtCurrentPositionIfNeeded().then(function(result) {

Nice and clean this way.

[Joseph Pecoraro]

+Brian. Your thoughts on the WebInspector.WrappedPromise? I prefer it for most async work that can't easily be resolved via an event. Think it will hit any anti-patterns?

[WebKit Commit Bot]

Comment on attachment 255986 [details]
Patch

Clearing flags on attachment: 255986

Committed r186217: <http://trac.webkit.org/changeset/186217>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.