1377513 - color picker stuck if element picker was active/enabled when color picker got opened

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox 56.0a1 20170630030203 on Windows 8.1

The color picker is stuck if the element picker was active when color picker got opened.

Steps to reproduce:
1. Open https://www.mozilla.org/en-US/firefox/nightly/firstrun/
2. Hit Ctrl + C to open the Inspector.
3. On the right, selecte the 'Rules' tab if necessary.
4. Scroll down to the first color element.
5. Click on it.
6. Click on the color picker.
7. Move the mouse.

Actual result:
Color picker doesn't move.

Expected result:
Color picker can be moved by mouse.

Comment 1

[Patrick Brosset <:pbro>]

Tested on both 56 and 57 on Windows 10, and could not reproduce this issue so far.
Did you happen to see if there were any JavaScript errors in the browser console (ctrl+shift+J) when that happened?

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

It still reproduces with 56 and 56 on Windows 8 here the first time the color picker gets opened after the inspector launched. The inspector's element picker is still active. There is nothing relevant in the browser console.

Comment 3

[Patrick Brosset <:pbro>]

Ok, I understand now. I did not realize that the picker was still active. So what's happening here is that there are 2 overlays in the page now: the element picker and the eyedropper. And they both are listening for mouse events. Looks like the element picker prevents the eyedropper from receiving them.

I think we should hide the element picker overlay as soon as we start the eyedropper from then color picker tooltip.

This is where the eyedropper is being opened:
http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js#164-185

Somewhere in there we should cancel the picker with something like:
this.inspector.toolbox.highlighterUtils.stopPicker() (which returns a promise I think).

This should do the trick. I can help if someone is interested in fixing this.
Our contribution docs are hosted here: http://docs.firefox-dev.tools/

Comment 4

[Abhinav Koppula]

Attachment: Bug 1377513 - Cancel element-picker on selecting eye-dropper if element-picker is already selected.

Review commit: https://reviewboard.mozilla.org/r/179592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179592/

Comment 5

[Abhinav Koppula]

Hi Patrick,
Thanks for the useful pointers. I have taken a stab at it. 
Please let me know if it looks fine.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8907916 [details]
Bug 1377513 - Cancel element-picker on selecting eye-dropper if element-picker is already selected.

https://reviewboard.mozilla.org/r/179592/#review185340

Works great, thank you! I just made a minor comment to be addressed, but otherwise we should land this patch.

One more important note though, we should cover this with a new test, so that it doesn't re-occur in the future.
That means creating a new automated test file (a mochitest) in \devtools\client\inspector\rules\test\
We have some docs about these tests here: http://docs.firefox-dev.tools/tests/mochitest-devtools.html and here http://docs.firefox-dev.tools/tests/writing-tests.html
Essentially, you'll need to create a new test file, and reference this test file in the browser.ini file in the same directory.

Here is some partial code that should help you get started, let me know if you need more help to complete the test:

```
add_task(function* () {
  yield addTab("data:text/html;charset=utf-8,<style>body{background:red}</style>");
  let {view} = yield openRuleView();

  // 1. Start the element picker mode now, look at (hint: search for startPicker in the inspector folder, you should find tests that do this already)
  ...
  
  // 2. Now open the color picker tooltip (hint: there are several tests that do this, like \devtools\client\inspector\rules\test\browser_rules_colorpicker-hides-on-tooltip.js)
  ...
  
  // 3. Inside the color tooltip, click on the eyedropper icon to start it (hint: we also have a test that does this, so you can copy some code from \devtools\client\inspector\rules\test\browser_rules_eyedropper.js)
  ...
  
  // 4. Verify that the picker mode is now off to make sure your fix works (hint: the toolbox object emits events, one of them is "picker-stopped". So you could listen to it before clicking the eyedropper icon, and then verify that it was received here. The test \devtools\client\inspector\test\browser_inspector_picker-stop-on-tool-change.js does this).
});

```

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:169
(Diff revision 1)
>    _openEyeDropper: function () {
>      let {inspector, toolbox, telemetry} = this.inspector;
>      telemetry.toolOpened("pickereyedropper");
> +
> +    // cancelling picker(if it is already selected) on opening eye-dropper
> +    // see bug-1377513

Please remove this comment line. No need to reference bugs in comments, we can use the mercurial history/blame commands to know why a thing was added.

Comment 7

[Abhinav Koppula]

Comment on attachment 8907916 [details]
Bug 1377513 - Cancel element-picker on selecting eye-dropper if element-picker is already selected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179592/diff/1-2/

Comment 8

[Abhinav Koppula]

Comment on attachment 8907916 [details]
Bug 1377513 - Cancel element-picker on selecting eye-dropper if element-picker is already selected.

https://reviewboard.mozilla.org/r/179592/#review185340

Hi Patrick,
Thanks for the detailed explanation on how to go about writing the test.
Along with the test, I have done a bit of refactoring in moving a method to head.js.
Please let me know if it is fine.

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8907916 [details]
Bug 1377513 - Cancel element-picker on selecting eye-dropper if element-picker is already selected.

https://reviewboard.mozilla.org/r/179592/#review185910

Looks great, with the comment below addressed!
Thanks a lot.
I'll push to the CI environment so we can see if the tests still pass.

::: devtools/client/inspector/rules/test/browser.ini:80
(Diff revision 2)
>  [browser_rules_class_panel_state_preserved.js]
>  [browser_rules_class_panel_toggle.js]
>  [browser_rules_colorpicker-and-image-tooltip_01.js]
>  [browser_rules_colorpicker-and-image-tooltip_02.js]
>  [browser_rules_colorpicker-appears-on-swatch-click.js]
> +[browser_rules_colorpicker_bug_1377513.js]

Please rename this test with a self-explanatory name. It's better just to find a short description, because it's easier to scan later for people who are looking for specific tests.

I suggest: browser_rules_colorpicker-hides-element-picker.js

Comment 10

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cea76e4b0a1aad4350481821af6cfc14cf1f2a1a

Comment 11

[Abhinav Koppula]

Comment on attachment 8907916 [details]
Bug 1377513 - Cancel element-picker on selecting eye-dropper if element-picker is already selected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179592/diff/2-3/

Comment 12

[Abhinav Koppula]

Thanks Patrick for the review.
I have fixed the naming as per your comment.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5434fbd370ce
Cancel element-picker on selecting eye-dropper if element-picker is already selected. r=pbro

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5434fbd370ce

Comment 15

[Sajedul Islam]

I have reproduced this bug with Nightly 56.0a1 (2017-06-30) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest beta !

Build   ID    20171102181127
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20171101]