Bug 31400

Summary: Web Inspector: Support Ctrl+P, Ctrl+N, and other Readline keyboard shortcuts in the Console
Product: WebKit Reporter: Sam Aaron <samaaron.public>
Component: Web Inspector (Deprecated)Assignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: bweinstein, joepeck, keishi, pfeldman, pmuellr, rik, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.6   
Attachments:
Description Flags
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U for Macs Only
timothy: review-
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U for Macs Only
none
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U, Arrow Keys
none
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U, Arrow Keys
timothy: review-
[PATCH] Ctrl+P, Ctrl+N, and Arrow Keys timothy: review+

Description Sam Aaron 2009-11-12 03:17:44 PST
My muscle memory keeps trying ctrl-p to access history which totally fails to do anything at all.  Would it be possible to map ctrl-p and ctrl-n to up and down in the console to mimic the behaviour of Terminal.app?
Comment 1 Joseph Pecoraro 2009-11-20 23:00:04 PST
These are well known Readline keyboard shortcuts. Terminal.app Supports a number of them. Here is a general list:
http://www.bigsmoke.us/readline/shortcuts

keishi mentioned on IRC:
> I think it's good for os x but I'm wasn't sure about Windows.
> I'm not sure. I was wondering if it had any conflicts when i read it.
> all the readline key bindings don't work in Windows.
> http://en.wikipedia.org/wiki/Control_key seems like Ctrl+P Ctrl+N
> wouldn't have conflicts in the console. I like the readline key bindings
> and I want this, but I just wonder if it's natural for Windows users.
Comment 2 Joseph Pecoraro 2009-11-20 23:01:22 PST
Created attachment 43644 [details]
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U for Macs Only

Also fixes modifier keys (Shift/Alt/Meta/Control) from re-triggering the type ahead.
Comment 3 Brian Weinstein 2009-11-20 23:06:56 PST
I'm not sure the best way to handle this on Windows, maybe the meta key would work (Windows key).
Comment 4 Timothy Hatcher 2009-11-20 23:29:22 PST
Comment on attachment 43644 [details]
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U for Macs Only


> +                // fall through to default
> +            case "U+004E": // Ctrl+N = Next

Are you sure this falls through to the default? I thought it fell through to the next case.
Comment 5 Timothy Hatcher 2009-11-21 00:21:45 PST
Comment on attachment 43644 [details]
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U for Macs Only

The switch needs fixed.

Example:

switch(true) {
  case false:
    alert('false');
  case true:
    alert('true');
  case false:
    alert('false');
  case false:
    alert('false');
  default:
    alert('default');
}
Comment 6 Joseph Pecoraro 2009-11-21 07:11:52 PST
(In reply to comment #4)
> > +                // fall through to default
> > +            case "U+004E": // Ctrl+N = Next
> 
> Are you sure this falls through to the default? I thought it fell through to
> the next case.

Correct, it falls through to the next case, which then falls through to the next case, and eventually will end up at default. This cases a few extra conditions to be checked, so maybe I should just refactor this anyway.
Comment 7 Joseph Pecoraro 2009-11-21 07:26:13 PST
Created attachment 43648 [details]
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U for Macs Only

More explicit behavior in the switch. No fall throughs.

The only thing I am worried about is the O(n) behavior of deleting nodes in clearBeforeCaret. Even though my tests showed it was immediate for a rather large paste, I still think there should be a better way to delete a range of nodes in 1 operation, rather then n. Any pointers?
Comment 8 Joseph Pecoraro 2009-11-21 09:01:27 PST
Created attachment 43651 [details]
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U, Arrow Keys

Included arrow keys for multiline here because bug 30553 seems to have diverted its attention to Snippets.
Comment 9 Joseph Pecoraro 2009-11-21 09:08:30 PST
Video Showing Arrow Traversals so you can get a feel:
http://screencast.com/t/MTBlMGZiNjA
Comment 10 Joseph Pecoraro 2009-11-21 09:16:34 PST
Created attachment 43652 [details]
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U, Arrow Keys

Removed a "+1" I had added that shouldn't have been there and caused bad behavior.
Comment 11 Timothy Hatcher 2009-11-21 19:10:18 PST
Comment on attachment 43652 [details]
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U, Arrow Keys

> +        this.clearAutoComplete(true);
> +        if (this.isCaretAtEndOfPrompt()) {
> +            this.element.removeChildren();
> +            this.element.appendChild(document.createElement("br"));
> +            this.moveCaretToEndOfPrompt();
> +            return;
> +        }
> +
> +        while (focusNode.previousSibling)
> +            this.element.removeChild(focusNode.previousSibling);
> +        focusNode.textContent = focusNode.textContent.substring(selection.focusOffset);
> +        if (this.element.textContent.length === 0)
> +            this.element.appendChild(document.createElement("br"));
> +
> +        var newRange = document.createRange();
> +        newRange.setStart(focusNode, 0);
> +        newRange.setEnd(focusNode, 0);
> +
> +        selection.removeAllRanges();
> +        selection.addRange(newRange);

This can be simplified. Just make a DOM range that starts at the beginning and ends at the focusNode and focusOffset. Then call removeContents. This should leave you with a good caret selection too. You might need the <br> still if totally empty.

Even better:

    this.element.execCommand("DeleteToBeginningOfLine")

But looking at it, I don't think DeleteToBeginningOfLine is allowed from execCommand! Maybe we can change it to be "supported" not just "supportedFromMenuOrKeyBinding"?

http://trac.webkit.org/browser/trunk/WebCore/editing/EditorCommand.cpp#L1313

> +            selectionRange.setStart(this.element.firstChild, firstNewlineIndex);
> +            selectionRange.setEnd(this.element.firstChild, firstNewlineIndex);

Is it true the first child is always a text node that contains this index?
Comment 12 Joseph Pecoraro 2009-11-21 19:33:13 PST
Created attachment 43669 [details]
[PATCH] Ctrl+P, Ctrl+N, and Arrow Keys

Removed Ctrl+U based on chat in IRC, because It should act like Ctrl+K and put the content into the Kill Ring (usable with Ctrl+Y), but we cannot do that and it may be confusing.
Comment 13 Joseph Pecoraro 2009-11-21 19:40:26 PST
Landed in http://trac.webkit.org/changeset/51291
r51291 = 5e12adf30e89cc9f952fb8cd7b39a16731aadb2b

I will look into this.element.execCommand("DeleteToBeginningOfLine") when I find time. I'll close this bug.