Closed Bug 1089326 Opened 5 years ago Closed 10 months ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla66
Tracking Status
relnote-firefox --- -
firefox-esr60 - wontfix
firefox65 - wontfix
firefox66 --- fixed

People

(Reporter: alwyne.edison, Assigned: smaug)

References

(Blocks 1 open bug, Regressed 1 open bug, )

Details

(Whiteboard: [country-in] [js] [webcompat:p1][wptsync upstream])

User Story

Business driver: Long-ish tail of sites (Uber account recovery, Twitter, ...)

Attachments

(11 files, 2 obsolete files)

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.
Reproducible on desktop as well with the mobile UA; work's fine in Chrome desktop/mobile.
Status: UNCONFIRMED → NEW
Component: General → Mobile
Ever confirmed: true
Product: Firefox for Android → Tech Evangelism
Version: Firefox 33 → Trunk
Yes works fine on mobile chrome browser. Thanks.
See Also: → 974793
Whiteboard: [country-in]
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.
Something related to function nrWrapper() ?
in the main page.
Flags: needinfo?(hsteen)
Whiteboard: [country-in] → [country-in] [js]
Ok. Is it possible to fix this issue??
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/
Component: Mobile → DOM: Events
Flags: needinfo?(hsteen)
Product: Tech Evangelism → Core
Summary: Selecting different views of the product is not possible in www.myntra.com → event.target refers to parent BUTTON instead of clicked IMG when IMG is child of BUTTON (Selecting different views of the product is not possible in www.myntra.com)
Attached file TC - 1089326.htm
I'm surprised if this isn't a known issue already. Boris?
Flags: needinfo?(bzbarsky)
This is, or at least was, expected behavior. (HTML4 sort of wanted this behavior). 
Gecko and IIRC Netscape too have always had this.
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.
Buttons effectively capture events, quite purposefully, like Olli said.
Flags: needinfo?(bzbarsky)
I can't find this stuff spec'ed anywhere. Anne, can you help?
Flags: needinfo?(annevk)
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"
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 ;)
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?
Flags: needinfo?(annevk)
(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.
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...
(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 :)
Flags: needinfo?(ian)
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
(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 :)
>>  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?
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.
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.
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.
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.
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>.
(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?
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."
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.
(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).
How does <button> behave on Edge? IE is like Gecko, right?
Edge behaves like Chrome.
IE behaves like Gecko.
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 ;)
(In reply to Hallvord R. M. Steen [:hallvors] from comment #36)
>
> I recommend we change ;)

Olli, WDYT?
Flags: needinfo?(bugs)
Flags: needinfo?(bugs)
QA Contact: bugs
Assignee: nobody → bugs
QA Contact: bugs
Attached patch v1 with tests (obsolete) — Splinter Review
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
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!)
Something which seems slightly similar. 
https://webcompat.com/issues/3575

with a 
<button class="btn btn-blue">
  <a href="..."></a>
</button>
Whiteboard: [country-in] [js] → [country-in] [js] [webcompat]
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
Duplicate of this bug: 1276143
Duplicate of this bug: 1050625
Flags: webcompat?
See Also: → 1397981
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)
Priority: -- → P2
Summary: event.target refers to parent BUTTON instead of clicked IMG when IMG is child of BUTTON (Selecting different views of the product is not possible in www.myntra.com) → event.target refers to parent BUTTON instead of clicked IMG when IMG is child of BUTTON (inactive interative content inside <button>)
Duplicate of this bug: 1433887
Attachment #8844338 - Attachment description: button-test.html → Testcase with a button containing a link
Whiteboard: [country-in] [js] [webcompat] → [country-in] [js] [webcompat:p1]
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>
Yes. Internal's of <button> aren't part of hit testing.
Whiteboard: [country-in] [js] [webcompat:p1] → [country-in] [js] [webcompat:p2]
Duplicate of this bug: 1459379
Duplicate of this bug: 1431804
Flags: webcompat? → webcompat+
Duplicate of this bug: 1435331
Whiteboard: [country-in] [js] [webcompat:p2] → [country-in] [js] [webcompat:p1]
Attached patch part 1, v1 (obsolete) — Splinter Review
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
Attachment #8772586 - Attachment is obsolete: true
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
Attachment #8996079 - Attachment is obsolete: true
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
Some fixes to pass marionette tests.
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
(FWIW, I'm actively fixing the tests and FF UI code this change is breaking)
I'll split this to smaller pieces and FF UI fixes to separate bugs.
Depends on: 1498375
Depends on: 1498379
Depends on: 1498380
Depends on: 1498381
Depends on: 1498383
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.
Attachment #9016468 - Flags: review?(masayuki)
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().
Attachment #9016468 - Flags: review?(masayuki) → review+
(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.
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
Duplicate of this bug: 1507456
Depends on: 1511387
Depends on: 1511388
Is the lack of interactivity of data:text/html, <button><input type=range></button> this same issue?
Yes. And I'm working on this actively. Windows TVw test just doesn't like me.
Depends on: 1512259
Depends on: 1512264
rebased
Pushed by opettay@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14478 for changes under testing/web-platform/tests
Whiteboard: [country-in] [js] [webcompat:p1] → [country-in] [js] [webcompat:p1][wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/36b949bef798
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Upstream PR merged
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.
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.
(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.
Depends on: 1514613
Duplicate of this bug: 1513457
Depends on: 1514791
Depends on: 1514832
Duplicate of this bug: 1515514
Duplicate of this bug: 1516416
Duplicate of this bug: 1342173
Depends on: 1516798
Flags: needinfo?(bugs)

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)]:

relnote-firefox: --- → ?
Duplicate of this bug: 1526035
Duplicate of this bug: 1372408

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.

Depends on: 1525255

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

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.

Status: RESOLVED → VERIFIED
OS: Android → All
Hardware: Other → All

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?

@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.

Duplicate of this bug: 1533900
Depends on: 1536151
No longer depends on: 1536151
Regressions: 1539695
Regressions: 1556976

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.

You need to log in before you can comment on or make changes to this bug.