Summary: | Web Inspector: When autocompleting, pressing tab twice shouldn't insert a tab character | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||
Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | burg, chris, commit-queue, graouts, hi, joepeck, jonowells, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=146189 https://bugs.webkit.org/show_bug.cgi?id=146496 |
||||||||||||||||||
Attachments: |
|
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. Created attachment 255263 [details]
Patch
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. Created attachment 255311 [details]
Patch
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. Created attachment 255312 [details]
Patch
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? 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.
Created attachment 255785 [details]
Patch
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". 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 Created attachment 255986 [details]
Patch
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. +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 on attachment 255986 [details] Patch Clearing flags on attachment: 255986 Committed r186217: <http://trac.webkit.org/changeset/186217> All reviewed patches have been landed. Closing bug. |
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.