Kostas Reporter
	Description • 10 years ago

I quote from https://adblockplus.org/en/elemhidehelper
> You start Element Hiding Helper by going into the menu of ABP's toolbar icon 
> and selecting "Select element to hide".
> Now you can select the element you want to hide with the mouse. 
> If you need to hide more than one element, try pressing W. 
> Pressing N will make the selection smaller again. 

Adding this feature to "Pick an element from the page" of Inspector
would be very useful.
Even better, add all the hotkeys/commands that the addon offers (https://adblockplus.org/en/images/ehh-select.png)

	Patrick Brosset <:pbro>
	Comment 1 • 10 years ago

If by wider you mean selecting the parent node, and narrower selecting the (first) child, then I agree this is very useful. In fact I remember this was a feature of the Ardvark add-on, back in the days even before Firebug.
It could be very useful when a child element has the same position/size than its parent, then it's impossible to select the parent with the picker. This would make it possible.

Thanks for filing.

	Kostas Reporter
	Comment 2 • 10 years ago

Yes, I mean exactly that. Glad to be of help.

	Patrick Brosset <:pbro>
	Comment 3 • 10 years ago

Alright cool!
Flagging this as "good first bug"-ish, and marking myself as mentor. It's not too hard and can be done by anyone who's interested and have some prior JS knowledge.

	Patrick Brosset <:pbro>
	Comment 4 • 10 years ago

Some pointers to get started:

- The file that will need to be changed is /toolkit/devtools/server/actors/highlighter.js (http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js)

- The first step will be adding key listeners (so that we know when W and H are pressed, or any other key we end up choosing for this). This can be done in this function for instance: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js#267

- Then an event handler function needs to be assigned to this listener. This function will need to find the right node, starting from the node currently being hovered (either its parent, or child). Here's how hovered nodes get highlighted and sent to the toolbox: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js#224
So we'll need to change this function to store the currently hovered node somewhere on the instance. Then when the right key is pressed, use this node to get to the right node.
With the right node located, we then need to call the |this._walker.attachElement(node);| part of |_findAndAttachElement| and then emit the "picker-node-hovered" event.

- We also need to change the |_onPick| handler (http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js#212) because the click won't go to the right node once we've pressed, say, W to select the parent.

I guess this is enough to get started. Feel free to join the IRC channel #devtools for more info, or NeedInfo? me on this bug.

	Prathik Sai [:Skandan]
	Comment 5 • 10 years ago

I'm still not very good at JS, but I think I can give this a fair shot. Will begin working on it.

	Prathik Sai [:Skandan]
	Comment 6 • 10 years ago

I have a few questions, with respect to finding the current node being hovered, doesn't this_hoveredNode variable contain exactly that?
(http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js#230)
If yes,
I suppose using _hoveredNode's node properties we can widen and narrow by calling the |this._walker.attachElement(node)| with node as _hoveredNode.firstChild or _hoveredNode.parentNode
by checking event.keyCode for the values of N and W.

This seems very basic, but what does this " event => {" do, specifically the "=>"? (Sorry about this question, but I had to know, couldn't find help anywhere online!)

Thanks in advance! :)

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 7 • 10 years ago

As [:pbrosset] said.
1. + "key*" Event Listeners
2. + |_onKey| handler with simple W-A-S-D implementation
3. ? |_onPick| modified to handle all requests.

Sorry [:Skandan] if I overstepped. But I could barely stop myself.

	Patrick Brosset <:pbro>
	Comment 8 • 10 years ago

(In reply to Prathik Sai  [:Skandan] from comment #6)
> This seems very basic, but what does this " event => {" do, specifically the
> "=>"? (Sorry about this question, but I had to know, couldn't find help
> anywhere online!)
It's an arrow function!
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

(In reply to Prathik Sai  [:Skandan] from comment #6)
> I have a few questions, with respect to finding the current node being
> hovered, doesn't this_hoveredNode variable contain exactly that?
> (http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/
> actors/highlighter.js#230)
Yeah I think you're right, we should be able to reuse _hoveredNode for this.

It looks like someone bit you to submitting a patch though :) (don't worry, we have plenty other bugs in case you want me to provide others).

(In reply to babhishek21 from comment #7)
> Created attachment 8548196 [details] [diff] [review]
> bug_1120111: + "key*" EventListener; + |_onKey| handler; Changed |_onPick|
I'll review your patch soon(-ish). Thanks.

	Patrick Brosset <:pbro>
	Comment 9 • 10 years ago

Comment on attachment 8548196 [details] [diff] [review]
bug_1120111: + "key*" EventListener; + |_onKey| handler; Changed |_onPick|

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

Thanks for this patch, this is the right general approach.

However, you haven't tested this patch locally ... if you had, you'd have seen it doesn't work at all, there are exceptions.
Please make sure you make your changes locally, then re-build, then run firefox and manually test that everything works correctly.
Make sure you read through this guide to hacking the devtools: https://wiki.mozilla.org/DevTools/Hacking

I really like the W/A/S/D idea, this would appeal to gamers :) However, I see one problem with this: not all keyboards have the QWERTY layout, and so WASD will not be in a logical geographical group on all keyboards (for instance for french keyboards).
So I would prefer if we stuck to choosing keys that are the first character of the action:
- W to Widen the selection = select the parent node
- N to Narrow the selection = select the child (I like how this is the same than the old aardvark addon: https://addons.mozilla.org/fr/firefox/addon/aardvark/)
Of course, this makes it hard to choose a key for Next sibling ... 

As I commented below, we also need to check whether the current node has children, parentNode, previous or next siblings before trying to select any of them. If not, we should just early return from the onKey function and not do anything (currentNode should remain unchanged).

Finally, we need new test(s) for this feature.
I'll comment on how to do this later, when you've corrected this patch.

::: toolkit/devtools/server/actors/highlighter.js
@@ +196,5 @@
>     * Once a node is picked, events will cease, and listeners will be removed.
>     */
>    _isPicking: false,
>    _hoveredNode: null,
> +  _currentElement: null,

For better consistency with _hoveredNode, please rename to _currentNode.

@@ +225,5 @@
>      this._onHovered = event => {
>        this._preventContentEvent(event);
> +      this._currentElement = this._findAndAttachElement(event);
> +      if (this._hoveredNode !== this._currentElement.node) {
> +        this._highlighter.show( this._currentElement.node.rawNode);

nit: remove the empty space after (

@@ +233,5 @@
>      };
>  
> +    this._onKey = event => {
> +      this._preventContentEvent(event);
> +      // Store current hovered element node and current element OwnerDocument

This comment should be removed.

@@ +234,5 @@
>  
> +    this._onKey = event => {
> +      this._preventContentEvent(event);
> +      // Store current hovered element node and current element OwnerDocument
> +      let cur = this._hoveredNode; 

nit: trailing whitespace.
nit: please rename to something more self-explanatory, like currentNode.

Also, why using this._hoveredNode here instead of this._currentElement? If you hover a node and then try pressing W several times in a row, we need the ancestors to be selected one by one.

@@ +239,5 @@
> +      let doc = event.target.ownerDocument;
> +
> +      switch(event.keyCode) {
> +        case 87: // 'w' for parent
> +          cur = doc.cur.parentNode;

We need to check if there is a parentNode to select at all.
Also, you should use parentElement instead, to make sure only element nodes are selected.

Also, doc.cur.parentNode isn't going to work, it's going to throw an exception.

You need to get the actual node, for instance:
let node = this._currentNode.node.rawNode;
and from that:
node.parentElement;

Same remark for other cases below.

@@ +243,5 @@
> +          cur = doc.cur.parentNode;
> +          break;
> +
> +        case 83: // 's' for first child
> +          cur = doc.cur.firstChild;

I don't think firstChild is the right solution in fact. Imagine the use case where you hover over a node that has previous siblings, and then press W. Then after this, you press S. You'd expect the same node you originally hovered to be selected again. Instead, the previous sibling of the node will be selected.

So we need to select whatever child leads to this._hoveredNode.
But when currentNode is hoveredNode, then we should probably select firstChild instead.

@@ +247,5 @@
> +          cur = doc.cur.firstChild;
> +          break;
> +
> +        case 65: // 'a' for previous sibling
> +          cur = doc.cur.previousSibling;

We need to check if there is a previoussibling node to select.
And you should use previousElementSibling instead, we don't want to select non element nodes.

Same remarks for nextsibling below.

@@ +259,5 @@
> +      }
> +
> +      // Store currently attached element
> +      this._currentElement = this._walker.attachElement(cur);
> +      this._highlighter.show( this._currentElement.node.rawNode);

nit: remove the whitespace after (

	Patrick Brosset <:pbro>
	Comment 10 • 10 years ago

I don't usually do this, but while working on the review, I did the changes locally to play with the feature and confirm its usefulness. Turns out is really useful! It's a nice way to traverse the DOM visually.

And since I was working on these changes, I thought I'd attach them here.
Make sure your patch is attached first, and then attach this one. This should take care of the my earlier comments.

One more comment about your patch: the commit message should be formatted like this:

Bug 1120111 - <description of what you did>; r=<nick of the reviewer>

So in this case, you should change the commit message to be something like this:

Bug 1120111 - Added key listeners while in highlighter pick mode to walk the DOM (parent, child, siblings); r=pbrosset

Finally, we miss new test(s) for this feature.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 11 • 10 years ago

Hello [:patrick],
Your review scared the bejeebers out of me (This is my second bug :P) 
Thank you A LOT for the corrections patch. That saves me loads of time.

I am sorry I did not test the patch. The idea was to land a patch to check if I even got the concept right (of what was asked.)

> I really like the W/A/S/D idea, this would appeal to gamers :) However, I see one problem with this: 
> not all keyboards have the QWERTY layout, and so WASD will not be in a logical geographical group
> on all keyboards (for instance for french keyboards).
It would be disastrous on a Dvorak.
> So I would prefer if we stuck to choosing keys that are the first character of the action:
> - W to Widen the selection = select the parent node
> - N to Narrow the selection = select the child (I like how this is the same than the old aardvar
> addon: https://addons.mozilla.org/fr/firefox/addon/aardvark/)
> Of course, this makes it hard to choose a key for Next sibling ... 
That's true. But I still think next/previous sibling should have separate hotkeys. That makes way for much less messier code and much better UX.

> As I commented below, we also need to check whether the current node has children, 
> parentNode, previous or next siblings before trying to select any of them.
> If not, we should just early return from the onKey function and
> not do anything (currentNode should remain unchanged).
Hmm. I assumed that if there is no node available, and currentNode now points to null, the |onHover| would just attach the nearest element to _correntElement. I used .parentNode and derivatives simply because they throw exceptions that can be handled. In fact [1] tells me to trust .parentNode over .parentElement.

> Also, doc.cur.parentNode isn't going to work, it's going to throw an exception.
> You need to get the actual node, for instance:
> let node = this._currentNode.node.rawNode;
> and from that:
> node.parentElement;
I don't understand this. Isn't cur = _hoveredNode making sure that I get a rawNode and not an element?
Sorry if I sound dorky. Its just that I've never worked with HTML DOM before. Is there any good resource for some quick DOM documentation?

> I don't think firstChild is the right solution in fact. Imagine the use case where you hover over a 
> node that has previous siblings, and then press W. Then after this, you press S. You'd expect the same 
> node you originally hovered to be selected again. Instead, the previous sibling of the node will be selected.
> So we need to select whatever child leads to this._hoveredNode.
> But when currentNode is hoveredNode, then we should probably select firstChild instead.
This sounds scary to me. While it may be a good idea to remember the childNode (and this is easily implemented), it can also defeat the purpose of previousSibling and nextSibling. Imagine browsing through your Linux directory. You expect to land on the first child when moving into a parent directory. Isn't that good programmer sense? Also wouldn't going back to previously selected childNode throw some people off, when they are actually expecting to move linearly through elements? (This is all IMHO. Don't mean to be rude.)

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
One quick question: I am using mercurial queues to make patches. I don't remove applied patches (meaning the patch I submitted is already applied and LIVE in the queue). I was wondering if simply importing you patch ( `qimport` ) and applying it on top of it is ok?
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[1] http://www.w3schools.com/jsref/prop_node_parentelement.asp

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 12 • 10 years ago

Hello [:patrick],
Apropos comment#11,
I stumbled upon this idea: Why not navigate by Elements only? 
*** We are checking for existence of Elements for a particular node before attaching it anyway. Ref:[1] ***
So we can simply use element.firstElementChild, element.firstElementChild, element.parentElement, element.nextElementSibling etc.

However this suffers from a major fault: We can only choose element nodes in this fashion. All `text` and `comment` nodes are ignored. This is a MAJOR drawback. Imagine this:
> <div id="textContainer">This is a simple text in the container.</div>
being passed over (because it's a text node) in our checks. Woahh. Big problem.

Now this is where it gets paradoxical: Your test |if (!currentNode.parentElement)|(REF: [1]) actually ensures that a Node is stored only if it is an ElementNode. So even though we are traversing using nodes (and not elements), we are potentially ignoring `text` and `comment` nodes (and reach the above problem)

So what should I do?

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[1] https://bugzilla.mozilla.org/attachment.cgi?id=8548909&action=diff#a/toolkit/devtools/server/actors/highlighter.js_sec3 Line 241

	Patrick Brosset <:pbro>
	Comment 13 • 10 years ago

(In reply to Abhishek Bhattacharya from comment #11)
> > So I would prefer if we stuck to choosing keys that are the first character of the action:
> > - W to Widen the selection = select the parent node
> > - N to Narrow the selection = select the child (I like how this is the same than the old aardvar
> > addon: https://addons.mozilla.org/fr/firefox/addon/aardvark/)
> > Of course, this makes it hard to choose a key for Next sibling ... 
> That's true. But I still think next/previous sibling should have separate
> hotkeys. That makes way for much less messier code and much better UX.
Yes I agree we should keep next/previous sibling, we just need to find the right key to attach them to.
I suggest:
- W to go up to the parent (Widen the selection)
- N to go down to the child (Narrow the selection)
- P for Previous sibling 
- F for next sibling (Following sibling). Not great, but we can't use N for Next since we already have N for Narrow.
I'm open to other ideas.

> > As I commented below, we also need to check whether the current node has children, 
> > parentNode, previous or next siblings before trying to select any of them.
> > If not, we should just early return from the onKey function and
> > not do anything (currentNode should remain unchanged).
> Hmm. I assumed that if there is no node available, and currentNode now
> points to null, the |onHover| would just attach the nearest element to
> _correntElement. I used .parentNode and derivatives simply because they
> throw exceptions that can be handled. In fact [1] tells me to trust
> .parentNode over .parentElement.
Yeah setting this._currentElement to null isn't a problem, but emitting the "picker-node-hovered" event in this case is a problem. That's why, in _onKey, we need to check if the element we want to navigate to actually exist.

> > Also, doc.cur.parentNode isn't going to work, it's going to throw an exception.
> > You need to get the actual node, for instance:
> > let node = this._currentNode.node.rawNode;
> > and from that:
> > node.parentElement;
> I don't understand this. Isn't cur = _hoveredNode making sure that I get a
> rawNode and not an element?
What I'm saying is that |doc.cur.parentNode| is going throw, |cur| is not a property of the |doc| object, so |doc.cur| returns undefined, and so |doc.cur.parentNode| will fail.
And |_hoveredNode| is not a raw DOM node, it's an object that has the |rawNode| property on it
> Sorry if I sound dorky. Its just that I've never worked with HTML DOM
> before. Is there any good resource for some quick DOM documentation?
This isn't really DOM related, it's just how you access properties of objects in JS. But if you're looking for DOM documentation, I suggest MDN: https://developer.mozilla.org/en-US/docs/Web/API/Document_Object_Model

> > I don't think firstChild is the right solution in fact. Imagine the use case where you hover over a 
> > node that has previous siblings, and then press W. Then after this, you press S. You'd expect the same 
> > node you originally hovered to be selected again. Instead, the previous sibling of the node will be selected.
> > So we need to select whatever child leads to this._hoveredNode.
> > But when currentNode is hoveredNode, then we should probably select firstChild instead.
> This sounds scary to me. While it may be a good idea to remember the
> childNode (and this is easily implemented), it can also defeat the purpose
> of previousSibling and nextSibling. Imagine browsing through your Linux
> directory. You expect to land on the first child when moving into a parent
> directory. Isn't that good programmer sense? Also wouldn't going back to
> previously selected childNode throw some people off, when they are actually
> expecting to move linearly through elements? (This is all IMHO. Don't mean
> to be rude.)
So, I think both arguments can be made. I understand your point of always selecting the first child because that's more predictable. But my point is that if you start at a node, then go up to the parent then down again, I'm pretty sure you'd expect the same node to be selected again. Yes this is different from, say, a file system, but this tool is to select elements in the page, not really navigate the DOM tree as a file system. The main point of the tool is to be a visual aid for people to find elements on the page, by looking at the page, not so much at the DOM tree representation in the toolbox. So I'm thinking it makes more sense to remember the previously selected node.
Let's ask for more feedback from other people.

> One quick question: I am using mercurial queues to make patches. I don't
> remove applied patches (meaning the patch I submitted is already applied and
> LIVE in the queue). I was wondering if simply importing you patch (
> `qimport` ) and applying it on top of it is ok?
Yeah, no problem there.

(In reply to Abhishek Bhattacharya from comment #12)
> Hello [:patrick],
> Apropos comment#11,
> I stumbled upon this idea: Why not navigate by Elements only? 
> *** We are checking for existence of Elements for a particular node before
> attaching it anyway. Ref:[1] ***
> So we can simply use element.firstElementChild, element.firstElementChild,
> element.parentElement, element.nextElementSibling etc.
Yes navigating by elements only was what I had in mind too. That's why I've been using parentElement/firstElementChild/... in my patch.

> However this suffers from a major fault: We can only choose element nodes in
> this fashion. All `text` and `comment` nodes are ignored. This is a MAJOR
> drawback. Imagine this:
> > <div id="textContainer">This is a simple text in the container.</div>
> being passed over (because it's a text node) in our checks. Woahh. Big
> problem.
> 
> Now this is where it gets paradoxical: Your test |if
> (!currentNode.parentElement)|(REF: [1]) actually ensures that a Node is
> stored only if it is an ElementNode. So even though we are traversing using
> nodes (and not elements), we are potentially ignoring `text` and `comment`
> nodes (and reach the above problem)
We can only select elements today with the picker anyway, so I don't this as a big limitation. Again, these key shortcuts aren't supposed to be a DOM navigation tool, but a visual way to expand/contract the current selection to help with selecting parents or child that have the same geometry as the current element. That's 99% of the use cases we try to cover here.
So let's navigate by elements only.

	Patrick Brosset <:pbro>
	Comment 14 • 10 years ago

Hey Brian, what's your take on this question:

> I don't think firstChild is the right solution in fact. Imagine the use case where you hover over a 
> node that has previous siblings, and then press W. Then after this, you press S. You'd expect the same 
> node you originally hovered to be selected again. Instead, the previous sibling of the node will be selected.
> So we need to select whatever child leads to this._hoveredNode.
> But when currentNode is hoveredNode, then we should probably select firstChild instead.
This sounds scary to me. While it may be a good idea to remember the childNode (and this is easily implemented), it can also defeat the purpose of previousSibling and nextSibling. Imagine browsing through your Linux directory. You expect to land on the first child when moving into a parent directory. Isn't that good programmer sense? Also wouldn't going back to previously selected childNode throw some people off, when they are actually expecting to move linearly through elements? (This is all IMHO. Don't mean to be rude.)

	Brian Grinstead [:bgrins]
	Comment 15 • 10 years ago

If you think sibling traversal is important, then I'd suggest you consider replacing all the keys with the arrow keys:

Left: go to parent
Right: go to child (last selected, or firstChild if none)
Up: previous sibling
Down: next sibling

This would have the benefit of more closely matching the markup view shortcuts, and being less things for a user to remember / easier to guess.

Regarding if the N shortcut should 'remember' the selected element - I think it definitely should.  The use case is that you want to select a parent element that is difficult to get to with the mouse.  So you keep going up one level until you go too far, then you go back one.

So given this DOM:

<parent>
  <child1 />
  <child2>
    <subchild1 />
    <subchild2 />
  </child2>
</parent>

If I select subchild2 with the mouse, but I really want whichever parent element that has the same dimensions (happens to be child2, but I don't know that yet).  Then I would press W -> child2, W -> parent (oops, too far), N -> child2.

	Brian Grinstead [:bgrins]
	Comment 16 • 10 years ago

Or you could not worry about next/previous sibling for now.  This behavior is handled with the same semantics in the markup view with the up/down arrows once the element is picked.

I just filed Bug 1122605 for a papercut I just noticed in which the markup view isn't focused once an element is picked.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 17 • 10 years ago

Hey Brian, 
Thanks for the reply. I understand your mentality for using arrow keys is the same as that already used for navigating through the markup in code inspector. Am I right? AFAIK arrow keys do NOT have any ASCII codes assigned to them. I just hope that does not create any problems later.

I will follow the scheme where last childNode is remembered. 

Apart from your arrow key suggestions, I researched some keyboard layouts and it seems the following layout holds pretty good: (keyCodes in parentheses)
w : wider or parent (87)
n : narrower or child (78) //remembers previous child
f : previous sibling (70)
g : next sibling (71)
h : help or show modal (72)

Most keyboards layouts (of all that I have seen) will probably have at least "f", "g" and "h" keys in direct sequence together (horizontal or skew vertical)

The "h" is to help show a modal which lists these keyboard hotkeys. (I have left a blank case for now)

So what do you think?

	Brian Grinstead [:bgrins]
	Comment 18 • 10 years ago

(In reply to Abhishek Bhattacharya from comment #17)
> Hey Brian, 
> Thanks for the reply. I understand your mentality for using arrow keys is
> the same as that already used for navigating through the markup in code
> inspector. Am I right? AFAIK arrow keys do NOT have any ASCII codes assigned
> to them. I just hope that does not create any problems later.
> 

Yes, that's what I was intending.  You could check e.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_LEFT, Ci.nsIDOMKeyEvent.DOM_VK_RIGHT, Ci.nsIDOMKeyEvent.DOM_VK_UP, Ci.nsIDOMKeyEvent.DOM_VK_DOWN as we do in the markup view: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#447.  These are mapping to the numbers 37-40.

> I will follow the scheme where last childNode is remembered. 
> 
> Apart from your arrow key suggestions, I researched some keyboard layouts
> and it seems the following layout holds pretty good: (keyCodes in
> parentheses)
> w : wider or parent (87)
> n : narrower or child (78) //remembers previous child
> f : previous sibling (70)
> g : next sibling (71)
> h : help or show modal (72)
> 
> Most keyboards layouts (of all that I have seen) will probably have at least
> "f", "g" and "h" keys in direct sequence together (horizontal or skew
> vertical)
> 

The arrow keys are just an idea, but the benefit is that we hopefully wouldn't need to have extra 'help' information, since I'm hoping users would be able to generally guess what they do (or experiment with them until they figured it out).  Patrick, what are you thinking?

> The "h" is to help show a modal which lists these keyboard hotkeys. (I have
> left a blank case for now)
> 

We should implement a help shortcut like this at the toolbox level (I've seen "?" as a common way to pull up keyboard shortcuts).  Let's file a new bug for that, but I think it's a very good idea.

	Brian Grinstead [:bgrins]
	Comment 19 • 10 years ago

(In reply to Brian Grinstead [:bgrins] from comment #18)
> > The "h" is to help show a modal which lists these keyboard hotkeys. (I have
> > left a blank case for now)
> > 
> 
> We should implement a help shortcut like this at the toolbox level (I've
> seen "?" as a common way to pull up keyboard shortcuts).  Let's file a new
> bug for that, but I think it's a very good idea.

Filed Bug 1122699

	Patrick Brosset <:pbro>
	Comment 20 • 10 years ago

(In reply to Brian Grinstead [:bgrins] from comment #18)
> (In reply to Abhishek Bhattacharya from comment #17)
> > Hey Brian, 
> > Thanks for the reply. I understand your mentality for using arrow keys is
> > the same as that already used for navigating through the markup in code
> > inspector. Am I right? AFAIK arrow keys do NOT have any ASCII codes assigned
> > to them. I just hope that does not create any problems later.
> > 
> 
> Yes, that's what I was intending.  You could check e.keyCode ==
> Ci.nsIDOMKeyEvent.DOM_VK_LEFT, Ci.nsIDOMKeyEvent.DOM_VK_RIGHT,
> Ci.nsIDOMKeyEvent.DOM_VK_UP, Ci.nsIDOMKeyEvent.DOM_VK_DOWN as we do in the
> markup view:
> https://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/
> markup-view.js#447.  These are mapping to the numbers 37-40.
I like that, let's go with the arrow keys!

> > I will follow the scheme where last childNode is remembered. 
> > 
> > Apart from your arrow key suggestions, I researched some keyboard layouts
> > and it seems the following layout holds pretty good: (keyCodes in
> > parentheses)
> > w : wider or parent (87)
> > n : narrower or child (78) //remembers previous child
> > f : previous sibling (70)
> > g : next sibling (71)
> > h : help or show modal (72)
> > 
> > Most keyboards layouts (of all that I have seen) will probably have at least
> > "f", "g" and "h" keys in direct sequence together (horizontal or skew
> > vertical)
> > 
> 
> The arrow keys are just an idea, but the benefit is that we hopefully
> wouldn't need to have extra 'help' information, since I'm hoping users would
> be able to generally guess what they do (or experiment with them until they
> figured it out).  Patrick, what are you thinking?
Agreed, the arrow keys feel more logical, no need for any help screen, it's more discoverable.

> > The "h" is to help show a modal which lists these keyboard hotkeys. (I have
> > left a blank case for now)
> > 
> 
> We should implement a help shortcut like this at the toolbox level (I've
> seen "?" as a common way to pull up keyboard shortcuts).  Let's file a new
> bug for that, but I think it's a very good idea.
Thanks for filing that bug, indeed an excellent idea.

Also:
> Regarding if the N shortcut should 'remember' the selected element - I think
> it definitely should.  The use case is that you want to select a parent
> element that is difficult to get to with the mouse.  So you keep going up
> one level until you go too far, then you go back one.
> 
> So given this DOM:
> 
> <parent>
>   <child1 />
>   <child2>
>     <subchild1 />
>     <subchild2 />
>   </child2>
> </parent>
> 
> If I select subchild2 with the mouse, but I really want whichever parent
> element that has the same dimensions (happens to be child2, but I don't know
> that yet).  Then I would press W -> child2, W -> parent (oops, too far), N
> -> child2.
Yes! Exactly my thoughts. If we implement this, we cover the main feature I intend this to be used for.
So, I'd even be happy if we only shipped parent/child traversal and let go of sibling navigation for now.

Abhishek: do you think you could provide a patch with only parent/child traversal using arrow keys for now, so that we start simple, and we can later provide a separate patch if we deem that more traversal options are necessary in pick mode.
I'd rather we start simple, and I think that's a good enough feature to ship it like this.
Thanks!

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 21 • 10 years ago

(In reply to Brian Grinstead [:bgrins] from comment #18)
> Abhishek: do you think you could provide a patch with only parent/child traversal using arrow keys 
> for now, so that we start simple, and we can later provide a separate patch if we deem that more
> traversal options are necessary in pick mode.
> I'd rather we start simple, and I think that's a good enough feature to ship it like this.
> Thanks!

Sure! So just to be sure, you want:
1. parent / child traversal using arrowUP, arrowDown, arrowLeft and arrowRight.
2. NO help/showModal key for now. (Bug 1122699 will probably take care of that.)

If you are asking for is the above, then I'm already done with the patch. All that is left is to test the patch with a build. (It brings my uber-awesome gaming rig to a halt!). Will try and upload the patch within a few hours.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 22 • 10 years ago

So this patch works perfectly. I've tested it on a debug build. The arrow keys is not intuitive straight-away. But looking at the markup view helps understand the selection process. So all's good.

BUT I've discovered one HUGE problem. We haven't assigned a key to "Pick" the element. As of now, it works only by onClick(). The problem is, if a user accidently moves his mouse pointer (even a little bit) while clicking, a different element could end up being "picked". In fact, this happened with me in about 80% of my test cases. 

Hence we really require a "selection/pick" key (I was thinking the "Enter" or "carriageReturn" key could be utilised for this purpose.

What do you say Patrick and Brian?

	Patrick Brosset <:pbro>
	Comment 23 • 10 years ago

(In reply to Abhishek Bhattacharya from comment #22)
> BUT I've discovered one HUGE problem. We haven't assigned a key to "Pick"
> the element. As of now, it works only by onClick(). The problem is, if a
> user accidently moves his mouse pointer (even a little bit) while clicking,
> a different element could end up being "picked". In fact, this happened with
> me in about 80% of my test cases. 
> 
> Hence we really require a "selection/pick" key (I was thinking the "Enter"
> or "carriageReturn" key could be utilised for this purpose.
> 
> What do you say Patrick and Brian?
Very good point. In fact, Brian had filed a bug some time ago about having "Esc" stop the picker: bug 959632. So having "Enter" pick the current element (without having to click) would be consistent with this *and* help solve the problem you mentioned.
If you're ok adding both Enter and Esc to your patch, I'm happy to review all at the same time. We could add Enter/Esc as a separate patch or bug if you preferred though. Up to you.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 24 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #23)
> (In reply to Abhishek Bhattacharya from comment #22)
> > BUT I've discovered one HUGE problem. We haven't assigned a key to "Pick"
> > the element. As of now, it works only by onClick(). The problem is, if a
> > user accidently moves his mouse pointer (even a little bit) while clicking,
> > a different element could end up being "picked". In fact, this happened with
> > me in about 80% of my test cases. 
> > 
> > Hence we really require a "selection/pick" key (I was thinking the "Enter"
> > or "carriageReturn" key could be utilised for this purpose.
> > 
> > What do you say Patrick and Brian?
> Very good point. In fact, Brian had filed a bug some time ago about having
> "Esc" stop the picker: bug 959632. So having "Enter" pick the current
> element (without having to click) would be consistent with this *and* help
> solve the problem you mentioned.
> If you're ok adding both Enter and Esc to your patch, I'm happy to review
> all at the same time. We could add Enter/Esc as a separate patch or bug if
> you preferred though. Up to you.

I don't think a separate bug is required. Unnecessary duplication is not good IMHO. Ok I'll add Enter and Esc cases:
1. Enter: pick element
2. Esc: exit from "element selector" with immediate effect. Nothing is picked. 
I haven't figured out how I'll do these. Can I have a week's time for complete testing and patching? (My college will ease up on the heat by this week)

	Patrick Brosset <:pbro>
	Comment 25 • 10 years ago

(In reply to Abhishek Bhattacharya from comment #24)
> I haven't figured out how I'll do these. Can I have a week's time for
> complete testing and patching? (My college will ease up on the heat by this
> week)

Take as much time as you need. Here are some pointers:

- Since you already added the _onKeyDown handler, you'll just have to add 2 new cases in it, one for Esc, the other for Enter.

- On Enter: just emit the "picker-node-picked" event with the right currentElement, like _onPick does.

- On Esc: you'll have to call cancelPick to stop getting called when mouse/keyboard events happen on the page and, you'll have to let the UI know that the picker has stopped. We currently don't have anything that does this so I believe we need to introduce a new event, maybe called "picker-canceled". You should send it like the others are sent:
events.emit(this._walker, "picker-canceled");

Now, the UI should listen to this event, look in toolbox-highlighter-utils.js: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox-highlighter-utils.js#131
This is where the UI listens to the "picked" and "hovered" events. So add a new listener to the "canceled" event here that executes stopPicker.

	Patrick Brosset <:pbro>
	Comment 26 • 10 years ago

Comment on attachment 8550700 [details] [diff] [review]
bug_1120111: rev_1: Better keyboard layout.  Incorporated corrections.

Clearing the review flag for now.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 27 • 10 years ago

Hey Patrick,
So I read your comment a bit late.(I was already done with the patch and build-testing) Anyway, an overview of changes:
1. Added "Enter" as an alternative way to select element.
2. Added "Esc" as a way to cancel "Pick an element" process.

Regarding change 2, I actually emitted the "picker-node-picked" message along with a "null" element. i.e. I'm exiting with "SUCCESS" even though I cancelled a pick.(Again, I read your comment too late)

I've tested it, and don't seem to face any errors. Feel free to test it on your own. If you still think that a separate "picker-canceled" event is required, I'll look in that direction.( I haven't had the time yet to look into UI handler file)

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 28 • 10 years ago

Is changing status to ASSIGNED alright?

	Patrick Brosset <:pbro>
	Comment 30 • 10 years ago

Comment on attachment 8551214 [details] [diff] [review]
bug_1120111: rev_2: Added "Enter" and "Esc" sequences

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

So, this patch looks good to me, but I have 2 remarks:

- I thought we agreed leaving prev/next sibling out of the patch for now and only have ways to navigate to the parent and child?
- Sending the picker-node-picked was also my first idea instead of introducing another event, but I see one problem with it: when the event is received on the front-end, we use it to set the new selection. This means that with your change, if the user had previously select a DIV element, then starts the picker, then presses ESCape, then no nodes will be selected anymore, i.e. the DIV element will be unselected. This isn't the best behavior I think. We need to make sure the originally selected element remains selected. And so for this, I believe introducing a new event is best.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 31 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #30)
> Comment on attachment 8551214 [details] [diff] [review]
> bug_1120111: rev_2: Added "Enter" and "Esc" sequences
> 
> Review of attachment 8551214 [details] [diff] [review]:
> -----------------------------------------------------------------
> So, this patch looks good to me, but I have 2 remarks:
> 
> - I thought we agreed leaving prev/next sibling out of the patch for now and
> only have ways to navigate to the parent and child?
Eh? I thought we were following what Brian said. :P. 
When you said the following in comment #20, I assumed all four arrow keys.
 
> Abhishek: do you think you could provide a patch with only parent/child traversal using arrow keys 
> for now, so that we start simple, and we can later provide a separate patch if we deem that 
> more traversal options are necessary in pick mode.
Aside from the misunderstanding, does it require removal? It seemed to work fine.

> - Sending the picker-node-picked was also my first idea instead of
> introducing another event, but I see one problem with it: when the event is
> received on the front-end, we use it to set the new selection. This means
> that with your change, if the user had previously select a DIV element, then
> starts the picker, then presses ESCape, then no nodes will be selected
> anymore, i.e. the DIV element will be unselected. This isn't the best
> behavior I think. We need to make sure the originally selected element
> remains selected. And so for this, I believe introducing a new event is best.

I had not thought on those lines. It prompted me to run my build again. Yes, you are right. It deselects any previous selections made. Now I'll leave two choices to you (I apologize for being a bad Mentee here):
1. If you are just looking to solve the aforesaid problem, then I can simply emit "picker-node-picked" with _currentNode (without resetting _currentNode = null). That would ensure that the previously selected element remains selected.  
2. If you want a new event like "picker-node-canceled" or "picker-canceled" for Q&A purposes, then I'll have to look into the UI framework file you mentioned in comment #25.

Wotsay?

	Patrick Brosset <:pbro>
	Comment 32 • 10 years ago

(In reply to Abhishek Bhattacharya from comment #31)
> > - I thought we agreed leaving prev/next sibling out of the patch for now and
> > only have ways to navigate to the parent and child?
> Eh? I thought we were following what Brian said. :P. 
> When you said the following in comment #20, I assumed all four arrow keys.

Oh ok, sorry for the misunderstanding. Given the comments below, I thought we agreed to have only parent/child traversal, using arrow keys:

(In reply to Brian Grinstead [:bgrins] from comment #16)
> Or you could not worry about next/previous sibling for now.  This behavior
> is handled with the same semantics in the markup view with the up/down
> arrows once the element is picked.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #20)
> Abhishek: do you think you could provide a patch with only parent/child
> traversal using arrow keys for now, so that we start simple, and we can
> later provide a separate patch if we deem that more traversal options are
> necessary in pick mode.
> I'd rather we start simple, and I think that's a good enough feature to ship
> it like this.
> Thanks!

So, yes, I do think we should only have parent/child traversal (using left/right arrows), and not siblings for now.
Let's worry about them later if we need to.

> I had not thought on those lines. It prompted me to run my build again. Yes,
> you are right. It deselects any previous selections made. Now I'll leave two
> choices to you (I apologize for being a bad Mentee here):
> 1. If you are just looking to solve the aforesaid problem, then I can simply
> emit "picker-node-picked" with _currentNode (without resetting _currentNode
> = null). That would ensure that the previously selected element remains
> selected.
Hmm, that wouldn't work, would it. currentNode isn't the same as the node that was selected in the inspector before the user clicked on the picker icon, right? So emitting the "picker-node-picked" with currentNode would change the selection even though the user has pressed ESC. I think this would be confusing, because as long as you haven't either clicked on a node or pressed ENTER, the highlighter shouldn't change the current selection.
> 2. If you want a new event like "picker-node-canceled" or "picker-canceled"
> for Q&A purposes, then I'll have to look into the UI framework file you
> mentioned in comment #25.
Yes, I think that's what we need. Let me know of any questions you have regarding the UI, there shouldn't be a lot of changes to do.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 33 • 10 years ago

Patrick,
Sorry for not responding. I am a bit hung up with Bug 1123309. I plan to look into the UI framework and patch up accordingly in about 3 days. Will that be alright? If you require immediate and urgent response, do tell.

I also have a small question: Since we are adding a new feature which has significant UX additions, do you think that a consultation with the UX team (if there exists one) is required? (We wouldn't want Q&A to turn it down, if I missed some specification or rule)

	Brian Grinstead [:bgrins]
	Comment 34 • 10 years ago

Let's continue in the direction you are heading.  Once the patch is updated with all the keyboard shortcuts working, we can try it out ourselves to see if it seems generally good, and then request some UX help to evaluate also.  At this point, I think it'd be easier to discuss the feature with UX once we have a build to play with.

	Patrick Brosset <:pbro>
	Comment 35 • 10 years ago

(In reply to Brian Grinstead [:bgrins] from comment #34)
> Let's continue in the direction you are heading.  Once the patch is updated
> with all the keyboard shortcuts working, we can try it out ourselves to see
> if it seems generally good, and then request some UX help to evaluate also. 
> At this point, I think it'd be easier to discuss the feature with UX once we
> have a build to play with.
Yep, that works for me.

(In reply to Abhishek Bhattacharya from comment #33)
> Patrick,
> Sorry for not responding. I am a bit hung up with Bug 1123309. I plan to
> look into the UI framework and patch up accordingly in about 3 days. Will
> that be alright? If you require immediate and urgent response, do tell.
Don't worry about it, take all the time you need.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 36 • 10 years ago

Sorry for not having posted any update for a long time. So I have started looking at the file mentioned in comment #25. I hope to have a good patch by tomorrow.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 37 • 10 years ago

So a patch after a long time.

THE GOOD:
1. Esc sequence has its very own emit message "picker-node-canceled" and handler |onPickerCanceled| @browser/devtools/framework/toolbox-highlighter-utils.js
2. Esc sequence works well and no resetting of _currentNode.
3. Build works excellent.

THE BAD:
1. The tiny little blue "pickerButton" is giving me a LOT of trouble. After Esc, it JUST WON'T TURN BACK to black! For essentially the same function as |onPickerNodePicked| (where it successfully turns black), |onPickerCanceled| is giving weird behaviour. I even looked through ALL devtools/framework files just in case my custom |onPickerCanceled| or "picker-node-canceled" message were creating trouble. But I couldn't find any problems there. Somehow:
	1. The toolbox.selection.setNodeFront() is not working all right with my "reason" and hence the stopPicker() is not being called.  ~~~~OR~~~~
	2. The stopPicker() is doing an early return because "somehow" |isPicking| === `false` ~~OR~~
	3. Something else, entirely!

THE UGLY:
1. I removed the previous/next sibling functionality as you asked, Patrick. And it does not feel good. Since I got used to playing around with the arrow keys (like in "Markup View Mode"), no sibling traversal is frustrating. Worse, now every time I have to choose a sibling of the same parent, I have to move my hand from the keyboard to the mouse (often ending up selecting a different parent altogether!). Its kind of a deal breaker. ( :P ) You'll feel it if you do a build with this patch.

So waiting to here more...

	Patrick Brosset <:pbro>
	Comment 38 • 10 years ago

Comment on attachment 8557223 [details] [diff] [review]
bug_1120111: rev_3: Added onPickerCanceled event

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

Thanks for the updated patch. I have made some comments about how to solve the issue you were describing.
Also, I have played with the picker, with the patch applied, and I like it a lot. The ESC/ENTER keys help a lot.
The left/right keys are very nice too. I don't feel like prev/next sibling is such a big problem not having for now. I like it as it is now and would like to see this land! We can always add them as a second patch or bug later.
I'll let Brian reply too, but in the meantime, it'd be really great if you could work on adding a new test for this new feature, to avoid future regressions.
Here's a little help on how to do this:

- Open folder browser/devtools/inspector/test/.

- Create a new test file. A good name could be: browser_inspector_highlighter-keybinding.js

- Register this new test file (by its name) in browser.ini (in the same folder)

- Look at other highlighter test files in the same dir to copy/paste the test structure (license info, use strict, description comment, add_task(...), opening a new tab, etc...

- Then comes the hard part: the actual test code. In your case, you need to start the picker, and then synthesize keyboard events inside the page, to then check whether the picker reacts appropriately. The first thing to do, is to add the ability to synthesize events in the page: you'll need to add a new message listener in doc_frame_script.js. Because the test and the page don't run in the same process, we have to communicate by messages. So in this file, search for "Test:SynthesizeMouse", copy/paste the whole message listener block, renaming it SynthesizeKey, and then adapt the code in the callback to use EventUtils.synthesizeKey(theKey, options, node.ownerDocument.defaultView); instead.
There are plenty of usage examples for this here: http://mxr.mozilla.org/mozilla-central/search?string=synthesizeKey&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

- Once this is done, you can simulate keys from your test file with this:
yield executeInContent("Test:SynthesizeKey", {...});

I know it's a lot to take in, especially if you have never written mozilla mochitests before, so let me know if you'd like me to create this test file, or help you get started with some code.

Once the test is written, it can be ran with the following command:
./mach mochitest-devtools browser/devtools/inspector/test/browser_inspector_highlighter-keybinding.js

::: browser/devtools/framework/toolbox-highlighter-utils.js
@@ +191,5 @@
>    }
>  
>    /**
> +   * When the picker is canceled
> +   * @param  {Object} data Information about the current node selected before canceling

Remove this @param comment.

@@ +194,5 @@
> +   * When the picker is canceled
> +   * @param  {Object} data Information about the current node selected before canceling
> +   */
> +  function onPickerNodeCanceled(data) {
> +    toolbox.selection.setNodeFront(data.node, "picker-node-canceled");

This line should be removed. If the picker mode is canceled, we just want to stop the picker, no select another node. Otherwise the current selection will be lost.

::: toolkit/devtools/server/actors/highlighter.js
@@ +277,5 @@
> +          return;
> +
> +        case Ci.nsIDOMKeyEvent.DOM_VK_ESCAPE: // cancel picking
> +          this.cancelPick();
> +          events.emit(this._walker, "picker-node-canceled", this._currentNode);

No need to send this._currentNode with this event. The goal of this event is to stop picking, so we don't want the client-side to select a different node.

Also, you need to define this new event in the walkerActor. Open the file /toolkit/devtools/server/actors/inspector.js and search for "picker-node-hovered". This is where all highlighter events are currently defined.

You'll need to add one more:

    "picker-node-canceled" : {
      type: "pickerNodeCanceled"
    }

In fact, because you didn't define this event in your patch, the event was never sent to the client-side, that's why the blue pick icon stayed blue.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 39 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #38)
> Comment on attachment 8557223 [details] [diff] [review]
> bug_1120111: rev_3: Added onPickerCanceled event
> 
> Review of attachment 8557223 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the updated patch. I have made some comments about how to solve
> the issue you were describing.
> Also, I have played with the picker, with the patch applied, and I like it a
> lot. The ESC/ENTER keys help a lot.
> The left/right keys are very nice too. I don't feel like prev/next sibling
> is such a big problem not having for now. I like it as it is now and would
> like to see this land! We can always add them as a second patch or bug later.
OK. That seems logical. Oh.. what about the UX team test??
> I'll let Brian reply too, but in the meantime, it'd be really great if you
> could work on adding a new test for this new feature, to avoid future
> regressions.
> Here's a little help on how to do this:
> 
> - Open folder browser/devtools/inspector/test/.
......
> - Once this is done, you can simulate keys from your test file with this:
> yield executeInContent("Test:SynthesizeKey", {...});
> 
> I know it's a lot to take in, especially if you have never written mozilla
> mochitests before, so let me know if you'd like me to create this test file,
> or help you get started with some code.
> 
> Once the test is written, it can be ran with the following command:
> ./mach mochitest-devtools
> browser/devtools/inspector/test/browser_inspector_highlighter-keybinding.js
I've heard about mochitests. Never really written one bottom up before (Edited one though). I would like to first try out on my own. If the endeavour seems hopeless, or if I run aground, I'll let you know! (I think it'll be fun) 
> ::: browser/devtools/framework/toolbox-highlighter-utils.js
> @@ +191,5 @@
> >    }
> >  
> >    /**
> > +   * When the picker is canceled
> > +   * @param  {Object} data Information about the current node selected before canceling
> 
> Remove this @param comment.
Will do so. So I guess our |onPickerNodeCanceled| will no longer require parameters.  
> @@ +194,5 @@
> > +   * When the picker is canceled
> > +   * @param  {Object} data Information about the current node selected before canceling
> > +   */
> > +  function onPickerNodeCanceled(data) {
> > +    toolbox.selection.setNodeFront(data.node, "picker-node-canceled");
> 
> This line should be removed. If the picker mode is canceled, we just want to
> stop the picker, no select another node. Otherwise the current selection
> will be lost.
> 
> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +277,5 @@
> > +          return;
> > +
> > +        case Ci.nsIDOMKeyEvent.DOM_VK_ESCAPE: // cancel picking
> > +          this.cancelPick();
> > +          events.emit(this._walker, "picker-node-canceled", this._currentNode);
> 
> No need to send this._currentNode with this event. The goal of this event is
> to stop picking, so we don't want the client-side to select a different node.
Sure. But I'd have to emit the "hovered node" as *picked*, wouldn't I? If I don't, nothing really has been *picked* (We are just left with the last highlighted node). Would that be alright? I guess I'll have to try it out with a build.
> Also, you need to define this new event in the walkerActor. Open the file
> /toolkit/devtools/server/actors/inspector.js and search for
> "picker-node-hovered". This is where all highlighter events are currently
> defined.
> 
> You'll need to add one more:
> 
>     "picker-node-canceled" : {
>       type: "pickerNodeCanceled"
>     }
> 
> In fact, because you didn't define this event in your patch, the event was
> never sent to the client-side, that's why the blue pick icon stayed blue.
Ahh. I was looking for that (It was driving me nuts). Looks like I was looking in the wrong place.

Thanks for the review, Patrick. I'll write about my next steps in another comment.

	Patrick Brosset <:pbro>
	Comment 40 • 10 years ago

(In reply to Abhishek Bhattacharya [:babhishek21] from comment #39)
> I've heard about mochitests. Never really written one bottom up before
> (Edited one though). I would like to first try out on my own. If the
> endeavour seems hopeless, or if I run aground, I'll let you know! (I think
> it'll be fun) 
Awesome. Let me know.

> > ::: browser/devtools/framework/toolbox-highlighter-utils.js
> > @@ +191,5 @@
> > >    }
> > >  
> > >    /**
> > > +   * When the picker is canceled
> > > +   * @param  {Object} data Information about the current node selected before canceling
> > 
> > Remove this @param comment.
> Will do so. So I guess our |onPickerNodeCanceled| will no longer require
> parameters.  
That's correct.

> > ::: toolkit/devtools/server/actors/highlighter.js
> > @@ +277,5 @@
> > > +          return;
> > > +
> > > +        case Ci.nsIDOMKeyEvent.DOM_VK_ESCAPE: // cancel picking
> > > +          this.cancelPick();
> > > +          events.emit(this._walker, "picker-node-canceled", this._currentNode);
> > 
> > No need to send this._currentNode with this event. The goal of this event is
> > to stop picking, so we don't want the client-side to select a different node.
> Sure. But I'd have to emit the "hovered node" as *picked*, wouldn't I? If I
> don't, nothing really has been *picked* (We are just left with the last
> highlighted node). Would that be alright? I guess I'll have to try it out
> with a build.
Not picking anything when pressing ESC was the original goal. So you shouldn't emit any event at all. The use case is the user starts the picker by accident, or realizes they wanted to do something else with the node before picking another one. It's important to make sure that when ESC is pressed, the last selection is still active.

> Thanks for the review, Patrick. I'll write about my next steps in another
> comment.
Thank you!

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 42 • 10 years ago

Comment on attachment 8557984 [details] [diff] [review]
bug_1120111: rev_4: Cleanup

Ok. Did the requested changes. Meanwhile trying to test with a build. 

I will make a separate patch for the test file. That way it won't interfere with my previous changes and keep the patch file small and readable.

	Patrick Brosset <:pbro>
	Comment 43 • 10 years ago

Comment on attachment 8548909 [details] [diff] [review]
bug1120111-corrections.patch

Marking this old patch as obsolete

	Patrick Brosset <:pbro>
	Comment 44 • 10 years ago

Comment on attachment 8557984 [details] [diff] [review]
bug_1120111: rev_4: Cleanup

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

Those changes look good! Thanks!
Let me know if I can be of help for writing the test(s).
I've only got really minor formatting comments below, no need to ask for R? again, you can just fix them, attach the new patch and mark it R+ youself since I've already R+'d this one.

::: toolkit/devtools/server/actors/highlighter.js
@@ +255,5 @@
> +          if (!currentNode.children.length) {
> +            return;
> +          }
> +
> +          /* Set firstElementChild by default */

Can you use // instead of /* */ for single line comments?

@@ +259,5 @@
> +          /* Set firstElementChild by default */
> +          let child = currentNode.firstElementChild;
> +          /**
> +           * If currentNode is parent of hoveredNode, then
> +           * previously selected childNode is set

Also for this comment, even if multi-lines:

// If currentNode is parent of hoveredNode, then
// previously selected childNode is set.

@@ +283,5 @@
> +
> +        default: break;
> +      }
> +
> +      /* Store currently attached element */

Same comment here.

	Patrick Brosset <:pbro>
	Comment 45 • 10 years ago

Comment on attachment 8557223 [details] [diff] [review]
bug_1120111: rev_3: Added onPickerCanceled event

Marking this older patch as obsolete too.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 46 • 10 years ago

Hey Patrick,
I was wondering if changing the title of this Bug (to a shorter version) was required. If you plan on making any such changes, then I'll make sure to put the new title as the commit message. Do let me know.

I am also starting work on the test file. Will let you know if I need help.

	Patrick Brosset <:pbro>
	Comment 47 • 10 years ago

Good point, the title of the bug could use some rephrasing.
Having said this, the commit message should not necessarily be a copy of the bug's title, but rather a (short) description of what you did.

Also, it should be formatted like so:
Bug ####### - short description of changes; r=reviewer

So in your case, something like this:
Bug 1120111 - Adds key listeners to highlighter pick mode to traverse DOM and submit/cancel selection; r=pbrosset

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 48 • 10 years ago

Alright. Fixed the comments. 

Working on tests, now.

	Patrick Brosset <:pbro>
	Comment 49 • 10 years ago

Clearing the needinfo for now. Don't hesitate to flag me again when you need help with the test or want me to review.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 50 • 10 years ago

Hey Patrick,

I have started with the test files. The only problems I've run into till now:
1. How do I check whether a node was picked by picker, when I synthesized the "ENTER/RETURN" key?
2. How do I check whether the picker was canceled, when I synthesized the "ESCAPE" key?
3. How do I check that the entire "body" element is highlighted? (Not really useful, just curious)

I am done with all other test sequences. If you want a look at what I've done, look below:
[1] browser_inspector_highlighter_keybindings.js => http://babhishek21.pastebin.mozilla.org/8639369
[2] doc_inspector_highlighter_dom.html => http://babhishek21.pastebin.mozilla.org/8639442
[3] doc_frame_script.js => http://babhishek21.pastebin.mozilla.org/8639505

	Patrick Brosset <:pbro>
	Comment 51 • 10 years ago

(In reply to Abhishek Bhattacharya [:babhishek21] from comment #50)
> Hey Patrick,
> 
> I have started with the test files. The only problems I've run into till now:
> 1. How do I check whether a node was picked by picker, when I synthesized
> the "ENTER/RETURN" key?
> 2. How do I check whether the picker was canceled, when I synthesized the
> "ESCAPE" key?
> 3. How do I check that the entire "body" element is highlighted? (Not really
> useful, just curious)

Here are a few pointers that should hopefully answer your questions:

- open the inspector:
  let {toolbox, inspector} = yield openInspectorForURL(TEST_URI);
- start the picker mode:
  yield toolbox.highlighterUtils.startPicker();
- stop the picker mode:
  yield toolbox.highlighterUtils.stopPicker();
- know when a node is picked:
  let onPicked = toolbox.once("picker-node-picked");
  // ... do something here that will pick a node
  yield onPicked;
- another way to do the same:
  let onInspectorUpdated = inspector.once("inspector-updated");
  // ... do something here that will pick a node
  yield onInspectorUpdated;
- simulate ENTER, ESCAPE or other keys on the document:
  unfortunately, this isn't yet possible, take a look at the doc_frame_script.js in the inspector test folder, search for SynthesizeMouse. You'll find a message listener. You'll need to add another one for keyboard events. Something like this:

addMessageListener("Test:SynthesizeKey", function(msg) {
  let {key, options} = msg.data;
  EventUtils.synthesizeKey(key, options, content);
  sendAsyncMessage("Test:SynthesizeKey");
});

And then, from your test, you can call it this way:

yield executeInContent("Test:SynthesizeKey", {
  options: {},
  key: "VK_LEFT"
});

Why do we go through messages? Because the test and the test page don't live in the same process anymore, that means the test cannot access the test page directly. Instead, it needs to go through the message-manager, and it can do this with the code I gave you above.

> I am done with all other test sequences. If you want a look at what I've
> done, look below:
> [1] browser_inspector_highlighter_keybindings.js =>
> http://babhishek21.pastebin.mozilla.org/8639369
This first version looks pretty good! But since you want to simulate keys while in picker mode, you first need to start the picker mode, not just call selectAndHighlightNode, using the startPicker and stopPicker functions described above.
> [2] doc_inspector_highlighter_dom.html =>
> http://babhishek21.pastebin.mozilla.org/8639442
> [3] doc_frame_script.js => http://babhishek21.pastebin.mozilla.org/8639505
Oh cool, you already added the SynthesizeKey message listener! Can I suggest that instead of having a switch case, you just make it accept the key code (like "VK_LEFT"), and it uses it directly in SynthesizeKey? Also, please add an optional argument for passing options to SynthesizeKey. This way it will be possible for future tests to simulate shift or alt keys too.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 52 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #51)
> (In reply to Abhishek Bhattacharya [:babhishek21] from comment #50)
> Here are a few pointers that should hopefully answer your questions:
> 
> - open the inspector:
>   let {toolbox, inspector} = yield openInspectorForURL(TEST_URI);
> - start the picker mode:
>   yield toolbox.highlighterUtils.startPicker();
> - stop the picker mode:
>   yield toolbox.highlighterUtils.stopPicker();
> - know when a node is picked:
>   let onPicked = toolbox.once("picker-node-picked");
>   // ... do something here that will pick a node
>   yield onPicked;
> - another way to do the same:
>   let onInspectorUpdated = inspector.once("inspector-updated");
>   // ... do something here that will pick a node
>   yield onInspectorUpdated;
> - simulate ENTER, ESCAPE or other keys on the document:
>   unfortunately, this isn't yet possible, take a look at the
> doc_frame_script.js in the inspector test folder, search for
> SynthesizeMouse. You'll find a message listener. You'll need to add another
> one for keyboard events. Something like this:
> 
> addMessageListener("Test:SynthesizeKey", function(msg) {
>   let {key, options} = msg.data;
>   EventUtils.synthesizeKey(key, options, content);
>   sendAsyncMessage("Test:SynthesizeKey");
> });
> 
> And then, from your test, you can call it this way:
> 
> yield executeInContent("Test:SynthesizeKey", {
>   options: {},
>   key: "VK_LEFT"
> });
Thanks for the pointers. I'm going to experiment a bit now! :P
> > I am done with all other test sequences. If you want a look at what I've
> > done, look below:
> > [1] browser_inspector_highlighter_keybindings.js =>
> > http://babhishek21.pastebin.mozilla.org/8639369
> This first version looks pretty good! But since you want to simulate keys
> while in picker mode, you first need to start the picker mode, not just call
> selectAndHighlightNode, using the startPicker and stopPicker functions
> described above.
OK. Oddly enough, some other tests for the |inspector| don't start the |Picker| first (they just start calling the |highlighter|). But now I understand that it only calls the highlighter and not the picker. What I was wondering though: After I start the picker, will the rest of my test code still perform as expected?
> > [2] doc_inspector_highlighter_dom.html =>
> > http://babhishek21.pastebin.mozilla.org/8639442
> > [3] doc_frame_script.js => http://babhishek21.pastebin.mozilla.org/8639505
> Oh cool, you already added the SynthesizeKey message listener! Can I suggest
> that instead of having a switch case, you just make it accept the key code
> (like "VK_LEFT"), and it uses it directly in SynthesizeKey? Also, please add
> an optional argument for passing options to SynthesizeKey. This way it will
> be possible for future tests to simulate shift or alt keys too.
Ok. Thanks for the idea of passing an object as parameter.

	Patrick Brosset <:pbro>
	Comment 53 • 10 years ago

(In reply to Abhishek Bhattacharya [:babhishek21] from comment #52)
> > > http://babhishek21.pastebin.mozilla.org/8639369
> > This first version looks pretty good! But since you want to simulate keys
> > while in picker mode, you first need to start the picker mode, not just call
> > selectAndHighlightNode, using the startPicker and stopPicker functions
> > described above.
> OK. Oddly enough, some other tests for the |inspector| don't start the
> |Picker| first (they just start calling the |highlighter|). But now I
> understand that it only calls the highlighter and not the picker.
Yeah, a lot of tests don't need the picker mode at all, they just need to highlight an element, and then check some other things. But in your case, you want to make sure the keybindings work while the picker mode is on. And your key event listener isn't added unless the picker mode is on anyway, so you do need to start it.
> What I was
> wondering though: After I start the picker, will the rest of my test code
> still perform as expected?
Yeah it should, although I haven't had a very thorough look at the code yet, you'll have to try and run the test. Btw, I forgot if I mentioned this: https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 54 • 10 years ago

Hey Patrick. I'm almost done with the tests. Just a few nitty-gritties left:
1. I am still stuck with finding a way to check whether the picker has stopped/picked an element. The only way I know right now is something like this:
> yield some-function()
> where some-function returns toolbox.once("picker-node-picked") after simulating a key event.
I wanted to use the variable |isPicking| [1] to check further. However it seems it isn't an exported symbol.

2. Apart from the slightly weird syntax of Mochitests, do standard JS semantics like while/for loop work in those?

[1] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox-highlighter-utils.js#45

	Patrick Brosset <:pbro>
	Comment 55 • 10 years ago

(In reply to Abhishek Bhattacharya [:babhishek21] from comment #54)
> Hey Patrick. I'm almost done with the tests. Just a few nitty-gritties left:
> 1. I am still stuck with finding a way to check whether the picker has
> stopped/picked an element. The only way I know right now is something like
> this:
> > yield some-function()
> > where some-function returns toolbox.once("picker-node-picked") after simulating a key event.
> I wanted to use the variable |isPicking| [1] to check further. However it
> seems it isn't an exported symbol.
isPicking is indeed an private variable of the highlighter utils module. I don't think it hurts to expose a public isPicking getter if you need. We haven't had that need up until now, so I'm not sure you really need it either. What's wrong with waiting for |toolbox.once("picker-node-picked")| as you described above? That looks fine to me.

> 2. Apart from the slightly weird syntax of Mochitests, do standard JS
> semantics like while/for loop work in those?
The syntax is just JS (es6), so yeah, any while/for loop will work.

	Patrick Brosset <:pbro>
	Comment 56 • 10 years ago

Hey Abhishek, could you let me know how things are evolving? The patch is almost ready for landing, we just need some tests, so it'd be great to get it in soon. Let me know if you need help.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 57 • 10 years ago

So sorry, I didn't reply earlier. I was caught up with some college work. I was just looking it up yesterday.

Almost done. I'll try and run the "mochitests" today and upload the "mochitests" patch today/tomorrow (approximately within 15 hrs from the time of posting this comment) If you need a faster patch, I can upload the patch right now (I just haven't tested the "tests" yet, so you could run/modify them if you wish). Let me know.

One more thing: When are you available on IRC (specific time window with your time zone)? Thanks.

	Patrick Brosset <:pbro>
	Comment 58 • 10 years ago

(In reply to Abhishek Bhattacharya [:babhishek21] from comment #57)
> So sorry, I didn't reply earlier. I was caught up with some college work. I
> was just looking it up yesterday.
No problem.
> Almost done. I'll try and run the "mochitests" today and upload the
> "mochitests" patch today/tomorrow (approximately within 15 hrs from the time
> of posting this comment) If you need a faster patch, I can upload the patch
> right now (I just haven't tested the "tests" yet, so you could run/modify
> them if you wish). Let me know.
15 hours sounds great :) No need to rush and attach something now.
> One more thing: When are you available on IRC (specific time window with
> your time zone)? Thanks.
My timezone is GMT+1, I'm on IRC right now, and will be for another 3 to 4 hours I think.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 59 • 10 years ago

So first patch regarding the Mochitests. Getting a lot of |TEST-UNEXPECTED-FAIL| events, though everything seems to be working fine. Also, my test seems to timeout right after the last operation. Is there a definite call to be placed at the end of tests, to exit safely?

	Patrick Brosset <:pbro>
	Comment 60 • 10 years ago

Comment on attachment 8570569 [details] [diff] [review]
bug_1120111_t: First patch for Mochitests

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

That's a very good start. There are just some details that make the test fail, but these things are kind of hard to know if you've not worked with our tests before.
Instead of just giving you my comments below, I'm also going to attach a patch for you, so you more easily see what I mean.

One other thing worth fixing (which you'll see in my patch but that I didn't mention below):
Our tests now run in e10s (multi-process) mode, this means that the test itself runs in a different process than the test page. So accessing DOM elements from the test page is possible via what we call CPOW (cross-process object wrappers), but not recommended and we're trying to move away from using them.
In practice, what this means for you is that you should try not to use getNode anymore. This function reaches into the test page, through the process boundary, and returns not the DOM node itself, but a wrapper for it that acts like the DOM node.

So, you'll see in my patch that I've made a simple change to SynthesizeMouse so it also accepts selectors instead of only CPOWs, this way, we can ask it to simulate mouse-over events on elements without having to get the elements.
And I've also removed occurrences of getNode in the test.

::: browser/devtools/inspector/test/browser_inspector_highlighter-keybinding.js
@@ +8,5 @@
> +
> +const TEST_URL = TEST_URL_ROOT + "doc_inspector_highlighter_dom.html";
> +
> +add_task(function*() {
> +	let {inspector, toolbox} = yield openInspectorForURL(TEST_URL);

nit: there are mixed tabs and spaces in this file. Please indent only with 2 spaces.

@@ +14,5 @@
> +	info("Starting element picker");
> +	yield toolbox.highlighterUtils.startPicker();
> +
> +	info("Selecting the simple-div1 DIV");
> +	yield selectAndHighlightNode("#simple-div1", inspector);

You shouldn't be using this function here, it selects nodes in the inspector directly, while what we want to do instead is simulating mouse-over nodes in the page directly.

You could do this instead:

yield moveMouseOver("#simple-div1");

And somewhere in this file, defined the function moveMouseOver:

  function moveMouseOver(selector) {
    info("Waiting for element " + selector + " to be highlighted");
    executeInContent("Test:SynthesizeMouse", {
      options: {type: "mousemove"},
      center: true
    }, {node: getNode(selector), false);
    return inspector.toolbox.once("picker-node-hovered");
  }

The same comment applies to other places in this file where you've used selectAndHighlightNode

@@ +25,5 @@
> +
> +  yield doKeyHover({key: "VK_RIGHT", options: {}});
> +  highlightedNode = yield getHighlitNode(toolbox);
> +  isnot(highlightedNode, getNode("#useful-para"), "The highlighter doesn't show wrong node");
> +  is(highlightedNode, getNode("#useless-para"), "The highlighter shows correct node");

There can only be one highlighted node at all times, so having 2 asserts seems useless. Just is(highlightedNode,...) is enough.

@@ +30,5 @@
> +
> +  info("Selecting the useful-para paragraph DIV");
> +	yield selectAndHighlightNode("#useful-para", inspector);
> +	highlightedNode = yield getHighlitNode(toolbox);
> +  is(highlightedNode, getNode("#useful-para"), "The highlighter shows correct node");

You've used this comment in many assertions. This makes it harder to investigate test failures, should that happen, when reading logs.
Please try and make each assertion description unique somehow. For instance, this one could be rephrased:
"The #useful-para node is highlighted"

@@ +67,5 @@
> +
> +  // Testing pick-node shortcut
> +  info("Testing enter/return key as pick-node command");
> +	yield doKeyPick({key: "VK_RETURN", options: {}});
> +  yield toolbox.selection.once("new-node-front");

This event listener should be moved inside doKeyPick, to keep the test code easier to read. See my comment inside doKeyPick.

@@ +78,5 @@
> +	yield selectAndHighlightNode("#simple-div1", inspector);
> +
> +  info("Testing escape key as cancel-picker command");
> +  yield doKeyStop({key: "VK_ESCAPE", options: {}});
> +  yield toolbox.selection.once("new-node-front");

You should remove this line, because pressing ESC stops the picker, it does not select a new node, so this event won't be emitted.
And you should also add an assertion to ensure that the current selection is still the same as before.

@@ +89,5 @@
> +
> +  function doKeyPick(msg) {
> +    info("Key pressed. Waiting for element to be picked");
> +    executeInContent("Test:SynthesizeKey", msg);
> +    return inspector.toolbox.once("picker-node-picked");

You could return this instead:

    return promise.all([
      toolbox.selection.once("new-node-front"),
      inspector.once("inspector-updated")
    ]);

This makes sure both promises resolve before resolving. So this will ensure that we both get a new selection active and that the inspector has refreshed completely to show this new selection.

@@ +95,5 @@
> +
> +  function doKeyStop(msg) {
> +    info("Key pressed. Waiting for picker to be canceled");
> +    executeInContent("Test:SynthesizeKey", msg);
> +    return inspector.toolbox.once("picker-node-canceled");

Listening for this event will cause a timeout, because the toolbox never emits it. The highlighter emits via the WalkerActor, and that is being listened by the toolbox-highlighter-utils object, which uses it to stop the picker, but it doesn't emit the event on the toolbox object.
So instead, you should do:

return inspector.toolbox.once("picker-stopped");

@@ +98,5 @@
> +    executeInContent("Test:SynthesizeKey", msg);
> +    return inspector.toolbox.once("picker-node-canceled");
> +  }
> +
> +});
\ No newline at end of file

nit: Please make sure files end with a newline.

::: browser/devtools/inspector/test/doc_frame_script.js
@@ +269,5 @@
> + */
> +addMessageListener("Test:SynthesizeKey", function(msg) {
> +  let {key, options} = msg.data;
> +
> +  EventUtils.synthesizeKey(key, options);

I think you need to use this form:
EventUtils.synthesizeKey(key, options, content);
'content' is the window object in the scope of this frame script, and synthesizeKey needs a window to know where the event should be dispatched.

::: browser/devtools/inspector/test/doc_inspector_highlighter_dom.html
@@ +4,5 @@
> +
> +<p>Hello World!</p>
> +
> +<div id="complex-div">
> +	<div id="simple-div1">

nit: we normally indent mozilla code with 2 spaces.

	Patrick Brosset <:pbro>
	Comment 61 • 10 years ago

Here's the patch I mentioned in my last comment.
I hope this helps.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 62 • 10 years ago

So Local Machine tests passed. \o/

One teeny tiny problem. The test is taking too long (~106 seconds on my non-optimized debug build). The log suggests using mouseover/out events.

> [4616] WARNING: Please do not use mouseenter/leave events in chrome. They are
> slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file...

Hence the Timeout FAIL.

>  INFO TEST-UNEXPECTED-FAIL |
> browser/devtools/inspector/test/browser_inspector_highlighter-keybinding.js | This
> test exceeded the timeout threshold. It should be rewritten or split up. If that's
> not possible, use requestLongerTimeout(N), but only as a last resort. - expected
> PASS
> SUITE-END | took 105s

So where now? I think our tests are working alright. Only the timeout issue left now.

	Patrick Brosset <:pbro>
	Comment 63 • 10 years ago

Clearing the NI? flag since you've already pinged me for review on the patch.
I was away last week, hence the delay on the review. I should be able to get to it in the coming days.

	Patrick Brosset <:pbro>
	Comment 64 • 10 years ago

I landed some changes to highlighter.js yesterday that caused the first patch not to apply cleanly anymore. So I rebased the first patch.

	Patrick Brosset <:pbro>
	Comment 65 • 10 years ago

Also rebased the test patch. I'm going to code review now.

	Patrick Brosset <:pbro>
	Comment 66 • 10 years ago

Comment on attachment 8575151 [details] [diff] [review]
bug_1120111_t: rev_1: Added corrections. Local Machine Tests Passed - rebased

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

Unrelated to this patch, but I noticed this while testing the highlighter: picker-node-hovered events are being emitted when any other keys are pressed too. That's because the _onKey event handler doesn't return if an unrecognized key is pressed. This ends up causing more protocol traffic than necessary.
It's not a big deal right now, and the patch 1 where this should be fixed is already R+'d. So I'll leave this to you to decide whether you want to revisit patch 1 and add a return in the "default" case of the switch statement, or land patch 1 and 2 first and worry about this later.

Now, this test patch looks good to me, and the test passes fine locally for me. If it times out for you, then there are good chances it will time out on our TRY server too, so let's split it in several parts:

- instead of browser_inspector_highlighter-keybinding.js create 3 files: browser_inspector_highlighter-keybinding_01.js, browser_inspector_highlighter-keybinding_02.js, browser_inspector_highlighter-keybinding_03.js
- test the first-child selection in 01
- test the previously chosen child in 02
- and test the cancel and submit in 03

This should be a simple file split. Don't forget to do
yield toolbox.highlighterUtils.stopPicker();
at the end of 01 and 02 though.

::: browser/devtools/inspector/test/doc_frame_script.js
@@ +239,5 @@
>   * - {Number} y
>   * - {Boolean} center If set to true, x/y will be ignored and
>   *             synthesizeMouseAtCenter will be used instead
>   * - {Object} options Other event options
> + * - {String} selector An option selector that will be used to find the node to

s/option/optional

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 67 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #66)
> Comment on attachment 8575151 [details] [diff] [review]
> bug_1120111_t: rev_1: Added corrections. Local Machine Tests Passed - rebased
> 
> Review of attachment 8575151 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Unrelated to this patch, but I noticed this while testing the highlighter:
> picker-node-hovered events are being emitted when any other keys are pressed
> too. That's because the _onKey event handler doesn't return if an
> unrecognized key is pressed. This ends up causing more protocol traffic than
> necessary.
> It's not a big deal right now, and the patch 1 where this should be fixed is
> already R+'d. So I'll leave this to you to decide whether you want to
> revisit patch 1 and add a return in the "default" case of the switch
> statement, or land patch 1 and 2 first and worry about this later.
That's scary. AFAIR there used to be a default case. Hmm, I'll fix that pronto.
> Now, this test patch looks good to me, and the test passes fine locally for
> me. If it times out for you, then there are good chances it will time out on
> our TRY server too, so let's split it in several parts:
> 
> - instead of browser_inspector_highlighter-keybinding.js create 3 files:
> browser_inspector_highlighter-keybinding_01.js,
> browser_inspector_highlighter-keybinding_02.js,
> browser_inspector_highlighter-keybinding_03.js
> - test the first-child selection in 01
> - test the previously chosen child in 02
> - and test the cancel and submit in 03
> 
> This should be a simple file split. Don't forget to do
> yield toolbox.highlighterUtils.stopPicker();
> at the end of 01 and 02 though.
OK. I'll try to finish it up today. (My mid-term exams are on!)
> ::: browser/devtools/inspector/test/doc_frame_script.js
> @@ +239,5 @@
> >   * - {Number} y
> >   * - {Boolean} center If set to true, x/y will be ignored and
> >   *             synthesizeMouseAtCenter will be used instead
> >   * - {Object} options Other event options
> > + * - {String} selector An option selector that will be used to find the node to
> 
> s/option/optional
Linguistics :-(

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 68 • 10 years ago

So I added the "return" statement to the default case in the |on_Key| handler.

BTW, what is this "rebase" that you've done.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 69 • 10 years ago

So I split the test files as you wished. Timeouts gone now! Cheers.

	Patrick Brosset <:pbro>
	Comment 70 • 10 years ago

Comment on attachment 8575286 [details] [diff] [review]
bug_1120111: rev_4.2: All done. Tested.

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

Thanks for adding this return statement.
I'm going to assume this was the only change since the last patch, so R+.
I'll also rebase this again so it can be landed on fx-team.

	Patrick Brosset <:pbro>
	Comment 71 • 10 years ago

Comment on attachment 8575149 [details] [diff] [review]
bug_1120111: rev_4.1: Final cleanup - rebased

When you upload new versions of your patches, please do mark the old one as obsolete on bugzilla.

	Patrick Brosset <:pbro>
	Comment 72 • 10 years ago

Comment on attachment 8575288 [details] [diff] [review]
bug_1120111_t: rev_2: Split files. All passed. Tested.

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

Alright, thanks for splitting that test. The patch looks good to me.
As discussed on IRC, I'll rebase both patches and push to TRY.

	Patrick Brosset <:pbro>
	Comment 73 • 10 years ago

Pending try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98e776247caa

	Patrick Brosset <:pbro>
	Comment 74 • 10 years ago

Rebased.

	Patrick Brosset <:pbro>
	Comment 75 • 10 years ago

Rebased

	Patrick Brosset <:pbro>
	Comment 76 • 10 years ago

The try build shows a couple of permanent failures in tests:
browser/devtools/framework/test/browser_keybindings.js
browser_webconsole_bug_653531_highlighter_console_helper.js

	Patrick Brosset <:pbro>
	Comment 77 • 10 years ago

This should hopefully fix the test failures.
Abhishek told me he was in the middle of exams at the moment, so I took the liberty to fix the patch instead of asking him to as I suspect he wouldn't have had time. Plus, the fixes were simple ones.
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=edbe974de944

	Patrick Brosset <:pbro>
	Comment 78 • 10 years ago

That last try push seems to be going well. There's a bustage on osx 10.6, but that's bug 1141745, unrelated to this patch.

	Patrick Brosset <:pbro>
	Comment 79 • 10 years ago

Pushed to fx-team:
remote:   https://hg.mozilla.org/integration/fx-team/rev/e026b87b725e
remote:   https://hg.mozilla.org/integration/fx-team/rev/a5f54d3827d5

	Carsten Book [:Tomcat]
	Comment 80 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/e026b87b725e
https://hg.mozilla.org/mozilla-central/rev/a5f54d3827d5

	Patrick Brosset <:pbro>
	Comment 81 • 10 years ago

Nice work Abhishek! Thanks for working on this bug. This has now landed in m-c, which means it will be in Firefox Nightly tomorrow, and will be released with 39.
Let me know if you're interested to tackle other bugs, I can help find some.

	Abhishek Bhattacharya [:babhishek21] Assignee
	Comment 82 • 10 years ago

Thanks Patrick. This one dragged on longer than I expected. I'll be happy to work on more bugs. (after the March 13). Feel free to suggest any.

BTW, when does a feature move from the Nightly to Release?

	Patrick Brosset <:pbro>
	Comment 83 • 10 years ago

(In reply to Abhishek Bhattacharya [:babhishek21] from comment #82)
> Thanks Patrick. This one dragged on longer than I expected.
It's often the case that bugs take longer to fully resolve than expected.
> I'll be happy to
> work on more bugs. (after the March 13). Feel free to suggest any.
How about bug ?
> BTW, when does a feature move from the Nightly to Release?
We have a new release every 6 weeks, and we have the nightly, aurora, beta, release channels (more info here: https://wiki.mozilla.org/RapidRelease).
The next uplift from nightly to aurora is on the 30th of March (https://wiki.mozilla.org/RapidRelease/Calendar), after this, you'll need to wait another 12 weeks until it reaches release.
But we have a fair amount of devtools users on Aurora (branded Developer Edition) who will see this new feature in about 2,5 weeks.

	Patrick Brosset <:pbro>
	Comment 84 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #83)
> > I'll be happy to
> > work on more bugs. (after the March 13). Feel free to suggest any.
> How about bug ?
Of course I hit the submit button too soon, before pasting the bug numbers here :) So, here they are:
bug 792063 (adding a shortcut for the last result in the webconsole)
bug 882790 (adding a new toolbar button to pause on exceptions)
bug 1134073 (showing the cause for XHR in the netmonitor panel)