Last Comment Bug 652752 - 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 dif...
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: Andrew Overholt [:overholt]
data:text/html,<p onclick="alert('cli...
Depends on:
Blocks: 655461
  Show dependency treegraph
Reported: 2011-04-25 22:42 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2011-05-07 03:27 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1.0 (25.02 KB, patch)
2011-04-25 22:42 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review-
Details | Diff | Splinter Review
Patch v2.0 (WIP) (50.32 KB, patch)
2011-04-28 17:01 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
something like this (1.73 KB, patch)
2011-05-06 07:00 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
er, this one (2.75 KB, patch)
2011-05-06 07:01 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
+test and using IsRootOfNativeAnonymousSubtree (3.52 KB, patch)
2011-05-06 08:03 PDT, Olli Pettay [:smaug]
masayuki: review+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-25 22:42:45 PDT
Created attachment 528266 [details] [diff] [review]
Patch v1.0

According to DOM 3 Events:

> 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.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-25 23:05:33 PDT

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 2 Olli Pettay [:smaug] 2011-04-26 03:38:49 PDT
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);
>+  var prefs = Components.classes[";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
Use SpecialPowers object, which is available in mochitests

r- because of and also
because I'm hoping to see more manual or automatic testing.
Comment 3 Olli Pettay [:smaug] 2011-04-26 03:39:45 PDT
And test also <input type="file">
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-28 17:01:34 PDT
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?
Comment 5 Olli Pettay [:smaug] 2011-05-06 07:00:28 PDT
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.
Comment 6 Olli Pettay [:smaug] 2011-05-06 07:01:33 PDT
Created attachment 530608 [details] [diff] [review]
er, this one
Comment 7 Olli Pettay [:smaug] 2011-05-06 08:03:19 PDT
Created attachment 530627 [details] [diff] [review]
+test and using IsRootOfNativeAnonymousSubtree
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-06 16:25:22 PDT
(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.
Comment 9 Olli Pettay [:smaug] 2011-05-06 16:41:42 PDT
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

Landing the patches separately makes it easier to find possible regression-ranges.
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-06 17:10:30 PDT
Comment on attachment 530627 [details] [diff] [review]
+test and using IsRootOfNativeAnonymousSubtree

okay, r=masayuki
Comment 11 Olli Pettay [:smaug] 2011-05-07 03:27:27 PDT

Filed bug 655461 for the followup work.
(Sorry for stealing this bug for my patch.)

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