Bug 31400 – Web Inspector: Support Ctrl+P, Ctrl+N, and other Readline keyboard shortcuts in the Console

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Sam Aaron]

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?

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Brian Weinstein]

I'm not sure the best way to handle this on Windows, maybe the meta key would work (Windows key).

[Timothy Hatcher]

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.

[Timothy Hatcher]

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');
}

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

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?

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Video Showing Arrow Traversals so you can get a feel:
http://screencast.com/t/MTBlMGZiNjA

[Joseph Pecoraro]

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.

[Timothy Hatcher]

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?

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.