Sharak Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20161019004013

Steps to reproduce:

Firefox Developer Edition 51.0a2. Opening DevTools (F12).
1) Checking Rules section for color -> clicking an icon on the left from the color i.e. color: <icon> #ff0000 -> color picker pops -> clicking Eyedropper icon -> mouse coursor changes to Eyedropper view on the page -> [clicking somewhere on the page to change color in the Rules view] <- doesn't change!
2) The same result when using Eyedropper straight from Eyedropper button (Inspector tab, next to search field and hide-rules button) <- doesn't copy anything to clipbiard


Actual results:

Click does nothing. No copy and no change. Have to use Enter key for that.


Expected results:

https://developer.mozilla.org/en-US/docs/Tools/Eyedropper says "Clicking copies the current color value to the clipboard." for simple picking and "Now, when you click the Eyedropper, the color in the Rules view is set to the color you selected." for editing color.

	Loic
	Comment 1 • 8 years ago

I tried with the latest Nightly (32b) on Win 7 with e10s on/off, no issue, the eyedropper works, copies on click and pastes the color to the inspected element.

I'll try with the 64b version.

	Loic
	Comment 2 • 8 years ago

Nightly x64 works too.

	Julian Descottes [:jdescottes]
	Comment 3 • 8 years ago

FWIW, if the page disables click events via 
> window.addEventListener('click', (e) => {e.stopPropagation();})

then the behavior described by the logger can be observed.

git: Are you seeing this on every page or on a specific page?

	Sharak Reporter
	Comment 4 • 8 years ago

Happens i.e. on Prestashop apps. Here's a fresh demo of Prestashop 1.6.1.7 (newest): http://demo.sharak.pl/en/
On default Firefox (32b) eyedropper works fine - click event copies color and closes eyedropper.
On Firefox Developer Edition (64b) click does nothing. At first I thought it's caused by jquery 1.11.0 Prestashop uses, but I used this version on other apps and it worked perfectly fine. Yet jquery is somehow involved in this as console shows some jquery related reflow notifications on eyedropper's click event:
> reflow: 0.83 ms function „.reliableHiddenOffsets”, jquery-1.11.0.min.js row 3
> reflow: 0.6 ms function „.reliableHiddenOffsets”, jquery-1.11.0.min.js row 3
> reflow: 0.42 ms function „n.expr.filters.hidden”, jquery-1.11.0.min.js row 4

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

Confirmed on http://demo.sharak.pl/en/
Click events never reach the handleEvent callback in devtools\server\actors\highlighters\eye-dropper.js

The box-model highlighter does something similar, and it works fine on this page, so most probably the eye-dropper should do more of what the box-model does around these parts of the code:

http://searchfox.org/mozilla-central/rev/84075be5067b68dc6cb3b89f999645650e68c05b/devtools/server/actors/highlighters.js#381-390

Anyway this sounds bad enough to be a P1.

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

Mike, do you think you'd have time to look into this P1?

	Julian Descottes [:jdescottes]
	Comment 7 • 8 years ago

(Patrick commented in the meantime, but I just wanted to confirm the issue was linked to an event handler on the page, so still posting my comment)

Thanks for the link! The click event is blocked by the following script:
http://demo.sharak.pl/themes/default-bootstrap/js/global.js

>  $(document).on('click', function(e){
>    e.stopPropagation();
>    var elementHide = $(elementClick).next(elementSlide);
>    $(elementHide).slideUp();
>    $(elementClick).removeClass('active');
>  });

which gets called for every click on the document. 

FYI, I tried changing our event listener to useCapture:true in eye-droppper.js [1] and it fixes the issue.

[1] http://searchfox.org/mozilla-central/source/devtools/server/actors/highlighters/eye-dropper.js#147

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 8 • 8 years ago

(In reply to Patrick Brosset <:pbro> from comment #6)
> Mike, do you think you'd have time to look into this P1?

Of course, will get onto it right away.

	Sharak Reporter
	Comment 9 • 8 years ago

I can confirm removing this
>  $(document).on('click', function(e){
>    e.stopPropagation();
>    var elementHide = $(elementClick).next(elementSlide);
>    $(elementHide).slideUp();
>    $(elementClick).removeClass('active');
>  });
from http://demo.sharak.pl/themes/default-bootstrap/js/global.js fixes the clicking issue, but reflow notifications still appear and this doesn't happen on other apps or on this one with defalut Firefox (32b).

	Loic
	Comment 10 • 8 years ago

https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=e0bc88708ffed39aaab1fbc0ac461d93561195de&tochange=a7a882d122e36defe6c2a102a28ae7dfc16311c5

The issue came with the implementation of the new eye-dropper in bug 1262439.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 11 • 8 years ago

Approval Request Comment
[Feature/regressing bug #]: bug 1262439
[User impact if declined]: They will think the color picker eyedropper is broken.
[Describe test coverage new/current, TreeHerder]: Focus issue... not something we can test at this time and this should be landed quickly.
[Risks and why]: Very low risk... just added useCapture to an event listener.
[String/UUID change made/needed]: None

	Julian Descottes [:jdescottes]
	Comment 13 • 8 years ago

The signature of the call to removeListener should also be updated here.

> pageListenerTarget.removeEventListener("click", this);

should become 

> pageListenerTarget.removeEventListener("click", this, true);

Otherwise we are leaking events each time the eyedropper is used.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 14 • 8 years ago

(In reply to Julian Descottes [:jdescottes] (PTO until 04-Nov) from comment #13)
> The signature of the call to removeListener should also be updated here.
> 
> > pageListenerTarget.removeEventListener("click", this);
> 
> should become 
> 
> > pageListenerTarget.removeEventListener("click", this, true);
> 
> Otherwise we are leaking events each time the eyedropper is used.

Thanks for the drive-by... of course it would leak.

Fixed.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 17 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a645a24a07bf16cb68790c80b687452cc23227b2
Bug 1311409 - Eyedropper click doesn't work, have to use Enter key r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3d44030406e4b915d3a779576782ffe55d1c4a2
Bug 1311409 - CRLF to LF r=me

	Carsten Book [:Tomcat]
	Comment 18 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a645a24a07bf
https://hg.mozilla.org/mozilla-central/rev/f3d44030406e

	Marcia Knous [:marcia]
	Comment 19 • 8 years ago

Tracking 52- since this is now resolved fixed.

	Julien Cristau [:jcristau]
	Comment 20 • 8 years ago

Regression in 50, tracking.  Michael, was the uplift request intentionally not moved over to the new patch?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 21 • 8 years ago

Comment on attachment 8803563 [details] [diff] [review]
Patch 1: 1311409-eyedropper-needs-enter-key.diff

(In reply to Julien Cristau [:jcristau] from comment #20)
> Regression in 50, tracking.  Michael, was the uplift request intentionally
> not moved over to the new patch?

Nope, that was a mistake:

Approval Request Comment
[Feature/regressing bug #]: bug 1262439
[User impact if declined]: They will think the color picker eyedropper is broken.
[Describe test coverage new/current, TreeHerder]: Focus issue... not something we can test at this time and this should be landed quickly.
[Risks and why]: Very low risk... just added useCapture to an event listener.
[String/UUID change made/needed]: None

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 22 • 8 years ago

Comment on attachment 8803563 [details] [diff] [review]
Patch 1: 1311409-eyedropper-needs-enter-key.diff

This seems like a core scenario and a recent regression in 50, Aurora51+, Beta50+

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 23 • 8 years ago

Hello Sharak, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/fa17956b4761

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/b5b9398d0310

	Sharak Reporter
	Comment 26 • 8 years ago

(In reply to Ritu Kothari (:ritu) from comment #23)
> Hello Sharak, could you please verify this issue is fixed as expected on a
> latest Nightly build? Thanks!


Confirmed on latest Nightly.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 27 • 8 years ago

(In reply to Sharak from comment #26)
> (In reply to Ritu Kothari (:ritu) from comment #23)
> > Hello Sharak, could you please verify this issue is fixed as expected on a
> > latest Nightly build? Thanks!
> 
> 
> Confirmed on latest Nightly.

Thank you for a prompt verification! :)

	Sharak Reporter
	Comment 28 • 8 years ago

(In reply to Ritu Kothari (:ritu) from comment #27)
> (In reply to Sharak from comment #26)
> > (In reply to Ritu Kothari (:ritu) from comment #23)
> > > Hello Sharak, could you please verify this issue is fixed as expected on a
> > > latest Nightly build? Thanks!
> > 
> > 
> > Confirmed on latest Nightly.
> 
> Thank you for a prompt verification! :)

No problem :)
FFDE updated and working perfectly with eyedropper. Thanks.