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)

(Reporter)

Description

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

Updated

6 years ago
Component: DOM: Events → Event Handling
(Reporter)

Comment 1

6 years ago
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.
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-
And test also <input type="file">
(Reporter)

Comment 4

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

Updated

6 years ago
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
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.
Created attachment 530608 [details] [diff] [review]
er, this one
Attachment #530606 - Attachment is obsolete: true
Created attachment 530627 [details] [diff] [review]
+test and using IsRootOfNativeAnonymousSubtree
Attachment #530627 - Flags: review?(masayuki)
(Reporter)

Comment 8

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

Comment 10

6 years ago
Comment on attachment 530627 [details] [diff] [review]
+test and using IsRootOfNativeAnonymousSubtree

okay, r=masayuki
Attachment #530627 - Flags: review?(masayuki) → review+
(Reporter)

Updated

6 years ago
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
Blocks: 655461
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.