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]
:
: J. Ryan Stinnett [:jryans] (use ni?)
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 | Splinter Review
Patch with tests (12.27 KB, patch)
2011-09-01 07:46 PDT, Panos Astithas [:past]
rcampbell: review+
Details | Diff | Splinter Review
Patch v3 (11.53 KB, patch)
2011-09-02 01:36 PDT, Panos Astithas [:past]
rcampbell: review+
Details | Diff | Splinter Review
[in-fx-team] Patch v4 (12.39 KB, patch)
2011-10-03 01:38 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review

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

    
Comment 1 User image David Bolter [:davidb] 2011-07-20 12:26:41 PDT
STR: unplug your mouse.
Comment 2 User image 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 User image Rob Campbell [:rc] (:robcee) 2011-07-26 09:18:59 PDT
left: parent
right: first child
up: prev sibling
down: next sibling
Comment 4 User image 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 User image 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 User image Rob Campbell [:rc] (:robcee) 2011-08-19 12:42:41 PDT
whups, yes indeed. Thanks Wayne and Panos!
Comment 7 User image Panos Astithas [:past] 2011-08-29 08:26:54 PDT
I just started working on this.
Comment 8 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Rob Campbell [:rc] (:robcee) 2011-10-04 05:24:35 PDT
https://hg.mozilla.org/mozilla-central/rev/f425a8109714
Comment 16 User image 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.