Bug 23849 – Keyboard shortcuts needed in Web Inspector

• Component: New Bugs
• Status: RESOLVED FIXED

[Daniel Gackle]

It would be a big help to developers working in Web Inspector if the debugger allowed for keyboard shortcuts - having to use the mouse to click through every line is a significant drag (though the debugger works great otherwise). The standard shortcuts that I'm familiar with, and which Firebug also uses, are:

F10 - step over
F11 - step into
Shift+F10 - step out
F5 or F8 - continue
F9 - toggle breakpoint

[Yury Semikhatsky]

Created attachment 31920 [details]
Patch adding debugger and call stack navigation shortcuts.

[Francois Botha]

In Firebug, F8 is used for Continue.  F5 causes the page to reload (On my Windows XP, Fx 3.5 system at least). Therefore, my preference is for F8 instead of F5 for Continue.

[Yury Semikhatsky]

Created attachment 31921 [details]
Patch adding debugger and call stack navigation shortcuts(with copyright fix).

[Yury Semikhatsky]

Created attachment 31922 [details]
Patch adding debugger and call stack navigation shortcuts.

[Yury Semikhatsky]

Comment on attachment 31922 [details]
Patch adding debugger and call stack navigation shortcuts.

The patch needs to be changed to work properly on all platforms. Marking it as obsolete.

[Yury Semikhatsky]

Created attachment 32002 [details]
Add Firebug-compatible debugger and stack navigation shortcuts.

On Mac most functional keys are bound as OS shortcuts which means they cannot be used as the debugger shortcuts. To overcome this I've added several shortcuts that don't conflict with OS. Those are the keys used in Firebug so users should feel comfortable with them.

[Francois Botha]

Would somebody care to comment on the F5 vs F8 issue? Refer to comment #2

[Yury Semikhatsky]

(In reply to comment #7)
> Would somebody care to comment on the F5 vs F8 issue? Refer to comment #2
> 
If focus is on the sidebar pane pressing F5 should just continue execution(as long as this patch is applied).

[Francois Botha]

I'm aware that F5 will not refresh the page *in Chrome*.  But my point is that it *does* refresh the page in Firefox + Firebug.  So, if you want to be *consistent*, surely F8 is the right choice?

[Yury Semikhatsky]

(In reply to comment #9)
> I'm aware that F5 will not refresh the page *in Chrome*.  But my point is that
> it *does* refresh the page in Firefox + Firebug.  So, if you want to be
> *consistent*, surely F8 is the right choice?
> 
It looks to me as a bug in Firebug and I'd rather avoid repeating it in WebKit.

[Timothy Hatcher]

Comment on attachment 32002 [details]
Add Firebug-compatible debugger and stack navigation shortcuts.


> +    this.shortcuts_ = {};

Why does this end with an underscore? THis dosen't match any of our naming conventions. Just remove the underscore or use a underscore prefix if the property is "private".

> +    handleKeyEvent: function(event) {

Open curly brace should be on the next line.

> +      var shortcut = WebInspector.KeyboardShortcut.makeKeyFromEvent(event);
> +      var handler = this.shortcuts_[shortcut];
> +      if (handler) {
> +        handler(event);
> +        event.preventDefault();
> +        event.handled = true;
> +      }
> +    },

Use 4 space indents.

> +    _selectNextCallFrameOnStack: function() {

Open curly brace should be on the next line.

> +    _selectPreviousCallFrameOnStack: function() {

Open curly brace should be on the next line.

> +    _selectedPlacardByIndex: function(index) {

Open curly brace should be on the next line.

> +    _selectedCallFrameIndex: function() {

Open curly brace should be on the next line.

> +            if (placard.callFrame === this._selectedCallFrame) {
> +                return i;
> +            }

No need for the curly brances for one line.

> +WebInspector.KeyboardShortcut.Modifiers = {
> +    NONE: 0,
> +    SHIFT: 1,
> +    CTRL: 2,
> +    ALT: 4,
> +    META: 8
> +};

No need for the semicolon. I would also like to see these be CamelCase and spelled out.

> +WebInspector.KeyboardShortcut.KeyCodes = {
> +    ESC: 27,
> +    SPACE: 32,
> +    PAGE_UP: 33,     // also NUM_NORTH_EAST
> +    PAGE_DOWN: 34,   // also NUM_SOUTH_EAST
> +    END: 35,         // also NUM_SOUTH_WEST
> +    HOME: 36,        // also NUM_NORTH_WEST
> +    LEFT: 37,        // also NUM_WEST
> +    UP: 38,          // also NUM_NORTH
> +    RIGHT: 39,       // also NUM_EAST
> +    DOWN: 40,        // also NUM_SOUTH
> +    F1: 112,
> +    F2: 113,
> +    F3: 114,
> +    F4: 115,
> +    F5: 116,
> +    F6: 117,
> +    F7: 118,
> +    F8: 119,
> +    F9: 120,
> +    F10: 121,
> +    F11: 122,
> +    F12: 123,
> +    SEMICOLON: 186,
> +    COMMA: 188,
> +    PERIOD: 190,
> +    SLASH: 191,
> +    APOSTROPHE: 192,
> +    SINGLE_QUOTE: 222,
> +};

Same here. CamelCase and spelled out for the non function keys.

> +WebInspector.KeyboardShortcut.makeKey = function(keyCode, opt_modifiers) {

Open curly brace should be on the next line. The opt_modifiers parameter should be spelled out and camelCase instead of an underscore.

> +    for (var i = 1; i < arguments.length; i++) {
> +        modifiers |= arguments[i];
> +    }

No need for the curly brances for one line.

> +WebInspector.KeyboardShortcut.makeKeyFromCodeAndModifiers_ = function(keyCode, modifiers) {
> +    return (keyCode & 255) | (modifiers << 8);
> +};

The underscore should be a prefix if this is a "private" function.

> +    this.shortcuts_ = {};

Use an underscore prefix or drop the underscore.

> +    handleKeyEvent: function(event) {

Open curly brace should be on the next line.

> +      var shortcut = WebInspector.KeyboardShortcut.makeKeyFromEvent(event);
> +      var handler = this.shortcuts_[shortcut];
> +      if (handler) {
> +        handler(event);
> +        event.preventDefault();
> +        event.handled = true;
> +      } else {
> +        this.sidebarPanes.callstack.handleKeyEvent(event);
> +      }
> +    },

Use 4 space indent.

r- purely on code style. Functionally correct!

[Timothy Hatcher]

It would also be better to use keyIdentifier instead of keyCode (e.g. event.keyIdentifier === "Right").

[Yury Semikhatsky]

Created attachment 32044 [details]
Previous patch with Timothy's comments addressed.

I've  fixed all of the pointed issues(sorry, it's hard to switch between two different style guides) except for the semicolons. If you don't mind I'd rather have each expression ending with semicolon explicitly to avoid possible nasty errors happening when semicolon is accidentally omitted between two expressions like in the following example: 

var x = {
  'i': 1,
  'j': 2
} 
// 2.  Trying to do one thing on IE and another on FF.
// I know you'd never write code like this, but throw me a bone.
[normalVersion, ffVersion][isIE]();

[Yury Semikhatsky]

(In reply to comment #12)
> It would also be better to use keyIdentifier instead of keyCode (e.g.
> event.keyIdentifier === "Right").
> 

On Windows pressing F10 gives me keyIdentifier F11, pressing F11 gives U+005A. Also for most of the other keys it's something like U+00BF which means it would require introducing constants for those keys anyway. Using uniform numerical constants for keyCode values looks more convenient.

[Adam Roben (:aroben)]

(In reply to comment #14)
> On Windows pressing F10 gives me keyIdentifier F11

This sounds like a bug. Could you please file it?

[Timothy Hatcher]

Comment on attachment 32044 [details]
Previous patch with Timothy's comments addressed.

There are tabs in CallStackSidebarPane.js. Otherwise fine.

[Yury Semikhatsky]

Created attachment 32116 [details]
Replaced the tabs with spaces.

[Yury Semikhatsky]

(In reply to comment #15)
> (In reply to comment #14)
> > On Windows pressing F10 gives me keyIdentifier F11
> 
> This sounds like a bug. Could you please file it?
> 

Done: https://bugs.webkit.org/show_bug.cgi?id=26878

[Eric Seidel (no email)]

Sending        WebCore/ChangeLog
Sending        WebCore/inspector/front-end/CallStackSidebarPane.js
Adding         WebCore/inspector/front-end/KeyboardShortcut.js
Sending        WebCore/inspector/front-end/ScriptsPanel.js
Sending        WebCore/inspector/front-end/WebKit.qrc
Sending        WebCore/inspector/front-end/inspector.html
Transmitting file data ......
Committed revision 45460.

[Francois Botha]

> It looks to me as a bug in Firebug and I'd rather avoid repeating it in WebKit.

Possible bug filed at http://code.google.com/p/fbug/issues/detail?id=1960 for clarification.  If it's not a bug (as I suspect) I hope this patch gets adjusted.

[Francois Botha]

Firebug developers confirm that the Continue button is F8, not F5.
http://code.google.com/p/fbug/issues/detail?id=1960 

So, if you want to be consistent, emulate Firebug. 

Emulating Visual Studio makes less sense because web developers use Firebug a lot more than VS.

[Yury Semikhatsky]

Created attachment 32287 [details]
Change 'Continue' shortcut from F5 to F8

[Pavel Feldman]

Sending        WebCore/ChangeLog
Sending        WebCore/inspector/front-end/ScriptsPanel.js
Transmitting file data ..
Committed revision 45591.