961771 - Highlighter remains active when switching tools

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Rob Campbell [:rc] (:robcee)]

STR:

1. Open the Inspector in highlight mode (cmd-alt-C)
2. Switch to another tool (e.g., cmd-alt-K)

Expected:

Highlighter stops following mouse.

Actual:

Highlighter follows mouse.

Comment 1

[Patrick Brosset <:pbro>]

I think this makes sense indeed.
Because we switch to the inspector when entering highlight mode, we should exit highlight mode when we switch to another tool.
This is very easily fixed too, I'm flagging this as a good first bug.

Comment 2

[Peiyong Lin[:lpy](UTC+8)]

I am willing to take this bug. Any more guide please? Which part of code should I look into? I am trying to read the codes in "browser/devtools/inspector".

Comment 3

[Patrick Brosset <:pbro>]

You should look into browser/devtools/framework/toolbox.js
The Toolbox object fires "select" events when tools are being switched too.
See: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#802

You should also look at the toolbox's startPicker method: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#1110
It is responsible for starting the highlighter when you click on the inspector button.

So, when starting the picker, we should listen to the select event, and if it occurs, stop the picker.
Of course, if stopPicker occurs first, we should stop listening to the event.

I hope this helps to get started.
Feel free to drop by our IRC channel #devtools if you need more help.

Comment 4

[Peiyong Lin[:lpy](UTC+8)]

Thank you :) I assign this bug to myself. And I will submit the patch soon.

Comment 5

[Peiyong Lin[:lpy](UTC+8)]

I get confused on where to add the event listener. I tried to add |this.addEventListener("select", this.stopPicker)| to [1], but it didn't work. Any help? Thank you.

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

Comment 6

[Patrick Brosset <:pbro>]

Yep, adding your event listener in the |done| function should work because it's called after the pick process has started.

The issue comes from the way you're adding the event listener:
> this.addEventListener("select", this.stopPicker);
The "select" event is an event that's handled by the EventEmitter library. See how's it's included here:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#14
And how the Toolbox is made observable here:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#92
This gives the toolbox the ability to do things like this:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#802

Looking into the EventEmitter library here:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/event-emitter.js#45
you'll see that the way to listen to events is to use the |on| function.
So something like |this.on("select", this.stopPicker)| would work better.

However, one word of caution, in what I wrote above, the stopPicker function won't be bound to the right scope, so you could do |this.on("select", this.stopPicker.bind(this))| instead, but then again, doing so will make it impossible to remove the event listener properly afterwards.
So, you might want to save the bound version of the function first somewhere: |this.stopPicker = this.stopPicker.bind(this);|, so then you can do |this.on("select", this.stopPicker)| to add the listener and |this.off("select", this.stopPicker)| to remove it when you don't need it anymore.

Comment 7

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug961771.patch

I appreciate your help. :) Thank you.

Comment 8

[Patrick Brosset <:pbro>]

Thanks for the patch. It works correctly, and the code looks good.
As discussed over IRC, let's try to add a test for this, so we can avoid future regressions on this feature.
Here is some help:

- We have several types of tests, but one that suits this usecase nicely is the mochitest-browser type of test. This is an integration type of test where the browser is started and you get to manipulate its UI (via javascript) as a user would do it (https://developer.mozilla.org/en-US/docs/Mochitest and https://developer.mozilla.org/en-US/docs/Browser_chrome_tests).
- You can create a new mochitest-browser test file and place it into the /browser/devtools/inspector/test/ folder.
- It should be referenced in browser.ini and its name should start with browser_.
- It should contain a single |test| function, and call |waitForExplicitFinish()| to be able to finish the test asynchronously after the |test| function has returned, and call |finish()| when it's done.
- head.js in the inspector/test directory is loaded in the test's scope and contains all sorts of useful functions to ease the writing of the test.
- You have several assertions functions at your disposal: is, ok, isnot, ...
- Running the test goes like: ./mach mochitest-browser browser/devtools/inspector/test/name_of_your_test.js
- You can (and should) also run some/all tests by specifying a parent dir: ./mach mochitest-browser browser/devtools/inspector/ or even ./mach mochitest-browser browser/devtools/

It's not enough information to write the test, but I think the best for you is to look at a few of the tests we have in this directory, and that will probably be enough to learn what you need to learn to complete it.

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8364168 [details] [diff] [review]
bug961771.patch

Can you ask for review again when the test is done?

Comment 10

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug961771-V2.patch

Thank you for your help :)

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8364966 [details] [diff] [review]
bug961771-V2.patch

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

Looking good.
I think that's all we need here.
I have just a few very small comments below.

Also, I started a TRY build: https://tbpl.mozilla.org/?tree=Try&rev=b4cb0ad89635

Can you attach a new patch with the changes required, and I'll R+ the patch as soon as the try build is green.

::: browser/devtools/inspector/test/browser.ini
@@ +47,5 @@
>  [browser_inspector_bug_922125_destroy_on_navigate.js]
>  [browser_inspector_bug_952294_tooltips_dimensions.js]
>  [browser_inspector_bug_958456_highlight_comments.js]
>  [browser_inspector_bug_958169_switch_to_inspector_on_pick.js]
> +[browser_inspector_highlighter_picker.js]

When the test isn't about a brand new feature, but rather about a bug we're fixing, we tend to add the bug number in the file name as the other ones just above.
Can you rename it browser_inspector_bug_961771_picker_stops_on_tool_select.js ?

::: browser/devtools/inspector/test/browser_inspector_highlighter_picker.js
@@ +1,4 @@
> +/* 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/. */
> +

We don't always do it, but I think it's good to start each test with a comment that explains what this test is about. Something like:

// Test that the highlighter's picker mode is stopped when a different tool is selected

@@ +17,5 @@
> +    gBrowser.selectedBrowser.removeEventListener("load", onload, true);
> +    doc = content.document;
> +    waitForFocus(setupTest, content);
> +  }, true);
> +  content.location = "data:text/html,<h1>foo</h1><h2>bar</h2>";

The markup here isn't required, just "data:text/html,testing the highlighter goes away on tool selection" would be enough.

@@ +41,5 @@
> +    inspector = doc = toolbox = null;
> +    gBrowser.removeCurrentTab();
> +    finish();
> +  }
> +} 
\ No newline at end of file

There's an extra white-space here.
Also, we normally add an empty line at the end.

Comment 12

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug961771-V3.patch

Thank you. I update the patch.

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8365013 [details] [diff] [review]
bug961771-V3.patch

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

::: browser/devtools/inspector/test/browser.ini
@@ +47,5 @@
>  [browser_inspector_bug_922125_destroy_on_navigate.js]
>  [browser_inspector_bug_952294_tooltips_dimensions.js]
>  [browser_inspector_bug_958456_highlight_comments.js]
>  [browser_inspector_bug_958169_switch_to_inspector_on_pick.js]
> +[browser_inspector_highlighter_picker.js]

Final change needed: update the test reference here.
Should be browser_inspector_bug_961771_picker_stops_on_tool_select.js

Comment 14

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug961771-Final.patch

Sorry about that. And thank you :)

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8365023 [details] [diff] [review]
bug961771-Final.patch

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

Ok, looks good.
Also, try build is green.

Comment 16

[Patrick Brosset <:pbro>]

Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/7906b49abbb3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7906b49abbb3