Bug 23849 - Keyboard shortcuts needed in Web Inspector
Summary: Keyboard shortcuts needed in Web Inspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 525.x (Safari 3.2)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-09 10:29 PST by Daniel Gackle
Modified: 2009-07-07 10:57 PDT (History)
7 users (show)

See Also:


Attachments
Patch adding debugger and call stack navigation shortcuts. (10.62 KB, patch)
2009-06-26 01:55 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch adding debugger and call stack navigation shortcuts(with copyright fix). (10.92 KB, patch)
2009-06-26 02:02 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch adding debugger and call stack navigation shortcuts. (11.40 KB, patch)
2009-06-26 02:10 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Add Firebug-compatible debugger and stack navigation shortcuts. (12.47 KB, patch)
2009-06-29 02:38 PDT, Yury Semikhatsky
timothy: review-
Details | Formatted Diff | Diff
Previous patch with Timothy's comments addressed. (13.03 KB, patch)
2009-06-30 02:23 PDT, Yury Semikhatsky
timothy: review-
Details | Formatted Diff | Diff
Replaced the tabs with spaces. (13.04 KB, patch)
2009-06-30 23:43 PDT, Yury Semikhatsky
timothy: review+
Details | Formatted Diff | Diff
Change 'Continue' shortcut from F5 to F8 (1.38 KB, patch)
2009-07-06 00:45 PDT, Yury Semikhatsky
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Gackle 2009-02-09 10:29:05 PST
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
Comment 1 Yury Semikhatsky 2009-06-26 01:55:16 PDT
Created attachment 31920 [details]
Patch adding debugger and call stack navigation shortcuts.
Comment 2 Francois Botha 2009-06-26 02:01:26 PDT
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.
Comment 3 Yury Semikhatsky 2009-06-26 02:02:53 PDT
Created attachment 31921 [details]
Patch adding debugger and call stack navigation shortcuts(with copyright fix).
Comment 4 Yury Semikhatsky 2009-06-26 02:10:31 PDT
Created attachment 31922 [details]
Patch adding debugger and call stack navigation shortcuts.
Comment 5 Yury Semikhatsky 2009-06-26 05:29:41 PDT
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.
Comment 6 Yury Semikhatsky 2009-06-29 02:38:17 PDT
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.
Comment 7 Francois Botha 2009-06-29 03:20:41 PDT
Would somebody care to comment on the F5 vs F8 issue? Refer to comment #2
Comment 8 Yury Semikhatsky 2009-06-29 03:32:43 PDT
(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).
Comment 9 Francois Botha 2009-06-29 04:25:28 PDT
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?
Comment 10 Yury Semikhatsky 2009-06-29 04:49:49 PDT
(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.
Comment 11 Timothy Hatcher 2009-06-29 09:58:21 PDT
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!
Comment 12 Timothy Hatcher 2009-06-29 10:01:28 PDT
It would also be better to use keyIdentifier instead of keyCode (e.g. event.keyIdentifier === "Right").
Comment 13 Yury Semikhatsky 2009-06-30 02:23:05 PDT
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]();
Comment 14 Yury Semikhatsky 2009-06-30 02:31:42 PDT
(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.
Comment 15 Adam Roben (:aroben) 2009-06-30 06:30:22 PDT
(In reply to comment #14)
> On Windows pressing F10 gives me keyIdentifier F11

This sounds like a bug. Could you please file it?
Comment 16 Timothy Hatcher 2009-06-30 13:25:35 PDT
Comment on attachment 32044 [details]
Previous patch with Timothy's comments addressed.

There are tabs in CallStackSidebarPane.js. Otherwise fine.
Comment 17 Yury Semikhatsky 2009-06-30 23:43:19 PDT
Created attachment 32116 [details]
Replaced the tabs with spaces.
Comment 18 Yury Semikhatsky 2009-06-30 23:56:44 PDT
(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
Comment 19 Eric Seidel 2009-07-01 18:03:39 PDT
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.
Comment 20 Francois Botha 2009-07-03 03:35:57 PDT
> 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.
Comment 21 Francois Botha 2009-07-04 00:20:37 PDT
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.
Comment 22 Yury Semikhatsky 2009-07-06 00:45:33 PDT
Created attachment 32287 [details]
Change 'Continue' shortcut from F5 to F8
Comment 23 Pavel Feldman 2009-07-07 10:57:55 PDT
Sending        WebCore/ChangeLog
Sending        WebCore/inspector/front-end/ScriptsPanel.js
Transmitting file data ..
Committed revision 45591.