Bug 1877815 - Make `PresShell::EventHandler::DispatchPrecedingPointerEvent` update event target after dispatching a pointer event r=smaug

The reason of the bug is, no `mouseup` event is fired after the `pointerup`
event listener removed the target.  Therefore, `nsIFrame::HandleRelease()` does
not run and nobody cleans up the drag state of the `nsFrameSelection`.

This is caused by that `PresShell::EventHandler::DispatchPrecedingPointerEvent()`
updates only event target frame of the following mouse event target if the
pointer event target was removed from the tree, however, the frame may not be
ready.  In this case, `PresShell::GetCurrentContent()` will clear both current
event target content and frame because its composed document (`nullptr`) never
matches with the document for the `PresShell`.  Therefore, it needs to update
the target too.

This patch makes all developers won't create similar bugs, this encapsulate
`EventTargetData::mContent` and `EventTargetData::mFrame` to make their setters
clean up or automatically check the relation.

Differential Revision: https://phabricator.services.mozilla.com/D201053

layout/base/PresShell.cpp

@@ -9,12 +9,15 @@
 #include "mozilla/PresShell.h"
 
 #include "Units.h"
+#include "mozilla/RefPtr.h"
+#include "mozilla/dom/AncestorIterator.h"
 #include "mozilla/dom/FontFaceSet.h"
 #include "mozilla/dom/ElementBinding.h"
 #include "mozilla/dom/LargestContentfulPaint.h"
 #include "mozilla/dom/PerformanceMainThread.h"
 #include "mozilla/dom/HTMLAreaElement.h"
 #include "mozilla/ArrayUtils.h"
+#include "mozilla/Assertions.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/AutoRestore.h"
 #include "mozilla/CaretAssociationHint.h"
@@ -7185,9 +7188,9 @@ nsresult PresShell::EventHandler::HandleEventUsingCoordinates(
   // content is not a descendant of the capturing content.
   if (capturingContent && !pointerCapturingElement &&
       (PresShell::sCapturingContentInfo.mRetargetToElement ||
-       !eventTargetData.mFrame->GetContent() ||
+       !eventTargetData.GetFrameContent() ||
        !nsContentUtils::ContentIsCrossDocDescendantOf(
-           eventTargetData.mFrame->GetContent(), capturingContent))) {
+           eventTargetData.GetFrameContent(), capturingContent))) {
     // A check was already done above to ensure that capturingContent is
     // in this presshell.
     NS_ASSERTION(capturingContent->OwnerDoc() == GetDocument(),
@@ -7198,13 +7201,13 @@ nsresult PresShell::EventHandler::HandleEventUsingCoordinates(
     }
   }
 
-  if (NS_WARN_IF(!eventTargetData.mFrame)) {
+  if (NS_WARN_IF(!eventTargetData.GetFrame())) {
     return NS_OK;
   }
 
   // Suppress mouse event if it's being targeted at an element inside
   // a document which needs events suppressed
-  if (MaybeDiscardOrDelayMouseEvent(eventTargetData.mFrame, aGUIEvent)) {
+  if (MaybeDiscardOrDelayMouseEvent(eventTargetData.GetFrame(), aGUIEvent)) {
     return NS_OK;
   }
 
@@ -7214,7 +7217,7 @@ nsresult PresShell::EventHandler::HandleEventUsingCoordinates(
   // active document.  This way content can get mouse events even when mouse
   // is over the chrome or outside the window.
   if (eventTargetData.MaybeRetargetToActiveDocument(aGUIEvent) &&
-      NS_WARN_IF(!eventTargetData.mFrame)) {
+      NS_WARN_IF(!eventTargetData.GetFrame())) {
     return NS_OK;
   }
 
@@ -7379,7 +7382,7 @@ bool PresShell::EventHandler::ComputeEventTargetFrameAndPresShellAtEventPoint(
   nsIFrame* targetFrame =
       GetFrameToHandleNonTouchEvent(aRootFrameToHandleEvent, aGUIEvent);
   aEventTargetData->SetFrameAndComputePresShell(targetFrame);
-  return !!aEventTargetData->mFrame;
+  return !!aEventTargetData->GetFrame();
 }
 
 bool PresShell::EventHandler::DispatchPrecedingPointerEvent(
@@ -7404,7 +7407,7 @@ bool PresShell::EventHandler::DispatchPrecedingPointerEvent(
   // and do hit test for each point.
   nsIFrame* targetFrame = aGUIEvent->mClass == eTouchEventClass
                               ? aFrameForPresShell
-                              : aEventTargetData->mFrame;
+                              : aEventTargetData->GetFrame();
 
   if (aPointerCapturingContent) {
     aEventTargetData->mOverrideClickTarget =
@@ -7420,16 +7423,16 @@ bool PresShell::EventHandler::DispatchPrecedingPointerEvent(
     }
 
     targetFrame = aPointerCapturingContent->GetPrimaryFrame();
-    aEventTargetData->mFrame = targetFrame;
+    aEventTargetData->SetFrameAndContent(targetFrame, aPointerCapturingContent);
   }
 
   AutoWeakFrame weakTargetFrame(targetFrame);
-  AutoWeakFrame weakFrame(aEventTargetData->mFrame);
-  nsCOMPtr<nsIContent> content(aEventTargetData->mContent);
+  AutoWeakFrame weakFrame(aEventTargetData->GetFrame());
+  nsCOMPtr<nsIContent> content(aEventTargetData->GetContent());
   RefPtr<PresShell> presShell(aEventTargetData->mPresShell);
   nsCOMPtr<nsIContent> targetContent;
   PointerEventHandler::DispatchPointerFromMouseOrTouch(
-      presShell, aEventTargetData->mFrame, content, aGUIEvent,
+      presShell, aEventTargetData->GetFrame(), content, aGUIEvent,
       aDontRetargetEvents, aEventStatus, getter_AddRefs(targetContent));
 
   // If the target frame is alive, the caller should keep handling the event
@@ -7453,10 +7456,10 @@ bool PresShell::EventHandler::DispatchPrecedingPointerEvent(
     return false;
   }
 
-  // XXX Why don't we reset aEventTargetData->mContent here?
-  aEventTargetData->mFrame = targetContent->GetPrimaryFrame();
+  aEventTargetData->SetFrameAndContent(targetContent->GetPrimaryFrame(),
+                                       targetContent);
   aEventTargetData->mPresShell = PresShell::GetShellForEventTarget(
-      aEventTargetData->mFrame, targetContent);
+      aEventTargetData->GetFrame(), aEventTargetData->GetContent());
 
   // If new target PresShel is not found, we cannot keep handling the event.
   return !!aEventTargetData->mPresShell;
@@ -11771,12 +11774,61 @@ void PresShell::EventHandler::EventTargetData::SetContentForEventFromFrame(
   MOZ_ASSERT(mFrame);
   mContent = nullptr;
   mFrame->GetContentForEvent(aGUIEvent, getter_AddRefs(mContent));
+  AssertIfEventTargetContentAndFrameContentMismatch(aGUIEvent);
 }
 
 nsIContent* PresShell::EventHandler::EventTargetData::GetFrameContent() const {
   return mFrame ? mFrame->GetContent() : nullptr;
 }
 
+void PresShell::EventHandler::EventTargetData::
+    AssertIfEventTargetContentAndFrameContentMismatch(
+        const WidgetGUIEvent* aGUIEvent) const {
+#ifdef DEBUG
+  if (!mContent || !mFrame || !mFrame->GetContent()) {
+    return;
+  }
+
+  // If we know the event, we can compute the target correctly.
+  if (aGUIEvent) {
+    nsCOMPtr<nsIContent> content;
+    mFrame->GetContentForEvent(aGUIEvent, getter_AddRefs(content));
+    MOZ_ASSERT(mContent == content);
+    return;
+  }
+
+  // Otherwise, we can check only whether mContent is an inclusive ancestor
+  // element or not.
+  if (!mContent->IsElement()) {
+    MOZ_ASSERT(mContent == mFrame->GetContent());
+    return;
+  }
+  const Element* const closestInclusiveAncestorElement =
+      [&]() -> const Element* {
+    for (const nsIContent* const content :
+         mFrame->GetContent()->InclusiveFlatTreeAncestorsOfType<nsIContent>()) {
+      if (content->IsElement()) {
+        return content->AsElement();
+      }
+    }
+    return nullptr;
+  }();
+  if (closestInclusiveAncestorElement == mContent) {
+    return;
+  }
+  if (closestInclusiveAncestorElement->IsInNativeAnonymousSubtree() &&
+      (mContent == closestInclusiveAncestorElement
+                       ->FindFirstNonChromeOnlyAccessContent())) {
+    return;
+  }
+  NS_WARNING(nsPrintfCString("mContent=%s", ToString(*mContent).c_str()).get());
+  NS_WARNING(nsPrintfCString("mFrame->GetContent()=%s",
+                             ToString(*mFrame->GetContent()).c_str())
+                 .get());
+  MOZ_ASSERT(mContent == mFrame->GetContent());
+#endif  // #ifdef DEBUG
+}
+
 bool PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument(
     WidgetGUIEvent* aGUIEvent) {
   MOZ_ASSERT(aGUIEvent);

layout/base/PresShell.h

@@ -2177,8 +2177,41 @@ class PresShell final : public nsStubDocumentObserver,
       Document* GetDocument() const {
         return mPresShell ? mPresShell->GetDocument() : nullptr;
       }
+
+      /**
+       * Return content of the frame if and only if a frame is set.
+       * I.e., this may return non-element node even when GetContent() returns
+       * an element node.
+       */
       nsIContent* GetFrameContent() const;
 
+      nsIFrame* GetFrame() const { return mFrame; }
+      nsIContent* GetContent() const { return mContent; }
+
+      /**
+       * Set the event target content and the topmost frame at the event point.
+       * This checks whether the relation is correct if aContent is not nullptr.
+       * If you set aGUIEvent, the check is done with strict way, but otherwise,
+       * it checks whether aContent is a proper inclusive ancestor of
+       * mFrame->GetContent() or not.
+       */
+      void SetFrameAndContent(nsIFrame* aFrame, nsIContent* aContent = nullptr,
+                              const WidgetGUIEvent* aGUIEvent = nullptr) {
+        mFrame = aFrame;
+        mContent = aContent ? aContent : GetFrameContent();
+        AssertIfEventTargetContentAndFrameContentMismatch(aGUIEvent);
+      }
+
+      /**
+       * Set the event target content and clear the frame.
+       */
+      void SetContent(nsIContent* aContent) {
+        mContent = aContent;
+        if (mFrame && GetFrameContent() != aContent) {
+          mFrame = nullptr;
+        }
+      }
+
       /**
        * MaybeRetargetToActiveDocument() tries retarget aGUIEvent into
        * active document if there is.  Note that this does not support to
@@ -2221,10 +2254,24 @@ class PresShell final : public nsStubDocumentObserver,
        */
       void UpdateWheelEventTarget(WidgetGUIEvent* aGUIEvent);
 
+     private:
+      void AssertIfEventTargetContentAndFrameContentMismatch(
+          const WidgetGUIEvent* aGUIEvent = nullptr) const;
+
+     public:
       RefPtr<PresShell> mPresShell;
-      nsIFrame* mFrame = nullptr;
-      nsCOMPtr<nsIContent> mContent;
       nsCOMPtr<nsIContent> mOverrideClickTarget;
+
+     private:
+      nsIFrame* mFrame = nullptr;
+      // mContent is the event target content for mFrame->GetContent().
+      // This may be nullptr even if mFrame is not nullptr.
+      // This may be an ancestor element of mFrame->GetContent() or native
+      // anonymous root content parent.
+      // This may be not an ancestor element of mFrame->GetContent() if
+      // mFrame->GetContentForEvent() returns such element. E.g., clicking in
+      // <area>, mContent is the <area> but mFrame->GetContent() is an <img>.
+      nsCOMPtr<nsIContent> mContent;
     };
 
     /**
@@ -2801,7 +2848,7 @@ class PresShell final : public nsStubDocumentObserver,
         MOZ_DIAGNOSTIC_ASSERT(!mEventHandler.mCurrentEventInfoSetter);
         mEventHandler.mCurrentEventInfoSetter = this;
         mEventHandler.mPresShell->PushCurrentEventInfo(
-            aEventTargetData.mFrame, aEventTargetData.mContent);
+            aEventTargetData.GetFrame(), aEventTargetData.GetContent());
       }
       ~AutoCurrentEventInfoSetter() {
         mEventHandler.mPresShell->PopCurrentEventInfo();

testing/web-platform/meta/pointerevents/compat/pointerevent_compat-mouse-events-when-removing-nodes.html.ini

@@ -1,14 +1,4 @@
 [pointerevent_compat-mouse-events-when-removing-nodes.html]
-  [Compat mouse events with node removal on pointerup]
-    expected: FAIL
-
   [Compat mouse events with no node removal]
     expected:
       if os == "android": FAIL
-
-  [Compat mouse events with node removal on pointerdown]
-    expected: FAIL
-
-  [Compat mouse events with node removal on pointermove]
-    expected: FAIL
-

testing/web-platform/tests/pointerevents/pointerevent_pointermove_after_pointerup_target_removed.html

@@ -0,0 +1,96 @@
+<!doctype html>
+<html>
+<head>
+<meta charset="utf-8">
+<title>After pointerup target is removed, selection should not be updated by pointer move</title>
+<script src='/resources/testharness.js'></script>
+<script src='/resources/testharnessreport.js'></script>
+<script src="/resources/testdriver.js"></script>
+<script src="/resources/testdriver-actions.js"></script>
+<script src="/resources/testdriver-vendor.js"></script>
+<script>
+"use strict";
+
+/**
+ * This behavior is not defined in any spec, but even if browser supports
+ * expanding selection across text nodes with dragging a pointer, it should
+ * not work after pointerup whose target is removed by a pointerup event
+ * listener.
+ */
+
+addEventListener("load", async () => {
+  const span1 = document.querySelector("span");
+  const span2 = span1.nextSibling;
+  const span3 = span2.nextSibling;
+  const button = document.querySelector("button");
+
+  promise_test(async () => {
+    const overlay = document.createElement("div");
+    overlay.className = "overlay";
+    overlay.addEventListener("pointerup", () => {
+      overlay.remove();
+    });
+    document.body.appendChild(overlay);
+    overlay.style.display = "block";
+    await new Promise(resolve => {
+      requestAnimationFrame(() => requestAnimationFrame(resolve));
+    });
+    await new test_driver.Actions()
+      .pointerMove(10, 10, {origin: span1})
+      .pointerDown()
+      .pointerUp()
+      .pointerMove(10, 10, {origin: span2})
+      .addTick()
+      .pointerMove(10, 10, {origin: span3})
+      .addTick()
+      .send();
+    assert_true(getSelection().isCollapsed);
+  }, "pointermove after pointerup which deletes the overlay should not keep expanding selection");
+
+  promise_test(async () => {
+    const overlay = document.createElement("div");
+    overlay.className = "overlay";
+    overlay.addEventListener("pointerup", () => {
+      button.focus();
+      overlay.remove();
+    });
+    document.body.appendChild(overlay);
+    overlay.style.display = "block";
+    await new Promise(resolve => {
+      requestAnimationFrame(() => requestAnimationFrame(resolve));
+    });
+    await new test_driver.Actions()
+      .pointerMove(10, 10, {origin: span1})
+      .pointerDown()
+      .pointerUp()
+      .pointerMove(10, 10, {origin: span2})
+      .addTick()
+      .pointerMove(10, 10, {origin: span3})
+      .addTick()
+      .send();
+    assert_true(getSelection().isCollapsed);
+  }, "pointermove after pointerup which deletes the overlay and move focus to the button should not keep expanding selection");
+}, {once: true});
+</script>
+<style>
+div.overlay {
+  position: fixed;
+  top: 0;
+  left: 0;
+  width: 100%;
+  height: 100%;
+  background: rgba(0, 0, 0, 0.5);
+  display: none;
+}
+#container {
+  font-size: 32px;
+}
+</style>
+</head>
+<body>
+  <div id="container">
+    <span>abc</span><span>def</span><span>ghi</span><br>
+    <button>button</button>
+  </div>
+</body>
+</html>