Last Comment Bug 672902 - Highlighter should be useable via keyboard.
: Highlighter should be useable via keyboard.
Status: VERIFIED FIXED
[highlighter][minotaur][best: 3h: lik...
: access
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 10
Assigned To: Panos Astithas [:past]
:
Mentors:
Depends on: 669658
Blocks: 663830
  Show dependency treegraph
 
Reported: 2011-07-20 12:25 PDT by David Bolter [:davidb]
Modified: 2012-12-21 08:43 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First draft (5.92 KB, patch)
2011-08-31 10:04 PDT, Panos Astithas [:past]
no flags Details | Diff | Review
Patch with tests (12.27 KB, patch)
2011-09-01 07:46 PDT, Panos Astithas [:past]
rcampbell: review+
Details | Diff | Review
Patch v3 (11.53 KB, patch)
2011-09-02 01:36 PDT, Panos Astithas [:past]
rcampbell: review+
Details | Diff | Review
[in-fx-team] Patch v4 (12.39 KB, patch)
2011-10-03 01:38 PDT, Panos Astithas [:past]
no flags Details | Diff | Review

Description David Bolter [:davidb] 2011-07-20 12:25:57 PDT

    
Comment 1 David Bolter [:davidb] 2011-07-20 12:26:41 PDT
STR: unplug your mouse.
Comment 2 Rob Campbell [:rc] (:robcee) 2011-07-20 12:29:35 PDT
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) 2011-07-26 09:18:59 PDT
left: parent
right: first child
up: prev sibling
down: next sibling
Comment 4 Wayne Mery (:wsmwk, NI for questions) 2011-08-19 08:14:23 PDT
robcee, if you have correctly keyed dependency on bug 669659, shouldn't it really be bug 644733?
Comment 5 Panos Astithas [:past] 2011-08-19 09:25:56 PDT
(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) 2011-08-19 12:42:41 PDT
whups, yes indeed. Thanks Wayne and Panos!
Comment 7 Panos Astithas [:past] 2011-08-29 08:26:54 PDT
I just started working on this.
Comment 8 Panos Astithas [:past] 2011-08-31 10:04:51 PDT
Created attachment 557221 [details] [diff] [review]
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 [:past] 2011-09-01 07:46:17 PDT
Created attachment 557494 [details] [diff] [review]
Patch with tests

Got the tests ready, so let's turn this into a proper review.
Comment 10 Rob Campbell [:rc] (:robcee) 2011-09-01 17:55:10 PDT
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 [:past] 2011-09-02 01:36:20 PDT
Created attachment 557780 [details] [diff] [review]
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) 2011-09-02 11:02:05 PDT
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 [:past] 2011-10-03 01:38:09 PDT
Created attachment 564142 [details] [diff] [review]
[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) 2011-10-03 08:23:49 PDT
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) 2011-10-04 05:24:35 PDT
https://hg.mozilla.org/mozilla-central/rev/f425a8109714
Comment 16 Teodosia Pop 2011-10-07 02:27:37 PDT
Verified Fixed using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1.

Note You need to log in before you can comment on or make changes to this bug.