993445 - [highlighter] Highlight nodes on hover of breadcrumbs

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

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.

Comment 1

[gurukolisetty]

I want to work on this bug.Can you tell me the requirements to start with fixing this bug?

Comment 2

[Brian Grinstead [:bgrins]]

(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.

Comment 3

[Patrick Brosset <:pbro>]

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.

Comment 4

[Jayesh]

Hey Patrick I want to work on this bug  as my first bug..

Thanks and Regards,
Jayesh

Comment 5

[Patrick Brosset <:pbro>]

(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.

Comment 6

[Jayesh]

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...

Comment 7

[Patrick Brosset <:pbro>]

(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.

Comment 8

[Jayesh]

Attachment: breadcrumbsHighlight.patch

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.

Comment 9

[Patrick Brosset <:pbro>]

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.

Comment 10

[Patrick Brosset <:pbro>]

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.

Comment 11

[Jayesh]

Ok. I got my mistake there.. Will update that..

Comment 12

[Jayesh]

Attachment: bcHighlight.patch

Hi Patrick,
Please do have a look.

Thanks,
Jayesh

Comment 13

[Patrick Brosset <:pbro>]

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

Comment 14

[Jayesh]

Thanks patrick, will have a look and update you...

Comment 15

[Jayesh]

Attachment: bcHighlight-2.patch

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.

Comment 16

[Patrick Brosset <:pbro>]

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.

Comment 17

[Jayesh]

Attachment: bcHighlight-3.patch

Hi Patrick,

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

Thanks,
Jayesh

Comment 18

[Patrick Brosset <:pbro>]

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.

Comment 19

[Jayesh]

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

Comment 20

[Patrick Brosset <:pbro>]

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.

Comment 21

[Jayesh]

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

Comment 22

[Jayesh]

Hi Patrick,

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

Thanks,
Jayesh

Comment 23

[Patrick Brosset <:pbro>]

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.

Comment 24

[Jayesh]

Thanks Patrick,

Will try out that and work on it today.

Thanks,
Jayesh

Comment 25

[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

Comment 26

[Brian Grinstead [:bgrins]]

(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.

Comment 27

[Jayesh]

Attachment: patch-breadcrumb-highlight.zip

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

Comment 28

[Brian Grinstead [:bgrins]]

Comment on attachment 8461120 [details] [diff] [review]
patch-breadcrumb-highlight.zip

Could you upload this as a patch?

Comment 29

[Jayesh]

Attachment: bcHighlightWithTest.patch

Hi Brian,

Please have a look at the attached patch.

Thanks,
Jayesh

Comment 30

[Brian Grinstead [:bgrins]]

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

Comment 31

[Jayesh]

Attachment: bcHighlightNodeHoverTest.patch

Hi Brian,

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

Thanks,
Jayesh

Comment 32

[Brian Grinstead [:bgrins]]

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

Comment 33

[Jayesh]

Attachment: bcHoverTestChange.patch

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

Comment 34

[Jayesh]

Attachment: bcHighlightHoverTest.patch

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

Comment 35

[Brian Grinstead [:bgrins]]

Comment on attachment 8464841 [details] [diff] [review]
bcHoverTestChange.patch

Don't need the test changes along, the full patch is fine

Comment 36

[Brian Grinstead [:bgrins]]

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");

Comment 37

[Brian Grinstead [:bgrins]]

Attachment: breadcrumb-hover-rebased.patch

Comment 38

[Jayesh]

Attachment: bcHighlightPatchToFxTeamRepo.patch

Hi Brian,

Have uploaded the patch removing the trailing spaces.

Thanks,
Jayesh

Comment 39

[Jayesh]

Attachment: bcHighlightPatchToFxTeamRepo.patch

Hi Brian,

Changing the commit message.

Thanks,
Jayesh

Comment 40

[Brian Grinstead [:bgrins]]

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

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2813f8efa366