Click event isn't fired if mousedown event and mouseup event are fired on different textnode of same element

RESOLVED FIXED

Status

()

Core
Event Handling
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: masayuki, Assigned: smaug)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

Created attachment 528266 [details] [diff] [review]
Patch v1.0

According to DOM 3 Events:

http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#event-type-click

> The click event may be preceded by the mousedown and mouseup events on the same element, disregarding changes between other node types (e.g., text nodes)

This is similar to bug 556493 and bug 534833.
Attachment #528266 - Flags: review?(Olli.Pettay)
Component: DOM: Events → Event Handling
STR:

1. Show the URL:
> data:text/html,<p onclick="alert('click event fired');" style="border: 1px solid">here is first text node.<br>here is second text node.</p>
2. Mousedown on first line.
3. Move cursor to second line.
4. Mouseup on the second line.

Then, you don't see alert. It should be shown.
(Assignee)

Comment 2

6 years ago
Comment on attachment 528266 [details] [diff] [review]
Patch v1.0


>-nsIContent* GetParentContentForMouseTarget(nsIContent* aContent)
>+nsIContent* GetMouseEventTargetElement(nsIContent* aContent)
> {
>-  return aContent && (aContent->IsInNativeAnonymousSubtree() ||
>-                      aContent->IsNodeOfType(nsINode::eTEXT)) ?
>-           aContent->GetParent() : nsnull;
>+  if (!aContent) {
>+    return nsnull;
>+  }
>+
>+  if (aContent->IsInNativeAnonymousSubtree()) {
>+    aContent = aContent->GetBindingParent();
>+  } else if (aContent->IsNodeOfType(nsINode::eTEXT)) {
>+    aContent = aContent->GetParent();
>+  }
>+
>+  NS_ASSERTION(aContent->IsElement(), "The result must be an element");
>+
>+  return aContent;
> }
> 
> nsresult
> nsEventStateManager::SetClickCount(nsPresContext* aPresContext,
>                                    nsMouseEvent *aEvent,
>                                    nsEventStatus* aStatus)
> {
>   nsCOMPtr<nsIContent> mouseContent;
>   mCurrentTarget->GetContentForEvent(aPresContext, aEvent, getter_AddRefs(mouseContent));
>-  nsIContent* mouseContentParent = GetParentContentForMouseTarget(mouseContent);
>+  nsIContent* mouseElement = GetMouseEventTargetElement(mouseContent);
> 
>   switch (aEvent->button) {
>   case nsMouseEvent::eLeftButton:
>     if (aEvent->message == NS_MOUSE_BUTTON_DOWN) {
>-      mLastLeftMouseDownContent = mouseContent;
>-      mLastLeftMouseDownContentParent = mouseContentParent;
>+      mLastLeftMouseDownElement = mouseElement;
>     } else if (aEvent->message == NS_MOUSE_BUTTON_UP) {
>-      if (mLastLeftMouseDownContent == mouseContent ||
>-          mLastLeftMouseDownContentParent == mouseContent ||
>-          mLastLeftMouseDownContent == mouseContentParent) {
>+      if (mLastLeftMouseDownElement == mouseElement) {
>         aEvent->clickCount = mLClickCount;
>         mLClickCount = 0;
>       } else {
>         aEvent->clickCount = 0;
>       }

Ah, this would be great. I hope this works in all cases. Need to tests and think about a bit.

Have you checked that scrollbars work still correctly? For example the case
when you mouse down on a scrollbar button and then move mouse to be over the scrollbar
and release. I think that should work, but better to test.
Also, please make sure clicks work with <svg:use>'s cloned tree.
Although, this patch shouldn't make change to that.
Please test also <audio> and <video> controls.



>+  window.addEventListener("contextmenu", killContextMenu, false);
>+  window.addEventListener("click", clickHandler, false);
>+
>+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+
>+  var prefs = Components.classes["@mozilla.org/preferences-service;1"].
>+                         getService(Components.interfaces.nsIPrefBranch2);
>+  prefs.setBoolPref("general.autoScroll", false);
>+  prefs.setBoolPref("dom.event.contextmenu.enabled", true);

Please don't add new tests which use netscape.security.PrivilegeManager.enablePrivilege
Use SpecialPowers object, which is available in mochitests
https://developer.mozilla.org/en/SpecialPowers


r- because of netscape.security.PrivilegeManager.enablePrivilege and also
because I'm hoping to see more manual or automatic testing.
Attachment #528266 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 3

6 years ago
And test also <input type="file">
Created attachment 528996 [details] [diff] [review]
Patch v2.0 (WIP)

Thank you for your advice. I realized a couple of bugs about click event dispatching.

1. We shouldn't dispatch click event to an HTML element even when mousedown and mouseup are fired on different anonymous elements. We shouldn't dispatch a click event on the content element.

For example, when I mousedown on a button of scrollbar and mouseup on the thumb, if click event was fired on a content's element, that may cause unexpected behavior for users. E.g., even though user tried to use scrollbar, he/she might click a link under the scrollbar.

For solving this issue, we need to store the original mousedown event target content rather than its binding parent until mouseup event.

2. XBL and <svg:use> can be nested. I'm not sure about <svg:use> element's behavior but I'm sure for XBL. Even if mousedown and mouseup were fired on different anonymous elements, we should dispatch a click event on nearest their common ancestor binding parent.

3. clickCount should be recomputed at mousedown. Even if the platform thinks the click event causes dblclick event, the first click event and second click event may be fired on different elements. Then, we shouldn't dispatch dblclick event.

For solving this issue, we need to store the last clicked element.

By these changes, the logic becomes more complicated. Now, I create ClickEventManager for ESM's subclass. It's created 3 instances for each mouse button.

Now, I'm creating additional testcases. But I have trouble about <svg:use> cases. Current trunk and my patched build dispatch click event on <svg:use>. However, internally, the event is dispatched to the actual event target in the instance tree. E.g., if clicked on rect element of <svg:g><svg:rect onclick="alert("");"/></svg:g>, the onclick handler was kicked. I'm not sure this is intentional behavior or not. I *think* this is reasonable, but I couldn't confirm this on SVG spec. How about you, smaug?
Attachment #528266 - Attachment is obsolete: true
Summary: Click event isn't fired if mousedown event and mouseup event are fired on different textnode of same element → Click event isn't fired if mousedown event and mouseup event are fired on different textnode of same element or different node in anonymous subtree
(Assignee)

Comment 5

6 years ago
Created attachment 530606 [details] [diff] [review]
something like this

Pushed to tryserver. This needs some more tests, since this fixes
also case like
data:text/html,<input onclick="alert('click event fired');" value="mousedown on border, mouseup here......................................................" size="50" style="border: blue solid 20px;">

Masayuki, do you think we could take a simple patch like this first, and
if there are other cases to fix, fix those in followup bugs.
(Assignee)

Comment 6

6 years ago
Created attachment 530608 [details] [diff] [review]
er, this one
Attachment #530606 - Attachment is obsolete: true
(Assignee)

Comment 7

6 years ago
Created attachment 530627 [details] [diff] [review]
+test and using IsRootOfNativeAnonymousSubtree
Attachment #530627 - Flags: review?(masayuki)
(In reply to comment #5)
> Masayuki, do you think we could take a simple patch like this first, and
> if there are other cases to fix, fix those in followup bugs.

Oh... I'm going to finish my patch soon. Would you check it first? That is little complicated but fixes all bugs which I found. If you think we should land the simple patch first, I'll look it.
(Assignee)

Comment 9

6 years ago
If we could land the simple patch first, and then (in a followup bug) the 
complicated one.
The simple patch does fix this bug, but the complicated can handle other cases
too.

Landing the patches separately makes it easier to find possible regression-ranges.
Comment on attachment 530627 [details] [diff] [review]
+test and using IsRootOfNativeAnonymousSubtree

okay, r=masayuki
Attachment #530627 - Flags: review?(masayuki) → review+
Assignee: masayuki → Olli.Pettay
Summary: Click event isn't fired if mousedown event and mouseup event are fired on different textnode of same element or different node in anonymous subtree → Click event isn't fired if mousedown event and mouseup event are fired on different textnode of same element
(Assignee)

Updated

6 years ago
Blocks: 655461
(Assignee)

Comment 11

6 years ago
http://hg.mozilla.org/mozilla-central/rev/571d5a04678d

Filed bug 655461 for the followup work.
(Sorry for stealing this bug for my patch.)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.