1420589 - [Pointer Event] Using the same target of mouse events and touch events to fire pointer events

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement, P2)

Description

[Ming-Chou Shih [:stone]]

Spec [1] says that we should dispatch mouse events to the same target as the mapped pointer events.

[1] https://w3c.github.io/pointerevents/#compatibility-mapping-with-mouse-events

Comment 1

[Ming-Chou Shih [:stone]]

Some testing results on Chrome
1. touch events are dispatched to the same target as the corresponding pointer events
2. setPointerCapture only applies to pointer events but not touch events
3. when the DOM tree is modified while handling pointerdown, touch events are dispatched to the same target as touchstart, pointer events are dispatched to the new hit target.

Comment 2

[Ming-Chou Shih [:stone]]

Regarding pointer events, Edge also behaves the same as Chrome.

Comment 3

[Ming-Chou Shih [:stone]]

We could also reuse the event target of pointer events (from touch) for touch events so that we can avoid redundant hit test.

Comment 4

[Ming-Chou Shih [:stone]]

Attachment: Part1: Separate the logic of finding the event target for touch events into a function

Comment 5

[Ming-Chou Shih [:stone]]

Attachment: Part2: Define a helper class to store the target of pointer events

Comment 6

[Ming-Chou Shih [:stone]]

Attachment: Part3: Merge PresShell::HandlePositionedEvent to PresShell::HandleEvent

We need to decide the final event target before dispatching pointer events.

Comment 7

[Ming-Chou Shih [:stone]]

Attachment: Part4: Using mouse or touch event to do hit test and then generate pointer events with the same target.

This patch does
1. Separate the logic to make sure all touch events are going to the same document.
   To dispatch pointer events to the original target found by hit test.
   Then adjust the event target for touch events.
2. Process pending pointer capture before doing hit test
   To skip doing hit test if there is a capturing element.

Comment 8

[Ming-Chou Shih [:stone]]

Attachment: Part5: Test multiple touch points on different documents

Make sure we dispatch touch events and pointer events to the correct target.

Comment 9

[Ming-Chou Shih [:stone]]

Attachment: Part6: Dispatch pointer events to the capturing target even it's frame is destroyed.

Comment 10

[Ming-Chou Shih [:stone]]

Comment on attachment 8935302 [details] [diff] [review]
Part5: Test multiple touch points on different documents

This patch tests multiple touch points to make sure touch events and pointer events are dispatched to correct target.
1. on the different iframes within the same parent document.
2. on a iframe and its parent document.
3. similar to case 2 with reverse order of touch points.

Comment 11

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

Comment on attachment 8935294 [details] [diff] [review]
Part1: Separate the logic of finding the event target for touch events into a function

This was hard code move to review. I hope I didn't miss anything.

>diff --git a/layout/base/TouchManager.h b/layout/base/TouchManager.h
>--- a/layout/base/TouchManager.h
... 
>+  // Perform hit test and setup the event targets for touchstart. Other touch
>+  // evnets are dispatched to the same target as touchstart.
events

Comment 12

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

Comment on attachment 8935298 [details] [diff] [review]
Part3: Merge PresShell::HandlePositionedEvent to PresShell::HandleEvent

A bit scary to change mCurrentEventContent and 
mCurrentEventFrame handling (unless I'm missing something). But in theory
this should be fine. But be prepared for regressions.

Comment 13

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

Comment on attachment 8935301 [details] [diff] [review]
Part4: Using mouse or touch event to do hit test and then generate pointer events with the same target.

yikes, this is complicated and large. Could you try to split this to smaller pieces. Code moves (like creating GetShellForTouchEvent) should really be in separate patch.

Comment 14

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

Comment on attachment 8935302 [details] [diff] [review]
Part5: Test multiple touch points on different documents

Could you explain in the tests better what the results are.
Something that we don't leak nodes or touch objects from different documents to others etc.
Right now there isn't really any documentation what is expected to happen.

Comment 15

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

Comment on attachment 8935303 [details] [diff] [review]
Part6: Dispatch pointer events to the capturing target even it's frame is destroyed.

So we don't handle the case when capturing content is in a document which doesn't have a presshell at all, but I think we have issues with such document anyhow, so fine.

Comment 16

[Ming-Chou Shih [:stone]]

Attachment: Revise PointerEventHandler utilities.

Comment 17

[Ming-Chou Shih [:stone]]

Attachment: Separate the logic to get shell for touch events into a function.

Comment 18

[Ming-Chou Shih [:stone]]

Attachment: Keep those touch points that are not in the same document so that we can use them to dispatch pointer events.

Comment 19

[Ming-Chou Shih [:stone]]

Attachment: Using mouse or touch event to do hit test and then generate pointer events with the same target.

Comment 20

[Ming-Chou Shih [:stone]]

Attachment: Test multiple touch points on different documents

Added more descriptions in the test cases.

Comment 21

[Ming-Chou Shih [:stone]]

Attachment: Keep those touch points that are not in the same document so that we can use them to dispatch pointer events.

Comment 22

[Ming-Chou Shih [:stone]]

Attachment: Separate the logic to get shell for touch events into a function.

Comment 23

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

Comment on attachment 8936024 [details] [diff] [review]
Test multiple touch points on different documents

+  We dispatch all touch events to the same document. We stop dispatching touch
+  events to a target if we can't find any ancestor document that is the same as
+  the document of the existed target. We check the points of the touch event in
+  reverse order. That means we choose the document of iframe2 as our targeted
+  document. We won't dispatch touch events to the document of iframe1 nor the
+  parent document of iframe1 and iframe2.
+
+  We dispatch pointer events to the hit targets even when there aren't in the
+  same document. This test expects that pointer events are dispatched to the div
+  element and the iframe document.
So do other browsers have the same behavior.



s/same as the document of the existed target./same as the document of the existing target./

Comment 24

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

Comment on attachment 8936019 [details] [diff] [review]
Revise PointerEventHandler utilities.





> 
>-    frame = PointerEventHandler::GetPointerCapturingFrame(frame, aEvent);
>+    nsIFrame* pointerCapturingFrame =
>+      PointerEventHandler::GetPointerCapturingFrame(aEvent);
>+
>+    bool isPointerCapturing = !!pointerCapturingFrame;
>+    if (isPointerCapturing) {
>+      frame = pointerCapturingFrame;
>+    }
isPointerCapturing just makes the code harder to read.
Why not
if (pointerCapturingFrame) {
  frame = pointerCapturingFrame;
}

Comment 25

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

Comment on attachment 8936118 [details] [diff] [review]
Keep those touch points that are not in the same document so that we can use them to dispatch pointer events.

Ok, so we try to guarantee that suppressed touches aren't visible in the touch event. At least better to add assertions to TouchEvent to ensure none of the touch lists contain suppressed touch objects.

>+  // Suppress those points that are not in the same document. We need to keep
>+  // those touch points until we dispatch the corresponding pointer events. The
>+  // suppressed touch points are removed in TouchManager::PreHandleEvent.
>+  static nsIFrame* SuppressInvalidPoints(WidgetTouchEvent* aEvent);
I don't understand what this method returns. Its name doesn't hint it at all.
Please document and change the method name too.

Comment 26

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

Sorry, this all is complicated so it will take several iterations to review this.

Comment 27

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

Comment on attachment 8936023 [details] [diff] [review]
Using mouse or touch event to do hit test and then generate pointer events with the same target.

>+      } else {
>+        // We didn't hit test for other touch events. Spec doesn't mention that
>+        // all pointer events should be dispatched to the same target as their
>+        // corresponding touch events. Call PresShell::HandleEvent so that we do
>+        // hit test for pointer events.
And I assume this is the behavior what other browsers have too. If so, r+.


I know I asked for smaller patches, but could you also upload a patch containing all the changes in this bug.
I'd like to see that to understand the big picture better.

Comment 28

[Ming-Chou Shih [:stone]]

Attachment: bug-1420589-fix-all.patch

Comment 29

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] (won't be in Austin) from comment #23)
> Comment on attachment 8936024 [details] [diff] [review]
> Test multiple touch points on different documents
> 
> +  We dispatch all touch events to the same document. We stop dispatching
> touch
> +  events to a target if we can't find any ancestor document that is the
> same as
> +  the document of the existed target. We check the points of the touch
> event in
> +  reverse order. That means we choose the document of iframe2 as our
> targeted
> +  document. We won't dispatch touch events to the document of iframe1 nor
> the
> +  parent document of iframe1 and iframe2.
> +
> +  We dispatch pointer events to the hit targets even when there aren't in
> the
> +  same document. This test expects that pointer events are dispatched to
> the div
> +  element and the iframe document.
> So do other browsers have the same behavior.
IIRC, Chrome behaves so. I'll check it with Edge later.

Comment 30

[Ming-Chou Shih [:stone]]

Attachment: Keep those touch points that are not in the same document so that we can use them to dispatch pointer events (v2)

Comment 31

[Ming-Chou Shih [:stone]]

Comment on attachment 8936375 [details] [diff] [review]
Keep those touch points that are not in the same document so that we can use them to dispatch pointer events (v2)

Please help to take a look if this is better. Thanks.

Comment 32

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] (won't be in Austin) from comment #27)
> I know I asked for smaller patches, but could you also upload a patch
> containing all the changes in this bug.
> I'd like to see that to understand the big picture better.

Please check attachment 8936249 [details] [diff] [review]. Thanks.

Comment 33

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

Comment on attachment 8936375 [details] [diff] [review]
Keep those touch points that are not in the same document so that we can use them to dispatch pointer events (v2)

Could you still add MOZ_DIAGNOSTIC_ASSERTs to TouchEvent.cpp to ensure
that Touches, TargetTouches nor ChangedTouches return lists containing suppressed Touch objects.

+    // We should remove all suppressed touch instances in
+    // TouchManager::PreHandleEvent.
+    MOZ_ASSERT(!touch->mIsTouchEventSuppressed);
Make this MOZ_DIAGNOSTIC_ASSERT.

>+/* static */ nsIFrame*
>+TouchManager::SuppressInvalidPointsAndGetTargetedFrame(WidgetTouchEvent* aEvent)
>+{
>+  MOZ_ASSERT(aEvent);
>+
>+  if (!aEvent || aEvent->mMessage != eTouchStart) {
>+    // All touch events except for touchstart use a captured target.
>+    return nullptr;
>+  }
>+
>   // if this is a continuing session, ensure that all these events are
>   // in the same document by taking the target of the events already in
>   // the capture list
>   nsCOMPtr<nsIContent> anyTarget;
>   if (aEvent->mTouches.Length() > 1) {
>     anyTarget = TouchManager::GetAnyCapturedTouchTarget();
>   }
> 
>-  nsPoint eventPoint;
>-  nsIFrame* frame = aFrame;
>+  nsIFrame* frame = nullptr;
>   for (int32_t i = aEvent->mTouches.Length(); i; ) {
>     --i;
>     dom::Touch* touch = aEvent->mTouches[i];
>+    if (TouchManager::HasCapturedTouch(touch->Identifier())) {
>+      continue;
>+    }
> 
>-    int32_t id = touch->Identifier();
>-    if (!TouchManager::HasCapturedTouch(id)) {
>-      // find the target for this touch
>-      eventPoint = nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent,
>-                                                        touch->mRefPoint,
>-                                                        aFrame);
>-      nsIFrame* target = FindFrameTargetedByInputEvent(aEvent,
>-                                                       aFrame,
>-                                                       eventPoint,
>-                                                       flags);
>-      if (target && !anyTarget) {
>-        target->GetContentForEvent(aEvent, getter_AddRefs(anyTarget));
>-        while (anyTarget && !anyTarget->IsElement()) {
>-          anyTarget = anyTarget->GetParent();
>-        }
>-        touch->SetTarget(anyTarget);
>-      } else {
>-        nsIFrame* newTargetFrame = nullptr;
>-        for (nsIFrame* f = target; f;
>-             f = nsLayoutUtils::GetParentOrPlaceholderForCrossDoc(f)) {
>-          if (f->PresContext()->Document() == anyTarget->OwnerDoc()) {
>-            newTargetFrame = f;
>-            break;
>-          }
>-          // We must be in a subdocument so jump directly to the root frame.
>-          // GetParentOrPlaceholderForCrossDoc gets called immediately to
>-          // jump up to the containing document.
>-          f = f->PresShell()->GetRootFrame();
>+    MOZ_ASSERT(touch->mTarget);
>+    nsCOMPtr<nsIContent> targetContent = do_QueryInterface(touch->GetTarget());
>+    nsIFrame* targetFrame = targetContent->GetPrimaryFrame();
>+    if (targetFrame && !anyTarget) {
>+      anyTarget = targetContent;
>+    } else {
>+      nsIFrame* newTargetFrame = nullptr;
>+      for (nsIFrame* f = targetFrame; f;
>+           f = nsLayoutUtils::GetParentOrPlaceholderForCrossDoc(f)) {
>+        if (f->PresContext()->Document() == anyTarget->OwnerDoc()) {
>+          newTargetFrame = f;
>+          break;
>         }
>-
>-        // if we couldn't find a target frame in the same document as
>-        // anyTarget, remove the touch from the capture touch list, as
>-        // well as the event->mTouches array. touchmove events that aren't
>-        // in the captured touch list will be discarded
>-        if (!newTargetFrame) {
>-          aEvent->mTouches.RemoveElementAt(i);
>-        } else {
>-          target = newTargetFrame;
>-          nsCOMPtr<nsIContent> targetContent;
>-          target->GetContentForEvent(aEvent, getter_AddRefs(targetContent));
>-          while (targetContent && !targetContent->IsElement()) {
>-            targetContent = targetContent->GetParent();
>-          }
>-          touch->SetTarget(targetContent);
>-        }
>+        // We must be in a subdocument so jump directly to the root frame.
>+        // GetParentOrPlaceholderForCrossDoc gets called immediately to
>+        // jump up to the containing document.
>+        f = f->PresShell()->GetRootFrame();
>       }
>-      if (target) {
>-        frame = target;
>+      // if we couldn't find a target frame in the same document as
>+      // anyTarget, remove the touch from the capture touch list, as
>+      // well as the event->mTouches array. touchmove events that aren't
>+      // in the captured touch list will be discarded
>+      if (!newTargetFrame) {
>+        touch->mIsTouchEventSuppressed = true;
>+      } else {
>+        targetFrame = newTargetFrame;
>       }
>-    } else {
>-      // This touch is an old touch, we need to ensure that is not
>-      // marked as changed and set its target correctly
>-      touch->mChanged = false;
>-
>-      RefPtr<dom::Touch> oldTouch = TouchManager::GetCapturedTouch(id);
>-      if (oldTouch) {
>-        touch->SetTarget(oldTouch->mTarget);
>+      targetFrame->GetContentForEvent(aEvent, getter_AddRefs(targetContent));
>+      while (targetContent && !targetContent->IsElement()) {
>+        targetContent = targetContent->GetParent();
>       }
>+      touch->SetTarget(targetContent);
>+    }
>+    if (targetFrame) {
>+      frame = targetFrame;
>     }
>   }
>-  return FindFrameTargetedByInputEvent(aEvent, frame, eventPoint, flags);
>+  return frame;
FWIW, this code would have been a lot easier to review without coding style change - I mean there isn't 
really need to change whether to have continue; early or whether have nested ifs.
I hope I didn't miss anything while going through the code moves

Comment 34

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

Hmm, I'm missing still something...
Which patch adds TouchManager::SetupTarget?
And why do we need to split that to SuppressInvalidPointsAndGetTargetedFrame?

Since
         frame = TouchManager::SetupTarget(aEvent->AsTouchEvent(), frame);
+        if (nsIFrame* newFrame =
+            TouchManager::SuppressInvalidPointsAndGetTargetedFrame(
+              aEvent->AsTouchEvent())) {
+          frame = newFrame;
+        }
looks a bit weird.
First we get the target frame, but then, for some reason that isn't the right one after all and
we call SuppressInvalidPointsAndGetTargetedFrame and reset the frame.

Comment 35

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

(I'm just trying to figure out how to make this code easier to follow)

Comment 36

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] (won't be in Austin) from comment #34)
> Hmm, I'm missing still something...
> Which patch adds TouchManager::SetupTarget?
> And why do we need to split that to SuppressInvalidPointsAndGetTargetedFrame?
> 
> Since
>          frame = TouchManager::SetupTarget(aEvent->AsTouchEvent(), frame);
> +        if (nsIFrame* newFrame =
> +            TouchManager::SuppressInvalidPointsAndGetTargetedFrame(
> +              aEvent->AsTouchEvent())) {
> +          frame = newFrame;
> +        }
> looks a bit weird.
> First we get the target frame, but then, for some reason that isn't the
> right one after all and
> we call SuppressInvalidPointsAndGetTargetedFrame and reset the frame.

The original implementation does hit test and removes invalid points in the same function. I separate them to send pointer events to the target found by hit test and then suppress invalid points.
attachment 8936194 [details] [diff] [review] separates the original implementation of finding event target for touch events as function 'SetupTarget'.
attachment 8936375 [details] [diff] [review] separates the part of removing invalid points as 'SuppressInvalidPointsAndGetTargetedFrame'.
attachment 8936023 [details] [diff] [review] adds the logic to dispatch pointer events between 'SetupTarget' and 'SuppressInvalidPointsAndGetTargetedFrame'.
Sorry for the bad organization of these patches.

Comment 37

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de24cd764353d2d7483295505a6a79d575abc95b

Comment 38

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4ea073a014197065238cf4a655a8d2683442849

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16a0ca0a2afa
Part1: Separate the logic of finding the event target for touch events into a function. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9b2476de5d
Part2: Define a helper class to store the target of pointer events. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f606cef407
Part3: Merge PresShell::HandlePositionedEvent to PresShell::HandleEvent. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/481886826377
Part4: Revise PointerEventHandler utilities. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a568d63c332
Part5: Separate the logic to get shell for touch events into a function. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/62f76b10e91f
Part6: Keep those touch points that are not in the same document so that we can use them to dispatch pointer events.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8b3173ded00
Part7: Using mouse or touch event to do hit test and then generate pointer events with the same target. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/49491891f6b1
Part8: Test multiple touch points on different documents. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2155af8d88c3
Part9: Dispatch pointer events to the capturing target even it's frame is destroyed. r=smaug.

Comment 40

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/16a0ca0a2afa
https://hg.mozilla.org/mozilla-central/rev/ce9b2476de5d
https://hg.mozilla.org/mozilla-central/rev/f9f606cef407
https://hg.mozilla.org/mozilla-central/rev/481886826377
https://hg.mozilla.org/mozilla-central/rev/7a568d63c332
https://hg.mozilla.org/mozilla-central/rev/62f76b10e91f
https://hg.mozilla.org/mozilla-central/rev/b8b3173ded00
https://hg.mozilla.org/mozilla-central/rev/49491891f6b1
https://hg.mozilla.org/mozilla-central/rev/2155af8d88c3