Closed Bug 1420589 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: stone, Assigned: stone)

References

(Regressed 1 open bug)

Details

Attachments

(11 files, 4 obsolete files)

12.67 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.12 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.66 KB, patch
smaug
: review+
Details | Diff | Splinter Review
13.80 KB, patch
smaug
: review-
Details | Diff | Splinter Review
11.95 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.98 KB, patch
smaug
: review+
Details | Diff | Splinter Review
28.21 KB, patch
smaug
: review+
Details | Diff | Splinter Review
15.75 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.44 KB, patch
smaug
: review+
Details | Diff | Splinter Review
81.87 KB, patch
Details | Diff | Splinter Review
17.52 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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
Assignee: nobody → sshih
Blocks: 1414336
Priority: -- → P2
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.
Regarding pointer events, Edge also behaves the same as Chrome.
We could also reuse the event target of pointer events (from touch) for touch events so that we can avoid redundant hit test.
Summary: [Pointer Event] Using the same target of mouse events to fire pointer events → [Pointer Event] Using the same target of mouse events and touch events to fire pointer events
Depends on: 1421480
Depends on: 1421482
We need to decide the final event target before dispatching pointer events.
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.
Make sure we dispatch touch events and pointer events to the correct target.
Attachment #8935294 - Flags: review?(bugs)
Attachment #8935295 - Flags: review?(bugs)
Attachment #8935298 - Flags: review?(bugs)
Attachment #8935301 - Flags: review?(bugs)
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.
Attachment #8935302 - Flags: review?(bugs)
Attachment #8935303 - Flags: review?(bugs)
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
Attachment #8935294 - Flags: review?(bugs) → review+
Attachment #8935295 - Flags: review?(bugs) → review+
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.
Attachment #8935298 - Flags: review?(bugs) → review+
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.
Attachment #8935301 - Flags: review?(bugs)
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.
Attachment #8935302 - Flags: review?(bugs) → review-
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.
Attachment #8935303 - Flags: review?(bugs) → review+
Attachment #8935301 - Attachment is obsolete: true
Added more descriptions in the test cases.
Attachment #8936019 - Flags: review?(bugs)
Attachment #8936023 - Flags: review?(bugs)
Attachment #8936024 - Flags: review?(bugs)
Attachment #8936118 - Flags: review?(bugs)
Attachment #8936194 - Flags: review?(bugs)
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./
Attachment #8936024 - Flags: review?(bugs) → review+
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;
}
Attachment #8936019 - Flags: review?(bugs) → review+
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.
Attachment #8936118 - Flags: review?(bugs) → review-
Sorry, this all is complicated so it will take several iterations to review this.
Attachment #8936194 - Flags: review?(bugs) → review+
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.
Attachment #8936023 - Flags: review?(bugs) → review+
(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 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.
Attachment #8936375 - Flags: review?(bugs)
(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 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
Attachment #8936375 - Flags: review?(bugs) → review+
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.
Flags: needinfo?(sshih)
(I'm just trying to figure out how to make this code easier to follow)
(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.
Flags: needinfo?(sshih)
Pushed by sshih@mozilla.com:
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.
Depends on: 1426388
Depends on: 1426527
Depends on: 1426728
Depends on: 1446771
Depends on: 1447993
Regressions: 1573117
You need to log in before you can comment on or make changes to this bug.