672902 - Highlighter should be useable via keyboard.

Status: VERIFIED FIXED

(DevTools :: General, defect, P1)

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

STR: unplug your mouse.

Comment 2

[Rob Campbell [:rc] (:robcee)]

Currently it doesn't seem to be possible to focus the HTML panel via the keyboard. We should also have global key commands for selecting previous/next sibling and parent/first-child.

Comment 3

[Rob Campbell [:rc] (:robcee)]

left: parent
right: first child
up: prev sibling
down: next sibling

Comment 4

[Wayne Mery (:wsmwk)]

robcee, if you have correctly keyed dependency on bug 669659, shouldn't it really be bug 644733?

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Wayne Mery (:wsmwk) from comment #4)
> robcee, if you have correctly keyed dependency on bug 669659, shouldn't it
> really be bug 644733?

Actually I believe this was an off-by-one error. The bug this depends on is about improving the inspector keybindings.

Comment 6

[Rob Campbell [:rc] (:robcee)]

whups, yes indeed. Thanks Wayne and Panos!

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

I just started working on this.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: First draft

This patch seems to work fine and not break anything. I'll be adding tests tomorrow, but just wanted to make sure I'm on the right track.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch with tests

Got the tests ready, so let's turn this into a proper review.

Comment 10

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 557494 [details] [diff] [review]
Patch with tests

In:

inspector.js

line 75:

+            let selection = this.selection;

Don't think we need the variable declaration.

line 79:

+              node = gBrowser.selectedBrowser.contentWindow.document.documentElement;

node = this.defaultSelection;

line 88:

+            selection = this.selection;

still don't need it! Though if it's convenient and saves space, you could cache it outside the switch condition.

nice test! r+ with the above fixed. I *think* that test looks good. I'll give it a look-see tomorrow.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v3

(In reply to Rob Campbell [:rc] (robcee) from comment #10)
> Comment on attachment 557494 [details] [diff] [review]
> Patch with tests
> 
> In:
> 
> inspector.js
> 
> line 75:
> 
> +            let selection = this.selection;
> 
> Don't think we need the variable declaration.
> 
> line 79:
> 
> +              node =
> gBrowser.selectedBrowser.contentWindow.document.documentElement;
> 
> node = this.defaultSelection;
> 
> line 88:
> 
> +            selection = this.selection;
> 
> still don't need it! Though if it's convenient and saves space, you could
> cache it outside the switch condition.
> 
> nice test! r+ with the above fixed. I *think* that test looks good. I'll
> give it a look-see tomorrow.

All fixed, and the defaultSelection even simplified the test a bit.

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 557780 [details] [diff] [review]
Patch v3

I'm ok with these changes. We should get gavin to give it a once-over.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: [in-fx-team] Patch v4

Rebased and updated, after the the inspector move into the devtools directory. Gavin, I can take this off your plate now, but if you were already in the middle of the review or still want to do it regardless, you are most welcome. 

Try run results at:

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ba831a3f48ca

Comment 14

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 564142 [details] [diff] [review]
[in-fx-team] Patch v4

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

Comment 15

[Rob Campbell [:rc] (:robcee)]

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

Comment 16

[Teodosia Pop]

Verified Fixed using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1.