1198627 - Breadcrumbs stop responding to keyboard events

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Geoff Lankow (:darktrojan)]

I think the cause of this might be if you try to go beyond the start or end of the breadcrumbs using the arrow keys, or by pressing a key that isn't an arrow key. A quick look at the code seems to agree with the latter.

I also get this error on the console (some irrelevant bits removed):

> A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
> See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise> 

> Date: Wed Aug 26 2015 20:25:23 GMT+1200 (NZST)
> Full Message: TypeError: node is null
> Full Stack: HTMLBreadcrumbs.prototype.handleKeyPress/this._keyPromise<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/breadcrumbs.js:383:7
> ...
> HTMLBreadcrumbs.prototype.handleKeyPress@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/breadcrumbs.js:359:25
> HTMLBreadcrumbs.prototype.handleEvent@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/breadcrumbs.js:288:7
> EventListener.handleEvent*HTMLBreadcrumbs.prototype._init@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/breadcrumbs.js:72:5
> HTMLBreadcrumbs@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/breadcrumbs.js:48:3
> InspectorPanel.prototype._deferredOpen@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:158:24
> InspectorPanel.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:98:14
> ...
> Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1228:9
> DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1178:7
> frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1330:14
> exports.WalkerFront<.querySelector<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:3220:12
> InspectorPanel.prototype._getDefaultNodeForSelection/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:265:14
> ...
> Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1228:9
> DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1178:7
> frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1330:14
> exports.InspectorFront<.getPageStyle<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:3715:12
> InspectorPanel.prototype._getPageStyle@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:226:12
> InspectorPanel.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:94:14
> ...
> InspectorPanel.prototype.open@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:93:12
> Toolbox.prototype.loadTool/onLoad@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:1171:19

Comment 1

[Patrick Brosset <:pbro>]

The node variable at breadcrumbs.js:383 can be null in some cases, so it should be a simple fix.
However, I realized there are absolutely no tests covering this in the tree, adding one is what's going to take the most time here.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug_1198627_-_Prevent_errors_when_keyboard_navigat.diff

Comment 3

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=106db2b4f666

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8652821 [details] [diff] [review]
Bug_1198627_-_Prevent_errors_when_keyboard_navigat.diff

Review of attachment 8652821 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 5

[Geoff Lankow (:darktrojan)]

While we're here, what happens to keyboard events that aren't from arrow keys? F12, for example. They don't seem to bubble up to a listener that does anything with them.

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Geoff Lankow (:darktrojan) from comment #5)
> While we're here, what happens to keyboard events that aren't from arrow
> keys? F12, for example. They don't seem to bubble up to a listener that does
> anything with them.
Let's deal with this in another bug.
I filed bug 1199102 for this. Could you provide a bit more information in that bug?

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/da72f4038221

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da72f4038221