Closed Bug 993445 Opened 6 years ago Closed 5 years ago

[highlighter] Highlight nodes on hover of breadcrumbs

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: pbro, Assigned: jayesh.choudhari17, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 11 obsolete files)

For consistency reasons, everywhere in our devtools where we display DOM nodes, they should be displayed similarly and support the same kind of actions.

One such action is highlight in the page on hover. Highlighter on hover works in the markupview, console and variablesview. However it doesn't work in the breadcrumbs, but it should.
Blocks: firebug-gaps
Whiteboard: [good first bug][lang=js][mentor=pbrosset]
Mentor: pbrosset
Whiteboard: [good first bug][lang=js][mentor=pbrosset] → [good first bug][lang=js]
I want to work on this bug.Can you tell me the requirements to start with fixing this bug?
(In reply to gurukolisetty from comment #1)
> I want to work on this bug.Can you tell me the requirements to start with
> fixing this bug?

Patrick will be able to give further instructions specifically for this bug, but you will want to start with building and running the project - see https://wiki.mozilla.org/DevTools/Hacking for steps to get to the first run.
As Brian said, you should follow that link to set up the environment. Setting up the required dependencies and building for the first time takes a little bit of time, but once that's done, further builds will be a lot faster.
For anything related to the environment, don't hesitate to ask in #introduction.

For anything related to the devtools source code and this bug in particular, you can ask in #devtools.

Here are some pointers to start on this bug:

- What's already in place:
  - the highlighter already exists, it is used from the markup panel to highlight nodes on mouseover
  - the breadcrumbs widget (just above the markup panel) is already here
- What's missing:
  - a mouse listener on the breadcrumbs widget that would use the highlighter to show the corresponding nodes in the page

In terms of code:

- breadcrumbs.js is the file that manages the breadcrumbs wiedgt: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/breadcrumbs.js#89 this line is where events are added.
- toolbox-highlighter-utils.js is the file that handles communication to the 'actor' that does the highlighting. http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox-highlighter-utils.js#200 is the function that you can call to highlight a node.
- the toolbox-highlighter-utils is available via the toolbox object. From the breadcrumbs class, you can do 'this.inspector.toolbox.highlighterUtils' to access it.
- if you want an example of how this can be used, you should look at markup-view.js:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#187 and
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#199

So the bug can be summed up as: add an event listener to the breadcrumbs widget, when you detect that the mouse is over one of the crumbs, get the NodeFront object for this crumb, and use the highlighter utils to highlight that node.

I think the open question at this stage is how to get the NodeFront object for a crumb. But what I explained above should get you started I guess. Don't hesitate to get back with more questions as soon as you have something.
Hey Patrick I want to work on this bug  as my first bug..

Thanks and Regards,
Jayesh
(In reply to Jayesh from comment #4)
> Hey Patrick I want to work on this bug  as my first bug..
> 
> Thanks and Regards,
> Jayesh
If you don't yet have the environment in place, make sure you follow the documentation here: https://wiki.mozilla.org/DevTools/Hacking
Once done, you'll find information about how to get started with the code in comment 3.
Hey Patrick,

I am already done with the required changes and have checked the things on my local. If you say I can post the patch here...
(In reply to Jayesh from comment #6)
> Hey Patrick,
> 
> I am already done with the required changes and have checked the things on
> my local. If you say I can post the patch here...
Sure, feel free to attach your first patch to the bug here.
Attached patch breadcrumbsHighlight.patch (obsolete) — Splinter Review
Hi Patrick,

Have the change that i have made to breadcrumbs.js makes the required thing to work but facing a issue that the section or content doesn't remains highlighted. Tried on that to figure it out.. But could make it. Your suggestions are required.
Attachment #8446714 - Flags: review+
Comment on attachment 8446714 [details] [diff] [review]
breadcrumbsHighlight.patch

review+ is normally used by the reviewer when the patch has been reviewed and is OK. If you want to ask for a review, please choose review? instead.
In that case though, if you're still facing issues, feedback? is better I believe.
I'll take a look at this tomorrow.
Attachment #8446714 - Flags: review+ → feedback?(pbrosset)
Comment on attachment 8446714 [details] [diff] [review]
breadcrumbsHighlight.patch

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

I don't think you attached the right patch. Looking at the diff view it doesn't seem this way.
Attachment #8446714 - Flags: feedback?(pbrosset)
Ok. I got my mistake there.. Will update that..
Attached patch bcHighlight.patch (obsolete) — Splinter Review
Hi Patrick,
Please do have a look.

Thanks,
Jayesh
Attachment #8446736 - Flags: review?(pbrosset)
Attachment #8446736 - Flags: feedback?(pbrosset)
Attachment #8446714 - Attachment is obsolete: true
Comment on attachment 8446736 [details] [diff] [review]
bcHighlight.patch

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

Thanks for the patch. The general direction is good.

I've added a few comments about missing mouseover, missing removeEventListeners and potentially useless code.
Can you take a look, submit a new patch and R? me? Thanks!

Also, we'll need to add tests for this new feature, to make sure they don't break in the future.
I'll help you with that.

::: browser/devtools/inspector/breadcrumbs.js
@@ +87,5 @@
>      this.container.parentNode.appendChild(this.separators);
>  
>      this.container.addEventListener("mousedown", this, true);
>      this.container.addEventListener("keypress", this, true);
> +    this.container.addEventListener("mouseover", this, true);

You also need to add a listener for mouseleave so that you can stop highlighting when the mouse leaves the breadcrumbs widget.
To unhighlight, you can call highlighterUtils.unhighlight()

Also, you need to call removeEventListener at some stage, to clean up behind you.
Look in the file for the 'destroy' method, you'll see where this is done.

@@ +374,5 @@
>        event.preventDefault();
>        event.stopPropagation();
>      }
> +
> +    if (event.type == "mouseover" && event.button == 0) {

Checking for event.button == 0 is not necessary here as the event type is mouseover.

@@ +375,5 @@
>        event.stopPropagation();
>      }
> +
> +    if (event.type == "mouseover" && event.button == 0) {
> +      // on mouseover

This comment alone isn't useful, we know its a mouseover from the line above.
If you feel like a comment is required here, please write a monger description like: "On mouseover, highlight the corresponding nodes in the page" or something like that.

@@ +376,5 @@
>      }
> +
> +    if (event.type == "mouseover" && event.button == 0) {
> +      // on mouseover
> +      let timer;

timer isn't used in your code, please remove it.

@@ +385,5 @@
> +        if (target.tagName == "button") {
> +          target.onBreadcrumbsHover();
> +        }
> +      }
> +      container.addEventListener("mouseover", handleMouseover, false);

To be honest, I'm not sure what you're trying to do here.
Line 91, you already added 'this.container.addEventListener("mouseover", this, true);' which is good, so there's no need to listen to mouseover again here.

This function here is already executed when the mouseover event occurs, why don't you simply get the target, check if it's a button and call onBreadcrumbsHover on it? As you've properly done lines 384 to 386?

Additionally, you'll need to check whether the new hovered node isn't the same as the previous one. Indeed, the event may be fired many times for the same node and we don't want to call highlightNodeFront for the same node everytime.

@@ +501,5 @@
>  
> +    button.onBreadcrumbsHover = () => {
> +      this.selection.setNodeFront(aNode, "breadcrumbs");
> +      let options={};
> +      this.inspector.toolbox.highlighterUtils.highlightNodeFront(aNode, options);

This part looks good, the only remark is that options is empty, you don't need to pass it at all.
this.inspector.toolbox.highlighterUtils.highlightNodeFront(aNode) should be enough
Attachment #8446736 - Flags: review?(pbrosset)
Attachment #8446736 - Flags: feedback?(pbrosset)
Thanks patrick, will have a look and update you...
Attached patch bcHighlight-2.patch (obsolete) — Splinter Review
Hi Patrick,

Have made the required changes as suggested. But still the "mouseleave" event is fired before leaving the breadcrumbs button. I was not able to figure that out.
Attachment #8447213 - Flags: review?(pbrosset)
Comment on attachment 8447213 [details] [diff] [review]
bcHighlight-2.patch

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

Alright, we're getting there!
I just have a few more comments, but as soon as these are fixed, the only thing needed is tests! And then we can land that patch.

::: browser/devtools/inspector/breadcrumbs.js
@@ +384,5 @@
> +    }
> +
> +    if (event.type == "mouseleave") {
> +      let target = event.originalTarget;
> +      if (target.tagName == "button") {

This check isn't needed, the mouseleave event was added to the breadcrumbs container element, so we want to know whenever the mouse leaves the container, not a button.

@@ +385,5 @@
> +
> +    if (event.type == "mouseleave") {
> +      let target = event.originalTarget;
> +      if (target.tagName == "button") {
> +        target.onBreadcrumbsLeave();

Hiding the highlighter isn't really something specific to a breadcrumb button so there's no need to define a function on target for this.
You can simplify this whole part to:

if (event.type == "mouseleave") {
  this.inspector.toolbox.highlighterUtils.unhighlight();
}

And therefore remove the onBreadcrumbsLeave function you added.

Note that I have removed the 'true' parameter you had added to the 'unhighlight' function, it isn't needed here, this is mostly for test purposes.

@@ +504,5 @@
>        this.selection.setNodeFront(aNode, "breadcrumbs");
>      };
>  
> +    button.onBreadcrumbsHover = () => {
> +      this.selection.setNodeFront(aNode, "breadcrumbs");

You shouldn't call setNodeFront here, this will change the currently selected node in the inspector on hover of a breadcrumbs button. This isn't the intended behavior.

@@ +511,5 @@
> +
> +    button.onBreadcrumbsLeave = () => {
> +      this.inspector.toolbox.highlighterUtils.unhighlight(true);
> +    };
> +    

nit: a few useless whitespaces here.
Attachment #8447213 - Flags: review?(pbrosset)
Attached patch bcHighlight-3.patch (obsolete) — Splinter Review
Hi Patrick,

Have made the required changes and can find that working as we need it.

Thanks,
Jayesh
Attachment #8448019 - Flags: review?(pbrosset)
Comment on attachment 8448019 [details] [diff] [review]
bcHighlight-3.patch

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

Perfect.
We now need tests for this new feature.

For this you'll need to:
- create a new test file in browser/devtools/inspector/test
- the file name should be something like 'browser_inspector_breadcrumbs-highlight-hover.js' (must start with browser)
- the new test file name should be added to browser.ini (in the same directory)

Next, you'll need to actually write the test and for this there's a bunch of concepts you need to know (first one is that most things are asynchronous in tests and tests make heavy use of Tasks). I'll be writing a new comment soon to give more information about that. In the meantime, you can take a look at a few tests:
- browser_inspector_highlighter.js
- browser_inspector_infobar.js
- browser_inspector_basic_highlighter.js
The new test you'll write should be somewhat similar to those ones.

::: browser/devtools/inspector/breadcrumbs.js
@@ +384,5 @@
> +    }
> +
> +    if (event.type == "mouseleave") {
> +      this.inspector.toolbox.highlighterUtils.unhighlight();
> +    }    

nit: trailing whitespaces here after the closing brace } should be removed.
Attachment #8448019 - Flags: review?(pbrosset) → review+
Hi Patrick, 

I am going through the files suggested by you, but want to know how to run/execute the files or how can i debug them to understand the complete functionality.

Thanks,
Jayesh
Here are some pages that might help:
- https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests
- https://wiki.mozilla.org/DevTools/mochitests_coding_standards

The type of test you'll need to write for this new feature is a mochitest-browser test.
Each tool folder has its own test suite. The breadcrumb widget is tested as part of the inspector tool folder.
To run this test suite: './mach mochitest-devtools browser/devtools/inspector/test'
To run a single test  : './mach mochitest-devtools browser/devtools/inspector/test/a_test_file_name.js'
To run a sequence of tests:  './mach mochitest-devtools browser/devtools/inspector/test --start-at first_test.js --end-at last-test.js'

- Create a new test file in the browser/devtools/inspector/test directory: 'browser_inspector_breadcrumbs-highlight-hover.js'
- Add an entry in 'browser.ini' for this new test file
- Run your new test: './mach mochitest-devtools browser/devtools/inspector/test/browser_inspector_breadcrumbs-highlight-hover.js'

You can take a look at this test which already does things with the breadcrumbs widget: browser_inspector_breadcrumbs.js

I suggest using the same pattern as in browser_inspector_basic_highlighter.js to write your test, it's a nice way to test asynchronous things.
For instance, this is a skeleton of a test file:

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

"use strict";

// Test that hovering over nodes in the breadcrumbs widget highlights the
// node in the page

let test = asyncTest(function*() {
  yield addTab("data:text/html;charset=utf-8,PUT SOME CONTENT HTML HERE");
  let {toolbox, inspector} = yield openInspector();

  ... ADD TEST CODE HERE ...

  gBrowser.removeCurrentTab();
});

Now for the actual content, here are some things to keep in mind:
- you'll need to get access to the breadcrumbs widget, once you get the reference to 'inspector' thanks to 'openInspector', it's easy: 'inspector.breadcrumbs'
- from that, you'll need to get access to the breadcrumbs buttons which, since you spent some time looking at breadcrumbs.js, I guess you know how to do
- you'll then need to simulate mouseover events on these buttons. If you run a search on the whole test folder for "mouseover" or "mousemove", you'll see we use the EventUtils library for this (for instance: EventUtils.synthesizeMouseAtCenter)
- once that's in place, you'll need to test that the highlighter was requested to highlight a node. I don't think you should test that the node is actually highlighted, there are other tests for this, and testing it here would bring additional complexity and risks of failures. What you *do* need to test though is that the highlighter was called. One way to do this is to wait for the "node-highlight" event to be fired:

let onNodeHighlighted = toolbox.once("node-highlight");
... simulate mouse over ...
yield onNodeHighlighted;

I hope this helps.
I know writing mochitest-browser tests is a whole new thing if you haven't done so before, so just try and attach a patch with something, even if that doesn't work, and I'll help you through it.
Hi Patrick,

Thanks for the information. I will start writing it now. I got to know about Mochitest while I was going through some other bug. And tried it for the folder we are working on. But couldn't see any browser window opening up, whereas when I did ran the test for the whole browser, I was able to see the window coming up. So was confused on that. But now, when I am running that for a single js file I can see the browser window coming up and can go ahead with writing the test cases. Will update you as I am done with the first patch.

Thanks,
Jayesh
Hi Patrick,

How can I put a breakpoint in the file test case js file..??

Thanks,
Jayesh
You can run the test like that:
./mach mochitest-devtools --jsdebugger path/to/test.js
This will open the browser toolbox and wait to run the test. So you can set breakpoints before starting the test.
If the files you want breakpoints in do not show in the debugger, then you can add 'debugger;' statements in the code directly and use the --jsdebugger to make sure the debugger is started.
That should work.
If it does not, feel free to join our #devtools IRC channel to ask there.
Thanks Patrick,

Will try out that and work on it today.

Thanks,
Jayesh
Hi Patrick,

I have got stuck at a point. Have wrote the test case in the similar way as in 'browser_inspector_basic_highlighter.js'. But the highlighter highlights the 'body' (not getting any clue why so). 

The code highlights are as follows:

  let bcButtons = inspector.breadcrumbs["container"];
  //this gives me the breadcrumb buttons
 
  info("Selecting the test node");
  
  yield selectNode("span", inspector);
 
  let onNodeHighlighted = toolbox.once("node-highlight");
  let button = bcButtons.childNodes[2];
  //for simplicity considering only a single node "span"

  EventUtils.synthesizeMouseAtCenter(button, {type: "mousemove"});
  //have not passed the third argument in this function.

  yield onNodeHighlighted;

  ok(isHighlighting(), "The highlighter is shown on a markup container hover");
  //the highlighter is called but highlights the body.

  is(getHighlitNode(), getNode("span"), "The highlighter highlights the right node");

  gBrowser.removeCurrentTab();


Can you please tell me why is the highlighter highlighting the body event after synthesizing mousemove on the button.

Thanks,
Jayesh
(In reply to Jayesh from comment #25)
> Hi Patrick,
> 
> I have got stuck at a point. Have wrote the test case in the similar way as
> in 'browser_inspector_basic_highlighter.js'. But the highlighter highlights
> the 'body' (not getting any clue why so). 
> 
> The code highlights are as follows:
> 
>   let bcButtons = inspector.breadcrumbs["container"];
>   //this gives me the breadcrumb buttons
>  
>   info("Selecting the test node");
>   
>   yield selectNode("span", inspector);
>  
>   let onNodeHighlighted = toolbox.once("node-highlight");
>   let button = bcButtons.childNodes[2];
>   //for simplicity considering only a single node "span"
> 
>   EventUtils.synthesizeMouseAtCenter(button, {type: "mousemove"});
>   //have not passed the third argument in this function.
> 
>   yield onNodeHighlighted;
> 
>   ok(isHighlighting(), "The highlighter is shown on a markup container
> hover");
>   //the highlighter is called but highlights the body.
> 
>   is(getHighlitNode(), getNode("span"), "The highlighter highlights the
> right node");
> 
>   gBrowser.removeCurrentTab();
> 
> 
> Can you please tell me why is the highlighter highlighting the body event
> after synthesizing mousemove on the button.
> 
> Thanks,
> Jayesh

So, it is pausing at the `yield onNodeHighlighted;` call, or is it finishing, but just not highlighting the correct thing? 

One thing I've noticed with synthesizeMouseAtCenter is that the third param should be the window (not sure if it is required), so this may be why the mousemove is not working correctly.

EventUtils.synthesizeMouseAtCenter(button, {type: "mousemove"}, button.ownerDocument.defaultView);

You may also console.log the button / button.outerHTML and make sure it is what you think it is.
Assignee: nobody → jayesh.choudhari17
Status: NEW → ASSIGNED
Attached patch patch-breadcrumb-highlight.zip (obsolete) — Splinter Review
Hi Brian,

Thanks for the update. I have made the changes as per your suggestions and it works fine. I am attaching the test-case file as well as the diff of the "breadcrumb.js" and "browser.ini" from the devtools/inspector/test folder after adding the update of the new test file "browser_inspector_breadcrumbs_highlight_hover.js". Do have a look; will wait for your feedback.

Thanks,
Jayesh
Attachment #8461120 - Flags: review?(bgrinstead)
Attachment #8461120 - Flags: feedback?(bgrinstead)
Comment on attachment 8461120 [details] [diff] [review]
patch-breadcrumb-highlight.zip

Could you upload this as a patch?
Attachment #8461120 - Flags: review?(bgrinstead)
Attachment #8461120 - Flags: feedback?(bgrinstead)
Attached patch bcHighlightWithTest.patch (obsolete) — Splinter Review
Hi Brian,

Please have a look at the attached patch.

Thanks,
Jayesh
Attachment #8463316 - Flags: review?(bgrinstead)
Attachment #8463316 - Flags: feedback?(bgrinstead)
Attachment #8461120 - Attachment is obsolete: true
Attachment #8447213 - Attachment is obsolete: true
Attachment #8446736 - Attachment is obsolete: true
Comment on attachment 8463316 [details] [diff] [review]
bcHighlightWithTest.patch

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

Looks good.  Just a couple of minor notes in the code plus this:

1) Update the commit message on the patch (it has multiple lines and looks cut off right now).  It should look something like:

Bug 993445: Highlight nodes on hover of breadcrumbs;r=pbrosset

2) I got a conflict when applying the browser.ini file against the latest code, so it needs to be rebased.

::: browser/devtools/inspector/test/browser.ini
@@ +51,5 @@
>  [browser_inspector_bug_958456_highlight_comments.js]
>  [browser_inspector_bug_958169_switch_to_inspector_on_pick.js]
>  [browser_inspector_bug_961771_picker_stops_on_tool_select.js]
>  [browser_inspector_bug_962478_picker_stops_on_destroy.js]
> +[browser_inspector_breadcrumbs_highlight_hover.js]

Please move this file up to the list in alphabetical order (after [browser_inspector_breadcrumbs.js])

::: browser/devtools/inspector/test/browser_inspector_breadcrumbs_highlight_hover.js
@@ +12,5 @@
> +  let {toolbox, inspector} = yield openInspector();
> +
> +  info("Selecting the test node");
> +  yield selectNode("span", inspector);
> +  

Nit: remove trailing whitespace

@@ +13,5 @@
> +
> +  info("Selecting the test node");
> +  yield selectNode("span", inspector);
> +  
> +  let bcButtons = inspector.breadcrumbs["container"];

Nit: remove trailing whitespace
Attachment #8463316 - Flags: review?(bgrinstead)
Attachment #8463316 - Flags: feedback?(bgrinstead)
Attachment #8463316 - Flags: feedback+
Attached patch bcHighlightNodeHoverTest.patch (obsolete) — Splinter Review
Hi Brian,

Have made the changes as suggested. Do have a look.

Thanks,
Jayesh
Attachment #8463533 - Flags: feedback?(bgrinstead)
Comment on attachment 8463533 [details] [diff] [review]
bcHighlightNodeHoverTest.patch

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

I'm still getting an error applying the patch:

patching file browser/devtools/inspector/test/browser.ini
Hunk #1 FAILED at 13
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/inspector/test/browser.ini.rej

Can you make sure you pull down the latest version of the code and reapply / rebuild the patch?

::: browser/devtools/inspector/test/browser_inspector_breadcrumbs_highlight_hover.js
@@ +12,5 @@
> +  let {toolbox, inspector} = yield openInspector();
> +
> +  info("Selecting the test node");
> +  yield selectNode("span", inspector);
> +  

Still one more line trailing whitespace here

@@ +16,5 @@
> +  
> +  let bcButtons = inspector.breadcrumbs["container"];
> +
> +  let onNodeHighlighted = toolbox.once("node-highlight");
> +  let button = bcButtons.childNodes[2];

May as well include checks for childNodes[0] (<html>) and childNodes[1] (<body>) while you are at it here
Attachment #8463533 - Flags: feedback?(bgrinstead)
Attached patch bcHoverTestChange.patch (obsolete) — Splinter Review
Hi Brian,

Have added the check for childNodes[0] and childNodes[1] in the test file. Do have a look and update.

Thanks and Regards,
Jayesh
Attachment #8464841 - Flags: feedback?(bgrinstead)
Attached patch bcHighlightHoverTest.patch (obsolete) — Splinter Review
Hi Brian,

This is full patch along with the check for the childNodes[0] and childNodes[1] in the test file. I do not know exactly whether the previous patch or this patch will be useful. sorry for the complications

Thanks,
Jayesh
Attachment #8464845 - Flags: feedback?(bgrinstead)
Attachment #8463316 - Attachment is obsolete: true
Attachment #8463533 - Attachment is obsolete: true
Comment on attachment 8464841 [details] [diff] [review]
bcHoverTestChange.patch

Don't need the test changes along, the full patch is fine
Attachment #8464841 - Attachment is obsolete: true
Attachment #8464841 - Flags: feedback?(bgrinstead)
Comment on attachment 8464845 [details] [diff] [review]
bcHighlightHoverTest.patch

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

Once we can get the patch rebase figured out, I think this is pretty much ready to land.  I'll upload a version of your patch based on the latest code for you to import and make any final changes on

::: browser/devtools/inspector/breadcrumbs.js
@@ +381,5 @@
> +      if (target.tagName == "button") {
> +        target.onBreadcrumbsHover();
> +      }
> +    }
> +    

Remove trailing whitespace

::: browser/devtools/inspector/test/browser_inspector_breadcrumbs_highlight_hover.js
@@ +15,5 @@
> +  yield selectNode("span", inspector);
> +  let bcButtons = inspector.breadcrumbs["container"];
> +  let onNodeHighlighted = toolbox.once("node-highlight");
> +
> +  if((bcButtons.childNodes[0].tooltipText == "html") && (bcButtons.childNodes[1].tooltipText == "body")){

When I said check the others, I meant something like:

  let onNodeHighlighted = toolbox.once("node-highlight");
  let button = bcButtons.childNodes[1];
  EventUtils.synthesizeMouseAtCenter(button, {type: "mousemove"}, button.ownerDocument.defaultView);
  yield onNodeHighlighted;
  ok(isHighlighting(), "The highlighter is shown on a markup container hover");
  is(getHighlitNode(), getNode("body"), "The highlighter highlights the right node");

  let onNodeHighlighted = toolbox.once("node-highlight");
  let button = bcButtons.childNodes[2];
  EventUtils.synthesizeMouseAtCenter(button, {type: "mousemove"}, button.ownerDocument.defaultView);
  yield onNodeHighlighted;
  ok(isHighlighting(), "The highlighter is shown on a markup container hover");
  is(getHighlitNode(), getNode("span"), "The highlighter highlights the right node");
Attachment #8464845 - Flags: feedback?(bgrinstead)
Attached patch breadcrumb-hover-rebased.patch (obsolete) — Splinter Review
Attachment #8464845 - Attachment is obsolete: true
Hi Brian,

Have uploaded the patch removing the trailing spaces.

Thanks,
Jayesh
Attachment #8465046 - Flags: feedback?(bgrinstead)
Hi Brian,

Changing the commit message.

Thanks,
Jayesh
Attachment #8465052 - Flags: feedback?(bgrinstead)
Attachment #8465046 - Attachment is obsolete: true
Attachment #8465046 - Flags: feedback?(bgrinstead)
Attachment #8464871 - Attachment is obsolete: true
Attachment #8448019 - Attachment is obsolete: true
Comment on attachment 8465052 [details] [diff] [review]
bcHighlightPatchToFxTeamRepo.patch

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

Changes look good.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=27d538db9856
Attachment #8465052 - Flags: feedback?(bgrinstead) → review+
Mentor: bgrinstead
https://hg.mozilla.org/integration/fx-team/rev/2813f8efa366
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2813f8efa366
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 34
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.