Bug 145885 - Web Inspector: When autocompleting, pressing tab twice shouldn't insert a tab character
Summary: Web Inspector: When autocompleting, pressing tab twice shouldn't insert a tab...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2015-06-11 11:03 PDT by Nikita Vasilyev
Modified: 2015-07-01 21:31 PDT (History)
11 users (show)

See Also:

Animated GIF of the problem (11.06 KB, image/gif)
2015-06-11 11:03 PDT, Nikita Vasilyev
no flags Details
Patch (5.65 KB, patch)
2015-06-19 18:23 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (10.48 KB, patch)
2015-06-20 22:16 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (4.08 KB, patch)
2015-06-20 22:29 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Video] Expected behavior (165.31 KB, video/mp4)
2015-06-22 20:12 PDT, Nikita Vasilyev
no flags Details
Patch (4.20 KB, patch)
2015-06-29 15:47 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (9.03 KB, patch)
2015-07-01 19:33 PDT, Devin Rousso
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 2015-06-11 11:03:14 PDT
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.
Comment 1 Radar WebKit Bug Importer 2015-06-11 11:03:52 PDT
Comment 2 Chris Chiera 2015-06-15 10:02:48 PDT
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.
Comment 3 Devin Rousso 2015-06-19 18:23:35 PDT
Created attachment 255263 [details]
Comment 4 Nikita Vasilyev 2015-06-19 18:42:29 PDT
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.
Comment 5 Devin Rousso 2015-06-20 22:16:56 PDT
Created attachment 255311 [details]
Comment 6 Devin Rousso 2015-06-20 22:27:09 PDT
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.
Comment 7 Devin Rousso 2015-06-20 22:29:40 PDT
Created attachment 255312 [details]
Comment 8 Timothy Hatcher 2015-06-22 14:25:33 PDT
Comment on attachment 255312 [details]

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?
Comment 9 Nikita Vasilyev 2015-06-22 20:12:03 PDT
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

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

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.
Comment 10 Devin Rousso 2015-06-29 15:47:58 PDT
Created attachment 255785 [details]
Comment 11 Timothy Hatcher 2015-06-30 09:33:33 PDT
Comment on attachment 255785 [details]

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".
Comment 12 Nikita Vasilyev 2015-06-30 23:43:41 PDT
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
Comment 13 Devin Rousso 2015-07-01 19:33:30 PDT
Created attachment 255986 [details]
Comment 14 Timothy Hatcher 2015-07-01 20:40:52 PDT
Comment on attachment 255986 [details]

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

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


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

Nice and clean this way.
Comment 15 Joseph Pecoraro 2015-07-01 21:20:32 PDT
+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?
Comment 16 WebKit Commit Bot 2015-07-01 21:31:51 PDT
Comment on attachment 255986 [details]

Clearing flags on attachment: 255986

Committed r186217: <http://trac.webkit.org/changeset/186217>
Comment 17 WebKit Commit Bot 2015-07-01 21:31:56 PDT
All reviewed patches have been landed.  Closing bug.