655461 - incorrect click event handling in native anonymous content

Status: NEW

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=652752#c9

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.0

There are a couple of problems about click event and dblclick event dispatching code in ESM.

The main issue is, these events have multiple event target if a deepest target in subtree but we don't handle it correctly. E.g., assume following tree:

+ E0 + E1
     + E2 - A0 + A1
               + A2 - B0 + B1
                         + B2

E11 has a subtree which has A0, A1 and A2. A2 has a nested subtree which has B0, B1 and B2.

If B1 is clicked, the deepest click event target is B0 (XBL) or B1 (<svg:use> or native anonymous tree). Furthermore, a click event is fired on A2 for subtree A (except when subtree A is XBL. A0 is the target at that time.) and also E2 for main tree. By this mechanism, each tree event handler can handle the events.
# Note that ESM doesn't need to think about the difference between XBL subtree and others. The redirect is done by another point.

The first issue is, when mousedown element is A1 and mouseup element A2, both A1 and A2 (of course A0 too) shouldn't be fired click event *but* E2 should be fired it because the subtree A looks like an element from the parent tree. Similarly, if mousedown element is B1 and mouseup element is B2, both A2 and E2 should be fired click events. And also mousedown element is A1 and mouseup element is B1, only E2 should be fired a click event.

dblclick event is similar to this. You should replace "mousedown" with "first click", "mouseup" with "second click" and "click"  with "dblclick" in the previous paragraph. Then, the paragraph will explain the behavior of dblclick event.

For dispatching to correct target, we need to cache last mousedown content for click event and last click content for dblclick event. They are named mMouseDownContent and mClickedContent in this patch. And we need to compute the each level event target. This is done by GetAncestorMouseEventTargets() in the patch.

Let me explain the actual handling process:

First of all, OnMouseDown() computes clickCount. I'm going to explain about this later. If the computed clickCount is 0, the mousedown event doesn't cause click event. At this time, it clears mMouseDownContent and mClickedContent because they will be never used. If the computed clickCount is 1, it clears mClickedContent because it will be never used. And the mMouseDownContent stores the mousedown event's explicitOriginalTarget. Otherwise, so, when clickCount is 2 or larger, OnMouseDown() checks whether the next click event can be a continuous action of previous click. If there is no click event target between previous click event target and current mousedown event target, it resets clickCount with 1 and clears mClickedContent. So, the next click event becomes first click event on the content.

Second, OnMouseUp() checks whether the mousedown event and current mouseup event can dispatch a click event. A nearest common ancestor event target is the deepest click event target. If there is no common event target, it clears mClickCount to 0 for preventing click event. Otherwise, it sets the target to mNextClickEventTarget. On the other hand, mMouseDownContent is cleared this time because it will be never used. Furthermore, if the mClickCount is larger than 1, it checks whether the next click event target and the previous clicked content have common event target. If there is a common target and mClickCount is 2, the click event causes dblclick event too. Therefore, it sets the common target to mNextDblClickEventTarget. Otherwise, it resets 1 to the mClickCount because the next click event is first click event on the content.

Finally, CheckForAndDispatchClick() is called by PostHandleEvent() of mouseup event. If clickCount is 1 or larger, it dispatches a click event to an element for mNextClickEventTarget (mNextClickEventTarget may be a textnode). If it succeeded, it set mNextClickEventTarget to mClickedContent. If clickCount is 2, it also dispatches a dblclick event to an element for mNextDblClickEventTarget. If this succeeded, it set mNextDblClickEventTarget to mClickedContent. And it clears mNextClickEventTarget and mNextDblClickEventTarget.

By this new logic, we can dispatch to different elements for click event and dblclick event (e.g., mousedown element is A1 and mouseup element is B2 case).

And this patch also cleans up the clickCount handling. At OnMouseDown(), aEvent->clickCount was set by the platform. So, it's not unconsidered about content level. In other words, even if aEvent->clickCount is 2, the previous click event may be dispatched on another content. Therefore, OnMouseDown() computes actual clickCount first. It has the clickCount of previous mouse event in mLatestNativeClickCount. If coming event's clickCount is larger than the previous and mClickEvent isn't NULL (i.e., click event is fired by the previous mouse button events and the target hasn't been gone), increases mClickCount. Otherwise, it sets 1 to mClickCount. And it sets mClickCount to aEvent->clickCount.

By this change, all tests need to dispatch click event twice for testing double click event. For example:

synthesizeMouse(e, 5, 5, {clickCount: 1});
synthesizeMouse(e, 5, 5, {clickCount: 2});

I think that this is better test because the first click handling code might break the double click event's behavior.

Finally, let be explain about the stored contents.

ContentRemoved() handles the mutation events. It checks whether the removed content is ancestor (or same) content of mMouseDownContent, mClickedContent, mNextClickEventTarget and mNextDblClickEventTarget. If there is a common click event target between the each member and parent of the removing content, sets it to the member. Otherwise, just clears it.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

> E11 has a subtree which has A0, A1 and A2. A2 has a nested subtree which has B0, B1 and B2.

E11 -> E2.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.0.1

renamed new test files for consistency with other test files.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 530889 [details] [diff] [review]
Patch v1.0.1


>+  // Be aware, mNextDblClickEventTarget might be removed by this click event.
>+  // For detecting it, we restore mNextDblClickEventTarget.
>+  dblClickTarget.swap(mNextDblClickEventTarget);
>+  rv = presShell->HandleEventWithTarget(&event2, dblClickFrame,
>+                                        dblClickElement, aStatus);
>+  dblClickTarget.swap(mNextDblClickEventTarget);
>+
>+  if (NS_FAILED(rv) || !dblClickTarget) {
>+    NS_ASSERTION(NS_SUCCEEDED(rv), "PresShell::HandleEventWithTarget() failed");
>+    mClickCount = 0;
>+    mClickedContent = nsnull;
>+    return rv;
>+  }
>+
>+  mClickedContent = dblClickTarget;
Why does mClickedContent need to be set here?


>+nsEventStateManager::ClickEventManager::ContentRemoved(nsIDocument* aDocument,
>+                                                       nsIContent*y aContent)
I need to still review this.
But even before that, could you add some comments why and when we're updating
the member variables.




>@@ -4452,16 +4710,20 @@ nsEventStateManager::ContentRemoved(nsID
>     mDragOverContent = nsnull;
>   }
> 
>   if (mLastMouseOverElement &&
>       nsContentUtils::ContentIsDescendantOf(mLastMouseOverElement, aContent)) {
>     // See bug 292146 for why we want to null this out
>     mLastMouseOverElement = nsnull;
>   }
>+
>+  mLeftClickEventManager.ContentRemoved(aDocument, aContent);
>+  mMiddleClickEventManager.ContentRemoved(aDocument, aContent);
>+  mRightClickEventManager.ContentRemoved(aDocument, aContent);
Would be interesting to know if this affects to some performance tests.
At least need to be careful that ClickEventManager.ContentRemoved() doesn't 
do any extra work (including as few implicit addref/release calls as possible).


> }
> 
> PRBool
> nsEventStateManager::EventStatusOK(nsGUIEvent* aEvent)
> {
>   return !(aEvent->message == NS_MOUSE_BUTTON_DOWN &&
>       static_cast<nsMouseEvent*>(aEvent)->button == nsMouseEvent::eLeftButton  && 
>       !sNormalLMouseEventInProcess);
>diff --git a/content/events/src/nsEventStateManager.h b/content/events/src/nsEventStateManager.h
>--- a/content/events/src/nsEventStateManager.h
>+++ b/content/events/src/nsEventStateManager.h
>@@ -259,17 +259,16 @@ protected:
>                            nsIContent* aTargetContent,
>                            nsWeakFrame& aTargetFrame);
>   /**
>    * Update the initial drag session data transfer with any changes that occur
>    * on cloned data transfer objects used for events.
>    */
>   void UpdateDragDataTransfer(nsDragEvent* dragEvent);
> 
>-  nsresult SetClickCount(nsPresContext* aPresContext, nsMouseEvent *aEvent, nsEventStatus* aStatus);
>   nsresult CheckForAndDispatchClick(nsPresContext* aPresContext, nsMouseEvent *aEvent, nsEventStatus* aStatus);
>   void EnsureDocument(nsPresContext* aPresContext);
>   void FlushPendingEvents(nsPresContext* aPresContext);
> 
>   /**
>    * The phases of HandleAccessKey processing. See below.
>    */
>   typedef enum {
>@@ -430,22 +429,70 @@ protected:
>   // an <area> of an image map this is the image. (bug 289667)
>   nsCOMPtr<nsIContent> mGestureDownFrameOwner;
>   // State of keys when the original gesture-down happened
>   PRPackedBool mGestureDownShift;
>   PRPackedBool mGestureDownControl;
>   PRPackedBool mGestureDownAlt;
>   PRPackedBool mGestureDownMeta;
> 
>-  nsCOMPtr<nsIContent> mLastLeftMouseDownContent;
>-  nsCOMPtr<nsIContent> mLastLeftMouseDownContentParent;
>-  nsCOMPtr<nsIContent> mLastMiddleMouseDownContent;
>-  nsCOMPtr<nsIContent> mLastMiddleMouseDownContentParent;
>-  nsCOMPtr<nsIContent> mLastRightMouseDownContent;
>-  nsCOMPtr<nsIContent> mLastRightMouseDownContentParent;
>+  struct ClickEventManager
>+  {
>+    // mClickCount is clickCount of mouse events.  This is computed by
>+    // OnMouseDown() from native event's clickCount.
>+    PRUint32 mClickCount;
>+    // mLatestNativeClickCount is the last clickCount of native mouse event.
>+    // This is set by OnMouseClick().
>+    PRUint32 mLatestNativeClickCount;
>+    // mMouseDownContent is last mousedown event's explicitOriginalTarget.
It is not, if I read code correctly. The thing called "explicitOriginalTarget"
is never in anonymous content.  (I know the terminology is strange)

I need to still review tests.

In general I like this, but I wonder if this will cause some regressions because
anonymous content expects different event handling.
Have you tested for example <audio>/<video>? Or does this affect how and when
click is dispatched when <select size="1"><option>1<option>2<option>3<option4<option5<option>6</select>
is scrolled by clicking the scrollbar?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

For example, if you click on 
http://mozilla.pettay.fi/moztests/innerhtml_nonsimple_nostyle_test.html page,
does the test perhaps slow down, or does ContentRemoved show up in the
profiles? (If you don't have a Mac, I could profile using Shark)

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(In reply to comment #4)
> >+  // Be aware, mNextDblClickEventTarget might be removed by this click event.
> >+  // For detecting it, we restore mNextDblClickEventTarget.
> >+  dblClickTarget.swap(mNextDblClickEventTarget);
> >+  rv = presShell->HandleEventWithTarget(&event2, dblClickFrame,
> >+                                        dblClickElement, aStatus);
> >+  dblClickTarget.swap(mNextDblClickEventTarget);
> >+
> >+  if (NS_FAILED(rv) || !dblClickTarget) {
> >+    NS_ASSERTION(NS_SUCCEEDED(rv), "PresShell::HandleEventWithTarget() failed");
> >+    mClickCount = 0;
> >+    mClickedContent = nsnull;
> >+    return rv;
> >+  }
> >+
> >+  mClickedContent = dblClickTarget;
> Why does mClickedContent need to be set here?

Oh, right.

> >@@ -4452,16 +4710,20 @@ nsEventStateManager::ContentRemoved(nsID
> >     mDragOverContent = nsnull;
> >   }
> > 
> >   if (mLastMouseOverElement &&
> >       nsContentUtils::ContentIsDescendantOf(mLastMouseOverElement, aContent)) {
> >     // See bug 292146 for why we want to null this out
> >     mLastMouseOverElement = nsnull;
> >   }
> >+
> >+  mLeftClickEventManager.ContentRemoved(aDocument, aContent);
> >+  mMiddleClickEventManager.ContentRemoved(aDocument, aContent);
> >+  mRightClickEventManager.ContentRemoved(aDocument, aContent);
> Would be interesting to know if this affects to some performance tests.
> At least need to be careful that ClickEventManager.ContentRemoved() doesn't 
> do any extra work (including as few implicit addref/release calls as
> possible).

Excuse me, what should I do?

> >+  struct ClickEventManager
> >+  {
> >+    // mClickCount is clickCount of mouse events.  This is computed by
> >+    // OnMouseDown() from native event's clickCount.
> >+    PRUint32 mClickCount;
> >+    // mLatestNativeClickCount is the last clickCount of native mouse event.
> >+    // This is set by OnMouseClick().
> >+    PRUint32 mLatestNativeClickCount;
> >+    // mMouseDownContent is last mousedown event's explicitOriginalTarget.
> It is not, if I read code correctly. The thing called
> "explicitOriginalTarget"
> is never in anonymous content.  (I know the terminology is strange)

When I clicked on text node, explicitOriginalTarget is the text node.
When I clicked on <input type="text">, explicitOriginalTarget is always the input element rather than anonymous <div> or text node.

Okay, I'll modify it.

> Have you tested for example <audio>/<video>?

I don't find any regressions on <video> with controls.

> Or does this affect how and when
> click is dispatched when <select
> size="1"><option>1<option>2<option>3<option4<option5<option>6</select>
> is scrolled by clicking the scrollbar?

I don't find any problems too.

(In reply to comment #5)
> For example, if you click on 
> http://mozilla.pettay.fi/moztests/innerhtml_nonsimple_nostyle_test.html page,
> does the test perhaps slow down, or does ContentRemoved show up in the
> profiles? (If you don't have a Mac, I could profile using Shark)

How to register the debug symbols to Shark? I tested on debug build and recorded the Time Profile of firefox-bin, but all Libraries are Unknown Library and only addresses of the methods are listed.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(In reply to comment #6)
> (In reply to comment #5)
> > For example, if you click on 
> > http://mozilla.pettay.fi/moztests/innerhtml_nonsimple_nostyle_test.html page,
> > does the test perhaps slow down, or does ContentRemoved show up in the
> > profiles? (If you don't have a Mac, I could profile using Shark)
> 
> How to register the debug symbols to Shark? I tested on debug build and
> recorded the Time Profile of firefox-bin, but all Libraries are Unknown
> Library and only addresses of the methods are listed.

FYI: Even if I clicked very very many times during the test, I don't see the performance regression.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

>> >@@ -4452,16 +4710,20 @@ nsEventStateManager::ContentRemoved(nsID
>> >     mDragOverContent = nsnull;
>> >   }
>> > 
>> >   if (mLastMouseOverElement &&
>> >       nsContentUtils::ContentIsDescendantOf(mLastMouseOverElement, aContent)) {
>> >     // See bug 292146 for why we want to null this out
>> >     mLastMouseOverElement = nsnull;
>> >   }
>> >+
>> >+  mLeftClickEventManager.ContentRemoved(aDocument, aContent);
>> >+  mMiddleClickEventManager.ContentRemoved(aDocument, aContent);
>> >+  mRightClickEventManager.ContentRemoved(aDocument, aContent);
>> Would be interesting to know if this affects to some performance tests.
>> At least need to be careful that ClickEventManager.ContentRemoved() doesn't 
>> do any extra work (including as few implicit addref/release calls as
>> possible).
> 
> Excuse me, what should I do?

And another possible design is, we should create the instance when we need them first time. Furthermore, they could be shared between all ESM instances.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Patch v1.1

test builds:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5b8c67740e7f

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #6)
> How to register the debug symbols to Shark? I tested on debug build and
> recorded the Time Profile of firefox-bin, but all Libraries are Unknown
> Library and only addresses of the methods are listed.
Don't profile with debug builds (it won't give any real performance numbers).
Just a normal build should be enough.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #8)
> And another possible design is, we should create the instance when we need
> them first time. Furthermore, they could be shared between all ESM instances.

I'm not sure if that helps. (Well, I'm not yet sure there is a performance
regression).
But actually, seems like we should optimize nsEventStateManager::ContentRemoved
anyway. It does ContentIsDescendantOf way too many times, and that
can show up in profiles (depends on testcase though).
I'll try to write a worst case test for ESM::ContentRemoved.

So, perhaps we could do the optimization work in a different bug.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Question: after clicking (just once) do mMouseDownContent, mClickedContent,
mNextClickEventTarget and/or mNextDblClickEventTarget point to some value
basically forever? 
(Just wondering what kind of performance testcase to write.)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible test

I don't have a Mac right now with me. I can profile this later this week when I have one.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(In reply to comment #12)
> Question: after clicking (just once) do mMouseDownContent, mClickedContent,
> mNextClickEventTarget and/or mNextDblClickEventTarget point to some value
> basically forever? 
> (Just wondering what kind of performance testcase to write.)

mMouseDownContent is released on mouseup event.
mNextClickEventTarget and mNextDblClickEventTarget are released at CheckForAndDispatchClick() which is called from ESM::PostHandle() at mouseup.

Unfortunately, mClickedContent may be grabbed forever since final click (or dblclick) event on the ESM. The possible solution may be:

1. Release when ESM loses focus.
2. Make the new members as static variables, then, they can be released at every mouse button event on all ESMs.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I think we should do 2.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 532136 [details] [diff] [review]
Patch v1.1

(And sorry for the terrible delay. This hasn't been a top-priority at all.)

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Resetting assignee which I don't work on in this several months.