1089326 - event.target refers to parent BUTTON instead of clicked IMG when IMG is child of BUTTON (inactive interative content inside <button>)

Status: VERIFIED FIXED

(Core :: DOM: Events, defect, P2)

Description

[Alwyn Edison Mendonca]

User Agent: Mozilla/5.0 (Android; Mobile; rv:33.0) Gecko/33.0 Firefox/33.0
Build ID: 20141011014717

Steps to reproduce:

I visited http://www.myntra.com/Tshirts/Superdry/Superdry-Men-Green--White-Grit-Warriors-T-shirt/420443/buy?serp=25&searchQuery=men%20tees%20polos this product and tried to see different viws, but nothing is happening when  I select them.


Actual results:

After selecting a product I tried to see that product in different view, like side view, back view, closer view and front. Which is not working. 


Expected results:

After selecting different view of the product it has to display them.

Comment 1

[Aaron Train [:aaronmt]]

Reproducible on desktop as well with the mobile UA; work's fine in Chrome desktop/mobile.

Comment 2

[Alwyn Edison Mendonca]

Yes works fine on mobile chrome browser. Thanks.

Comment 3

[Alwyn Edison Mendonca]

Desktop view is totally different and mobile view what I am getting is different same in chrome too. But chrome browser does the required job for me.

Comment 4

[Karl Dubost💡 :karlcow]

Something related to function nrWrapper() ?
in the main page.

Comment 5

[Alwyn Edison Mendonca]

Ok. Is it possible to fix this issue??

Comment 6

[Hallvord R. M. Steen [:hallvors]]

This seems a core issue, not a site issue.

The problem is that they try to read the new src attribute for the big image from the thumbnail DOM element with this code in the event listener:

toggle:function(e){ ...

i=e.target.getAttribute('data-main');

but the target of this event isn't what they expect. The markup is 

<button><img data-main="..." ... ></button>

and Gecko sets event.target to button, not img.

Demo: http://jsfiddle.net/hallvors/tthfrow2/

Comment 7

[Hallvord R. M. Steen [:hallvors]]

Attachment: TC - 1089326.htm

I'm surprised if this isn't a known issue already. Boris?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

This is, or at least was, expected behavior. (HTML4 sort of wanted this behavior). 
Gecko and IIRC Netscape too have always had this.

Comment 9

[Hallvord R. M. Steen [:hallvors]]

hm - interesting! Both old Presto-based Opera and the most recent IE agree with Gecko. So it seems to be blink/webkit against all others.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Buttons effectively capture events, quite purposefully, like Olli said.

Comment 11

[Hallvord R. M. Steen [:hallvors]]

I can't find this stuff spec'ed anywhere. Anne, can you help?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

IIRC the semi-defined behavior got lost in the specs when moving from HTML4 to HTML. 
HTML4 has stuff like "Buttons created with the BUTTON element function just like buttons created with the INPUT element, but they offer richer rendering possibilities"

Comment 13

[Hallvord R. M. Steen [:hallvors]]

I'm asking because if it's in a spec or on its way into a spec, we'll report Blink/WebKit bugs on fixing it in those engines and contact the site to try to get them to solve it. (Shouldn't be that hard when it fails in several browsers). It does however seem a bit of an anomaly or exception from the usual event model, and this bug confirms it can be a gotcha to developers. Perhaps going the other way would simplify specs and implementations? WebKit-based browsers obviously haven't had enough compat pain to align, so presumably changing it won't hurt us much in terms of compat either. Simplifying things sounds good to me ;)

Comment 14

[Anne (:annevk)]

This is defined as part of https://html.spec.whatwg.org/#activation I think. E.g. in the "run authentic click activation steps" in step 4 it finds the nearest activatable element which would be the button in this scenario, no?

Comment 15

[Hallvord R. M. Steen [:hallvors]]

(In reply to Anne (:annevk) from comment #14)
> This is defined as part of https://html.spec.whatwg.org/#activation I think.
> E.g. in the "run authentic click activation steps" in step 4 it finds the
> nearest activatable element which would be the button in this scenario, no?

Well, we're on to something (thanks a lot - über-spec-explainer) - it will indeed find the button as the "nearest activatable element" (though oddly enough if the markup was <button><span><img></span></button> it seems that algorithm wouldn't - since it only looks at parent??). But I see nothing in those steps that says event.target should therefore be the button? It says 
"Dispatch event (the required click event) at target." - not at the found element e.

Comment 16

[Anne (:annevk)]

It looks recursively at the parent element. Perhaps Ian remembers why we are not adjusting the target of the event. Perhaps we decided at some point to not align with Gecko...

Comment 17

[Hallvord R. M. Steen [:hallvors]]

(In reply to Anne (:annevk) from comment #16)
> It looks recursively at the parent element.

Ah, I overlooked the "and return to the first step" part of step 2.

> Perhaps Ian remembers why we are
> not adjusting the target of the event. Perhaps we decided at some point to
> not align with Gecko...

Look forward to any comments from him :)

Comment 18

[Alwyn Edison Mendonca]

In amazon.in also almost similar kind of issue. I tried to zoom the product but my firefox 36.0.3 behaves very odd like it just flashes screen, sometimes it displays but when I tried to pinch zoom it then it shows menu to save images . Here is the link http://www.amazon.in/dp/B00OK2ZW5W#immersive-view_1426959078049

Comment 19

[Hallvord R. M. Steen [:hallvors]]

(In reply to Alwyn Edison Mendonca from comment #18)
> In amazon.in also almost similar kind of issue. I tried to zoom the product
> but my firefox 36.0.3 behaves very odd like it just flashes screen,
> sometimes it displays but when I tried to pinch zoom it then it shows menu
> to save images .

This issue is different - the site doesn't allow pinch-to-zoom (probably for a good reason: they use a swipe-based effect to scroll through images, and pinch/swipe would become ambiguous unless the programmer was very careful. When you try to pinch-zoom, sometimes you pressed your finger on the screen until the browser shows the long-click menu. I hope this explains everything :)

Comment 20

[Hallvord R. M. Steen [:hallvors]]

I linked to this bug from https://code.google.com/p/chromium/issues/detail?id=418209

Comment 21

[Hallvord R. M. Steen [:hallvors]]

..now reported separately as https://code.google.com/p/chromium/issues/detail?id=519621

Comment 22

[Rick Byers]

>>  But I see nothing in those steps that says event.target should therefore be 
>> the button? It says "Dispatch event (the required click event) at target."
>> - not at the found element e.

> Perhaps Ian remembers why we are
> not adjusting the target of the event. Perhaps we decided at some point to
> not align with Gecko...

It sounds to me like the conclusion on this point is that Gecko's behavior doesn't match the spec, but blink's does.  Right?  The spec and chromium's behavior of dispatching the event to the inner node (instead of the button like Gecko does) makes more sense to me.  If the page has installed a click event handler on the inner node, shouldn't it get a chance to be invoked?

Comment 23

[Anne (:annevk)]

No, Firefox is correct: https://html.spec.whatwg.org/multipage/interaction.html#run-authentic-click-activation-steps

Comment 24

[Anne (:annevk)]

So I already said that and Hallvord pointed out it dispatches at /target/, not /e/. The reason it finds /e/ is to avoid activating multiple ancestor elements with activation behavior.

So at this point I tend to agree with Rick's analysis, though I guess it would be good for bz and smaug to take another look at this.

Comment 25

[Hallvord R. M. Steen [:hallvors]]

The spec does indeed dispatch at /target/ so it seems Gecko's behaviour is wrong. Perhaps I misunderstood Anne earlier. The /e/ element in the spec is just used for the "pre-click activation" and "post-click activation" stuff.

With Gecko's implementation, an event listener added to IMG in this test case will never fire.

Comment 26

[Anne (:annevk)]

The specification is generic for all elements with activation behavior. It seems like bz and smaug say there used to be some special magic for "buttons" (only <button>?) which disappeared. If no other browser has that I don't really see a good reason to introduce it.

Comment 27

[Hallvord R. M. Steen [:hallvors]]

Attachment: 1089326.htm with extra listener on IMG and more logging

Comment 28

[Rick Byers]

Ok, I'll close the chromium bug and remove myself from this bug.  Feel free to re-add me if any chromium-related questions/issues arise again.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

What magical buttons? <button> is <button> and per HTML4 it was supposed to just use the contents of it as the UI.
HTML spec doesn't actually define the behavior, since all it says is
"Let target be the element designated by the user (the target of event)."
In Gecko that target is <button>.

Comment 30

[Hallvord R. M. Steen [:hallvors]]

(In reply to Olli Pettay [:smaug] from comment #29)

> In Gecko that target is <button>.

..and in Blink it might be a descendant of <button>, which is why we have this discussion. I think we need a spec bug to get this clarified.

FWIW I find it weird that if I attach a click listener to a descendant of <button> the listener will never fire even if I attempt to click that element. <button><img onclick="..."></button> simply won't work in Gecko. Isn't that breaking some reasonable assumptions about how things work?

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

No. <button> is <button> (and <button type="button"> != <input type="button">)
it is the event target and the contents of it just represent the UI.
As HTML 4 says
"Buttons created with the BUTTON element function just like buttons created with the INPUT element, but they offer richer rendering possibilities: the BUTTON element may have content."

Comment 32

[Hallvord R. M. Steen [:hallvors]]

That text from HTML 4 is not very precise (it doesn't really say anything about event handling or indicate that the authors considered what we're talking about) and therefore better considered obsoleted by the more detailed stuff in https://html.spec.whatwg.org/multipage/interaction.html#run-authentic-click-activation-steps . If we disagree with that, we should report a bug against HTML5 saying the algorithm should change to set target to the found "activation element" /e/ instead of /target/. As of now, HTML5 says Gecko is wrong and we can't use the handwavy HTML4 text to get away with it, IMHO.

Comment 33

[dtapuska]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #32)
> That text from HTML 4 is not very precise (it doesn't really say anything
> about event handling or indicate that the authors considered what we're
> talking about) and therefore better considered obsoleted by the more
> detailed stuff in
> https://html.spec.whatwg.org/multipage/interaction.html#run-authentic-click-
> activation-steps . If we disagree with that, we should report a bug against
> HTML5 saying the algorithm should change to set target to the found
> "activation element" /e/ instead of /target/. As of now, HTML5 says Gecko is
> wrong and we can't use the handwavy HTML4 text to get away with it, IMHO.

I agree with this; and like to add if a decision needs to be made it needs to be made for the better of the web developer; and as in comment #30 the current Gecko behavior seems confusing. (Unless the developer consults the handy waving HTML4 documentation and interprets it to match what Gecko is doing).

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

How does <button> behave on Edge? IE is like Gecko, right?

Comment 35

[dtapuska]

Edge behaves like Chrome.
IE behaves like Gecko.

Comment 36

[Hallvord R. M. Steen [:hallvors]]

Now we have another interesting case (on a pretty important website FWIW - outlook.com) in bug 1277620. Basically it boils down to 

<button onclick="..."> .. <button onclick="...">X</button></button>

with the page expecting the inner BUTTON element's onclick to be triggered if you click/tap where the inner BUTTON is.

In my testing, Chrome (Win8), Safari (Mac), IE11 (Win8) and Edge (Win10) all do what the site (and per my reading, the HTML spec) expect - consider the inner button the "nearest activatable element" per https://html.spec.whatwg.org/multipage/interaction.html#run-authentic-click-activation-steps and fire that click event on the inner BUTTON element.

Gecko (and Opera Presto - RIP) do NOT fire click on the inner BUTTON.

I recommend we change ;)

Comment 37

[Andrew Overholt [:overholt]]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #36)
>
> I recommend we change ;)

Olli, WDYT?

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

https://github.com/whatwg/html/issues/1567

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

https://github.com/whatwg/html/issues/1568

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v1 with tests

aVisitor.mEvent->mOriginalTarget == this change is a bit controversial but follows the model we have for links
http://searchfox.org/mozilla-central/rev/65bed54efcce67cf04a890f7fe253ccdfa6befdc/dom/base/Element.cpp#3039
And DOMActivate is really super legacy and deprecated.


Let's see how many tests are broken
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b5045f276a45dfe9a31a8f0b155a6691d4e3cc9

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5f400f9815f
with new click event targeting, following the model from blink (which is rather different from what blink devs have told me before!)

Comment 42

[Karl Dubost💡 :karlcow]

Something which seems slightly similar. 
https://webcompat.com/issues/3575

with a 
<button class="btn btn-blue">
  <a href="..."></a>
</button>

Comment 43

[Karl Dubost💡 :karlcow]

Attachment: Testcase with a button containing a link

Comment 44

[Karl Dubost💡 :karlcow]

Because there is another case with:
https://webcompat.com/issues/4909

I created a test case for 

<button>
  <a href="…"></a>
</button>

http://codepen.io/webcompat/pen/QpKPeR
https://bugzilla.mozilla.org/attachment.cgi?id=8844338

# Link does NOT work
  * Firefox Nightly 54.

# Link works
  * Opera Beta 44 / Chrome/57.0.2987.19 
  * Safari 10.0.3 (12602.4.8)


To test: IE/Edge

Comment 47

[Eric Tsai (no more review request accepted)]

Not sure if it also effects: https://webcompat.com/issues/13103

Comment 48

[Mike Taylor [:miketaylr]]

https://github.com/webcompat/web-bugs/issues/14354 seems like another example of this bug:

<li>
  <button aria-label=foo>
    <div>foo</div>
  </button>
</li>

var a = event.target.parentNode.getAttribute('aria-label');

a is undefined in Firefox, and "foo" in Chrome (which the site depends on for menu navigation)

Comment 51

[Julien Wajsberg [:julienw]]

Attachment: testcase with positioned children

Comment 52

[Nolan Lawson]

Would this bug also cause the tooltip for an <img> with a title to not be shown if it's wrapped in a <button>? E.g.

<button>
  <img title="this title is not shown on mouse hover" />
</button>

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

Yes. Internal's of <button> aren't part of hit testing.

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 1, v1

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=779907fb1934aeb1fdf81bc2fe671267d8fbb1f3
remote: recorded changegroup in replication log in 0.008s

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 1, v1, with test

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/b816ed05da791b533daa55e8f8eee3eaa2b174d5
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=b816ed05da791b533daa55e8f8eee3eaa2b174d5
remote: recorded changegroup in replication log in 0.006s

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 2, v1, with test

This changes click event handling to follow similar model what blink has, which is not the spec model.
But blink might be ready to follow the spec.

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/ea7977629d5b9a6515b6b1421741019338d256f2
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea7977629d5b9a6515b6b1421741019338d256f2
remote: recorded changegroup in replication log in 0.045s

Comment 63

[Olli Pettay [:smaug][[e-mail]]]

Attachment: click_handling_marionette_test_fix.diff

Some fixes to pass marionette tests.

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/14ab7f707574031c2342ebe119ec7640d226b9a5
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=14ab7f707574031c2342ebe119ec7640d226b9a5
remote: recorded changegroup in replication log in 0.019s

Comment 65

[Olli Pettay [:smaug][[e-mail]]]

(FWIW, I'm actively fixing the tests and FF UI code this change is breaking)

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

Attachment: button_click_2.diff

I'll split this to smaller pieces and FF UI fixes to separate bugs.

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

Attachment: button_click_3_mod.diff

ok, so this stuff really needs to go in as single push.
I split some testing or FF UI related issues to separate bugs.

There are two major pieces here.
(1) Make button hit testing work like other elements which may have some child nodes. That is what nsHTMLButtonControlFrame.cpp is about and also the changes
to HTMLButtonElement.cpp and HTMLInputElement.cpp to deal with nested submit buttons.

(2) Change to click handling to follow blink model (which is quite close to the spec.) Fire click on the closest ancestor of mousedown/up targets, but "Interactive Content" makes this a tad trickier. See the comment in nsContentUtils.h.
This change is mostly just in EventStateManager::SetClickCount and nsContentUtils::GetCommonAncestorUnderInteractiveContent

Overall the changes are surprisingly small.

Comment 68

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9016468 [details] [diff] [review]
button_click_3_mod.diff

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

I hope that I don't fail finding some wrong changes in HTMLInputElement.cpp though (it's too big to review with Splinter).

::: dom/base/nsContentUtils.cpp
@@ +2659,5 @@
>  }
>  
> +// static
> +nsINode*
> +nsContentUtils::GetCommonAncestorUnderInteractiveContent(nsINode* aNode1,

I'm still not sure what is the standard, though.

This method stops climbing up the tree when it meets an interactive element. That means that when 2 DOM points under same interactive element but at least one of them is in another interactive element, this does not return the ancestor interactive element. Is this intended? E.g.,

<a href="foo">
<button>button</button>
anchor
</a>

mousedown in the <button> but mouseup in the text node after <button>, click event shouldn't be fired on the <a>. Is this intended behavior?

As far as I've tested, Chrome does not fire click event in this case (with pressing Alt, but looks like that Chrome does not fire click event if selection is changed by the drag anyway).
https://jsfiddle.net/d_toybox/mhga9xjd/1/

@@ +2696,5 @@
> +  uint32_t pos1 = parents1.Length();
> +  uint32_t pos2 = parents2.Length();
> +  nsINode* parent = nullptr;
> +  uint32_t len;
> +  for (len = std::min(pos1, pos2); len > 0; --len) {

Why don't you define |len| in the condition of this for loop?
I mean, |for (uint32_t len = ...)|

::: dom/events/EventStateManager.cpp
@@ +4898,4 @@
>      } else if (aEvent->mMessage == eMouseUp) {
> +      aEvent->mClickTarget =
> +        nsContentUtils::GetCommonAncestorUnderInteractiveContent(
> +          mouseContent, mLastLeftMouseDownContent);

Although I believe that this is the reasonable way for realistic cases.

However, if mLastLeftMouseDownContent has been moved to different parent and it *now* has a common interactive content with mouseup content, this fires click event (i.e., even if mousedown content was outside of the interactive content). Is it okay? I guess okay, but perhaps, not declared in any standards.

@@ +4960,5 @@
>      return false;
>    }
>    // If mouse is still over same element, clickcount will be > 1.
>    // If it has moved it will be zero, so no click.
> +  if (!aMouseEvent.mClickCount || !aMouseEvent.mClickTarget) {

Ah, sorry for you rewriting this patch due to my patch.

::: dom/events/test/test_dblclick_explicit_original_target.html
@@ +22,5 @@
>    synthesizeMouse(document.getElementById("display"), 5, 5, { clickCount: 2 });
>  }
>  
> +window.onmousedown = function(event) {
> +  is(event.explicitOriginalTarget.nodeType, Node.TEXT_NODE, "explicitOriginalTarget is a text node");

Why don't you compare event.explicitOriginalTarget and document.getElementById("display").firstChild directly?

@@ +26,5 @@
> +  is(event.explicitOriginalTarget.nodeType, Node.TEXT_NODE, "explicitOriginalTarget is a text node");
> +}
> +
> +window.onmouseup = function(event) {
> +  is(event.explicitOriginalTarget.nodeType, Node.TEXT_NODE, "explicitOriginalTarget is a text node");

Ditto.

@@ +35,2 @@
>  window.ondblclick = function(event) {
> +  is(event.explicitOriginalTarget.nodeType, Node.ELEMENT_NODE, "explicitOriginalTarget is an element node");

Similar to the above, event.explicitOriginalTarget should be document.getElementById("display") in this case?

::: dom/html/HTMLInputElement.cpp
@@ +138,4 @@
>  #define NS_CONTROL_TYPE(bits)  ((bits) & ~( \
>    NS_OUTER_ACTIVATE_EVENT | NS_ORIGINAL_CHECKED_VALUE | NS_NO_CONTENT_DISPATCH | \
> +  NS_ORIGINAL_INDETERMINATE_VALUE | NS_PRE_HANDLE_BLUR_EVENT | \
> +  NS_PRE_HANDLE_INPUT_EVENT | NS_IN_SUBMIT_CLICK))

Are you fixing different bug to include NS_PRE_HANDLE_BLUR_EVENT and NS_PRE_HANDLE_INPUT_EVENT into NS_CONTROL_TYPE? I don't find any reason to fix this now except you are adding (aVisitor.mItemFlags & NS_IN_SUBMIT_CLICK) check below. I'd be clearer if you fix such kind of change is separated to a preceding patch or mentioned in commit message.

::: dom/html/test/test_bug1089326.html
@@ +17,5 @@
> +    var b_rect = b.getBoundingClientRect();
> +    var a = document.getElementById("anchor");
> +    var a_rect = a.getBoundingClientRect();
> +
> +    is(document.elementFromPoint(b_rect.x + 1, b_rect.y + 1), b);

Please add message of this is() like "<button> element should be at {1-1} in its rect" or something.

@@ +18,5 @@
> +    var a = document.getElementById("anchor");
> +    var a_rect = a.getBoundingClientRect();
> +
> +    is(document.elementFromPoint(b_rect.x + 1, b_rect.y + 1), b);
> +    is(document.elementFromPoint(a_rect.x + 1, a_rect.y + 1), a);

Ditto.

@@ +54,5 @@
> +    var span2 = document.getElementById("span2");
> +    expectedTarget = container;
> +    synthesizeMouseAtCenter(span1, { type: "mousedown" });
> +    synthesizeMouseAtCenter(span2, { type: "mouseup" });
> +    is(clickCount, 2, "Should not have got a click event.");

s/not//

@@ +56,5 @@
> +    synthesizeMouseAtCenter(span1, { type: "mousedown" });
> +    synthesizeMouseAtCenter(span2, { type: "mouseup" });
> +    is(clickCount, 2, "Should not have got a click event.");
> +
> +    SimpleTest.finish();

Oh, you don't test synthesizeMouseAtCenter() with button#button and a#anchor. Is it intentional? Or just forgot?

::: widget/MouseEvents.h
@@ +303,5 @@
>    }
>  
> +  // If during mouseup handling we detect that click event might need to be
> +  // dispatched, this is setup to be the target of the click event.
> +  nsCOMPtr<dom::EventTarget> mClickTarget;

Okay, looks like that this is not necessary to copy with Duplicate() nor AssignMouseEventData().

Comment 69

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #68)
>I hope that I don't fail finding some wrong changes in HTMLInputElement.cpp though (it's too big to >review with Splinter).
Sorry about that. I don't ever use splinter and prefer plain raw bugzilla patches for reviewing.


> > +// static
> > +nsINode*
> > +nsContentUtils::GetCommonAncestorUnderInteractiveContent(nsINode* aNode1,
> 
> I'm still not sure what is the standard, though.
> 
Yeah, I'm following blink model here.


> This method stops climbing up the tree when it meets an interactive element.
> That means that when 2 DOM points under same interactive element but at
> least one of them is in another interactive element, this does not return
> the ancestor interactive element. Is this intended? E.g.,
yes

> 
> <a href="foo">
> <button>button</button>
> anchor
> </a>
> 
> mousedown in the <button> but mouseup in the text node after <button>, click
> event shouldn't be fired on the <a>. Is this intended behavior?
yup. At least for now. We may be able to remove the interactive content handling.




> 
> @@ +2696,5 @@
> > +  uint32_t pos1 = parents1.Length();
> > +  uint32_t pos2 = parents2.Length();
> > +  nsINode* parent = nullptr;
> > +  uint32_t len;
> > +  for (len = std::min(pos1, pos2); len > 0; --len) {
> 
> Why don't you define |len| in the condition of this for loop?
> I mean, |for (uint32_t len = ...)|
looks like a copy-paste from other similar method

> 
> ::: dom/events/EventStateManager.cpp
> @@ +4898,4 @@
> >      } else if (aEvent->mMessage == eMouseUp) {
> > +      aEvent->mClickTarget =
> > +        nsContentUtils::GetCommonAncestorUnderInteractiveContent(
> > +          mouseContent, mLastLeftMouseDownContent);
> 
> Although I believe that this is the reasonable way for realistic cases.
> 
> However, if mLastLeftMouseDownContent has been moved to different parent and
> it *now* has a common interactive content with mouseup content, this fires
> click event (i.e., even if mousedown content was outside of the interactive
> content). Is it okay? I guess okay, but perhaps, not declared in any
> standards.
interactive content part isn't defined, but even if we take that out, mousedown/up should fire click when under the same subtree. though, moving mousedown target is indeed unclear. Something to fix in UI Events spec.


> > +  if (!aMouseEvent.mClickCount || !aMouseEvent.mClickTarget) {
> 
> Ah, sorry for you rewriting this patch due to my patch.
no problem. Rebasing was easy.

> 
> ::: dom/events/test/test_dblclick_explicit_original_target.html
> @@ +22,5 @@
> >    synthesizeMouse(document.getElementById("display"), 5, 5, { clickCount: 2 });
> >  }
> >  
> > +window.onmousedown = function(event) {
> > +  is(event.explicitOriginalTarget.nodeType, Node.TEXT_NODE, "explicitOriginalTarget is a text node");
> 
> Why don't you compare event.explicitOriginalTarget and
> document.getElementById("display").firstChild directly?
Just being consistent with the existing test.


> @@ +35,2 @@
> >  window.ondblclick = function(event) {
> > +  is(event.explicitOriginalTarget.nodeType, Node.ELEMENT_NODE, "explicitOriginalTarget is an element node");
> 
> Similar to the above, event.explicitOriginalTarget should be
> document.getElementById("display") in this case?
Yeah, I can change.



> ::: dom/html/HTMLInputElement.cpp
> @@ +138,4 @@
> >  #define NS_CONTROL_TYPE(bits)  ((bits) & ~( \
> >    NS_OUTER_ACTIVATE_EVENT | NS_ORIGINAL_CHECKED_VALUE | NS_NO_CONTENT_DISPATCH | \
> > +  NS_ORIGINAL_INDETERMINATE_VALUE | NS_PRE_HANDLE_BLUR_EVENT | \
> > +  NS_PRE_HANDLE_INPUT_EVENT | NS_IN_SUBMIT_CLICK))
> 
> Are you fixing different bug to include NS_PRE_HANDLE_BLUR_EVENT and
> NS_PRE_HANDLE_INPUT_EVENT into NS_CONTROL_TYPE?
yeah, fixing NS_CONTROL_TYPE to not include those flags

 I don't find any reason to
> fix this now except you are adding (aVisitor.mItemFlags &
> NS_IN_SUBMIT_CLICK) check below. I'd be clearer if you fix such kind of
> change is separated to a preceding patch or mentioned in commit message.
True, it would have been better.

 
> ::: dom/html/test/test_bug1089326.html
> @@ +17,5 @@
> > +    var b_rect = b.getBoundingClientRect();
> > +    var a = document.getElementById("anchor");
> > +    var a_rect = a.getBoundingClientRect();
> > +
> > +    is(document.elementFromPoint(b_rect.x + 1, b_rect.y + 1), b);
> 
> Please add message of this is() like "<button> element should be at {1-1} in
> its rect" or something.
ok


> @@ +56,5 @@
> > +    synthesizeMouseAtCenter(span1, { type: "mousedown" });
> > +    synthesizeMouseAtCenter(span2, { type: "mouseup" });
> > +    is(clickCount, 2, "Should not have got a click event.");
> > +
> > +    SimpleTest.finish();
> 
> Oh, you don't test synthesizeMouseAtCenter() with button#button and
> a#anchor. Is it intentional? Or just forgot?

I can add another test


> >  
> > +  // If during mouseup handling we detect that click event might need to be
> > +  // dispatched, this is setup to be the target of the click event.
> > +  nsCOMPtr<dom::EventTarget> mClickTarget;
> 
> Okay, looks like that this is not necessary to copy with Duplicate() nor
> AssignMouseEventData().
yeah, mClickTarget needs to keep the target only temporarily.

Comment 70

[sarbbottam]

I came across a similar issue Please refer https://codepen.io/sarbbottam/pen/vVJQjy

<button>
  <span>Click Me!</span>
</button>


e.target & e.currentTarget both are BUTTON in firefox. 

But in Chrome, Edge, Safari, Opera e.target is SPAN & e.currentTarget is BUTTON.

For screenshots please refer https://twitter.com/sarbbottam/status/1050984396273082368

Comment 71

[Olli Pettay [:smaug][[e-mail]]]

Attachment: button_click_4.diff

Comment 74

[Andrew Overholt [:overholt]]

Is the lack of interactivity of data:text/html, <button><input type=range></button> this same issue?

Comment 75

[Olli Pettay [:smaug][[e-mail]]]

Yes. And I'm working on this actively. Windows TVw test just doesn't like me.

Comment 76

[Olli Pettay [:smaug][[e-mail]]]

Attachment: click_target_rebase.diff

rebased

Comment 77

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36b949bef798
make <button> hit testing similar to other elements which may have some content, and for click target find the common (interactive) ancestor, r=masayuki

Comment 78

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14478 for changes under testing/web-platform/tests

Comment 79

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 80

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/36b949bef798

Comment 81

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 82

[Wolfgang Rosenauer [:wolfiR]]

If this fix is proven to work in the 66 target I'd like to raise it for possible uplifting to 65 (and possibly 60esr).
It seems to affect enough sites and there might be many more hidden.

Comment 83

[Olli Pettay [:smaug][[e-mail]]]

This is massive behavioral change, scariest change I've done for years (changes how Gecko decides when to dispatch click and where), so we need to be quite sure this doesn't cause major regressions.

Comment 84

[Mike Taylor [:miketaylr]]

(In reply to Olli Pettay [:smaug] (high review load) from comment #83)
> This is massive behavioral change, scariest change I've done for years
> (changes how Gecko decides when to dispatch click and where), so we need to
> be quite sure this doesn't cause major regressions.

Agree we should ride the trains here.

Comment 90

[Olli Pettay [:smaug][[e-mail]]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Changes what gets detected as a click. Traditionally click in Gecko has meant
mousedown and mouseup happening on the same DOM element, but now Gecko doing similar thing as other browsers where click is dispatched on the common ancestor of mousedown target and mouseup target.
[Affects Firefox for Android]: Yes
[Suggested wording]:
[Links (documentation, blog post, etc)]:

Comment 93

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm the following behavior is no longer reproducible. I verified using Fx 66.0b6 on Windows 10 x64, mac 10.14 and Ubuntu 16.04 LTS.

1. Launch Firefox.
2. Go to https://www.linkedin.com and log in.
3. Hover over the following buttons : "About", "Help Center", "Privacy & Terms", "Advertising", "Business Services".

Actual result:

The drop down buttons are not highlighted while hovering.

Expected result:

All the buttons are highlighted while hovering.

Comment 94

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Can someone verify this issue over Android devices in order to close this issue?
Thank you!

Comment 95

[Daniel Holbert [:dholbert]]

I'm guessing comment 94 was because this had "Platform: Other/Android" for some reason but really this affected all platforms. [updating to adjust that]

Anyway, though, I did verify that this is fixed in Nightly on my Android phone. I couldn't use the STR from comment 93 because hovering isn't a thing you can do on Android, so instead I used the testcase from duplicate bug 1342173. In that testcase, the "I am a button" text isn't tappable (i.e. nothing happens when you tap it) in Firefox 65 Release, but it is tappable in Firefox 67 Nightly, on Android.

--> Verified FIXED based on that and comment 93. Also, I filed bug 1529036 on one piece of this that I think remains un-fixed.

Comment 96

[eric]

Just encountered this problem where on FF, when I click the child element of a button, the event.target shows the outer parent button and not the child, whereas on Chrome it shows the child.

Is there a work around for this?

Comment 97

[Julien Wajsberg [:julienw]]

@eric the only workaround is to not use a button. The good news is that this issue is now fixed and should happen in firefox 66, which will be released in about 1 month.
Of course our ESR release won't have it soon, our next ESR is 68 which will be out in October.

Comment 100

[Julien Cristau [:jcristau]]

Coming to this late, but this sounds like something for the developer release notes on MDN rather than the user-facing ones on www.mozilla.org.

Comment 106

[Julien Cristau [:jcristau]]

Restricting comments here to try and cut down on spam.