Closed Bug 1311409 Opened 3 years ago Closed 3 years ago

Eyedropper click doesn't work, have to use Enter key

Categories

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

50 Branch
x86_64
Windows 10
defect

Tracking

(firefox49 unaffected, firefox50+ fixed, firefox51+ fixed, firefox52- verified)

VERIFIED FIXED
Firefox 52
Tracking Status
firefox49 --- unaffected
firefox50 + fixed
firefox51 + fixed
firefox52 - verified

People

(Reporter: git, Assigned: miker)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

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.
Component: Untriaged → Developer Tools: CSS Rules Inspector
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
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.
Nightly x64 works too.
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?
Flags: needinfo?(git)
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
Flags: needinfo?(git)
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Mike, do you think you'd have time to look into this P1?
Flags: needinfo?(mratcliffe)
(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
(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.
Assignee: nobody → mratcliffe
Flags: needinfo?(mratcliffe)
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).
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
Attachment #8803522 - Flags: review+
Attachment #8803522 - Flags: approval-mozilla-beta?
Attachment #8803522 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(mratcliffe)
(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.
Attachment #8803522 - Attachment is obsolete: true
Attachment #8803522 - Flags: approval-mozilla-beta?
Attachment #8803522 - Flags: approval-mozilla-aurora?
Flags: needinfo?(mratcliffe)
Attachment #8803563 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a645a24a07bf
https://hg.mozilla.org/mozilla-central/rev/f3d44030406e
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Tracking 52- since this is now resolved fixed.
Regression in 50, tracking.  Michael, was the uplift request intentionally not moved over to the new patch?
Flags: needinfo?(mratcliffe)
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
Flags: needinfo?(mratcliffe)
Attachment #8803563 - Flags: approval-mozilla-beta?
Attachment #8803563 - Flags: approval-mozilla-aurora?
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+
Attachment #8803563 - Flags: approval-mozilla-beta?
Attachment #8803563 - Flags: approval-mozilla-beta+
Attachment #8803563 - Flags: approval-mozilla-aurora?
Attachment #8803563 - Flags: approval-mozilla-aurora+
Hello Sharak, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(git)
(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.
Flags: needinfo?(git)
(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! :)
Status: RESOLVED → VERIFIED
(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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.