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

NEW
Assigned to

Status

()

P2
normal
4 years ago
29 days ago

People

(Reporter: alwyne.edison, Assigned: smaug)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Other
Android
Points:
---
Dependency tree / graph
Bug Flags:
webcompat +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [country-in] [js] [webcompat:p1], URL)

User Story

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

Attachments

(10 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 2

4 years ago
Yes works fine on mobile chrome browser. Thanks.
See Also: → bug 974793
Whiteboard: [country-in]
(Reporter)

Comment 3

4 years ago
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]
(Reporter)

Comment 5

4 years ago
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)
Created attachment 8539656 [details]
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 ;)

Comment 14

4 years ago
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.

Comment 16

4 years ago
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)
(Reporter)

Comment 18

4 years ago
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 :)

Comment 22

3 years ago
>>  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 24

3 years ago
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.

Comment 26

3 years ago
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.
Created attachment 8647470 [details]
1089326.htm with extra listener on IMG and more logging

Comment 28

3 years ago
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.

Comment 33

3 years ago
(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?

Comment 35

3 years ago
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
Created attachment 8772586 [details] [diff] [review]
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
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]
Created attachment 8844338 [details]
Testcase with a button containing a link
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

Updated

2 years ago
Duplicate of this bug: 1276143

Updated

2 years ago
Duplicate of this bug: 1050625
Flags: webcompat?
See Also: → bug 1397981
Blocks: 1406505
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

Updated

10 months ago
Duplicate of this bug: 1397981

Updated

10 months ago
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>)
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1433887
Attachment #8844338 - Attachment description: button-test.html → Testcase with a button containing a link
Created attachment 8946239 [details]
testcase with positioned children
Whiteboard: [country-in] [js] [webcompat] → [country-in] [js] [webcompat:p1]
User Story: (updated)

Comment 52

7 months ago
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]
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1459379
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1431804
Flags: webcompat? → webcompat+
Duplicate of this bug: 843003
Duplicate of this bug: 1435331
Whiteboard: [country-in] [js] [webcompat:p2] → [country-in] [js] [webcompat:p1]
Created attachment 8996079 [details] [diff] [review]
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
Attachment #8772586 - Attachment is obsolete: true
Created attachment 8996402 [details] [diff] [review]
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
Attachment #8996079 - Attachment is obsolete: true
Created attachment 8996408 [details] [diff] [review]
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
Duplicate of this bug: 1468008
Duplicate of this bug: 1468008
Created attachment 9014071 [details] [diff] [review]
click_handling_marionette_test_fix.diff

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)
Created attachment 9016459 [details] [diff] [review]
button_click_2.diff

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
Created attachment 9016468 [details] [diff] [review]
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.
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.

Comment 70

a month ago
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
Created attachment 9016933 [details] [diff] [review]
button_click_4.diff
Comment hidden (spam)
You need to log in before you can comment on or make changes to this bug.