Bug 1910452: Part 3 - Fix some issues with synthesizeMockDragAndDrop …

…r=m_kato This fixes four issues: 1. The test didn't provide enough movement to generate a drag session on the source before moving to the target. This meant that, when they were in different windows, Gecko wouldn't send dragleave to the source or dragenter to the target. It also never sent dragenter to the source in the first place. This remedies that. 2. dragenter and dragleave weren't properly handled because the test was sending dragleaves instead of dragexits (the latter being what Gecko expects and the former being synthesized from that -- see e.g. nsNativeDragTarget::DragLeave). This now uses dragexits and sets the proper expectations. 3. expectProtectedDataTransferAccess was needlessly complicated and, after #1, gave the wrong answers for some events like dragenter called on the source. 4. The event handler wasn't checking for exceptions and the drop handler was intentionally causing one, which was causing it to miss the rest of its execution. Differential Revision: https://phabricator.services.mozilla.com/D219550
i3roly · Sep 11, 2024 · 4da7eda · 4da7eda
1 parent fb6f5c0
commit 4da7eda
Show file tree

Hide file tree

Showing 4 changed files with 123 additions and 89 deletions.
diff --git a/testing/mochitest/tests/SimpleTest/DragChildContextBase.sys.mjs b/testing/mochitest/tests/SimpleTest/DragChildContextBase.sys.mjs
@@ -60,11 +60,6 @@ export class DragChildContextBase {
   // dataTransfer?  Set as parameter to initialize.
   expectProtectedDataTransferAccess = false;
 
-  // Should events dragend events have access to the dataTransfer?  dragend
-  // access is also subject to expectProtectedDataTransferAccess.  Set as
-  // parameter to initialize.
-  expectProtectedDataTransferAccessDragendOnly = false;
-
   window = null;
 
   dragService = null;
@@ -232,49 +227,56 @@ export class DragChildContextBase {
       sandbox
     );
 
-    if (aEv.type == "dragstart") {
-      // Add some additional data to the DataTransfer so we can look for it
-      // as we get later events.
-      this.is(
-        getFromDataTransfer(),
-        "",
-        `[${aEv.type}]| DataTransfer didn't have kTestDataTransferType`
-      );
-      setInDataTransfer();
-      this.is(
-        getFromDataTransfer(),
-        kTestDataTransferData,
-        `[${aEv.type}]| Successfully added kTestDataTransferType to DataTransfer`
-      );
-    } else if (aEv.type == "drop") {
-      this.is(
-        getFromDataTransfer(),
-        kTestDataTransferData,
-        `[${aEv.type}]| Successfully read from DataTransfer`
-      );
-      clearDataTransfer();
-      this.is(
-        getFromDataTransfer(),
-        kTestDataTransferData,
-        `[${aEv.type}]| Properly failed to write to DataTransfer`
-      );
-    } else if (
-      aEv.type == "dragenter" ||
-      aEv.type == "dragover" ||
-      aEv.type == "dragleave" ||
-      aEv.type == "dragend"
-    ) {
-      let expectProtectedDataTransferAccess =
-        this.expectProtectedDataTransferAccess ||
-        (aEv.type == "dragend" &&
-          this.expectProtectedDataTransferAccessDragendOnly);
-      this.is(
-        getFromDataTransfer(),
-        expectProtectedDataTransferAccess ? kTestDataTransferData : "",
-        `[${aEv.type}]| ${
-          expectProtectedDataTransferAccess ? "Successfully" : "Unsuccessfully"
-        } read from DataTransfer`
-      );
+    try {
+      if (aEv.type == "dragstart") {
+        // Add some additional data to the DataTransfer so we can look for it
+        // as we get later events.
+        this.is(
+          getFromDataTransfer(),
+          "",
+          `[${aEv.type}]| DataTransfer didn't have kTestDataTransferType`
+        );
+        setInDataTransfer();
+        this.is(
+          getFromDataTransfer(),
+          kTestDataTransferData,
+          `[${aEv.type}]| Successfully added kTestDataTransferType to DataTransfer`
+        );
+      } else if (aEv.type == "drop") {
+        this.is(
+          getFromDataTransfer(),
+          kTestDataTransferData,
+          `[${aEv.type}]| Successfully read from DataTransfer`
+        );
+        try {
+          clearDataTransfer();
+          this.ok(false, "Writing to DataTransfer throws an exception");
+        } catch (ex) {
+          this.ok(true, "Got exception: " + ex);
+        }
+        this.is(
+          getFromDataTransfer(),
+          kTestDataTransferData,
+          `[${aEv.type}]| Properly failed to write to DataTransfer`
+        );
+      } else if (
+        aEv.type == "dragenter" ||
+        aEv.type == "dragover" ||
+        aEv.type == "dragleave" ||
+        aEv.type == "dragend"
+      ) {
+        this.is(
+          getFromDataTransfer(),
+          this.expectProtectedDataTransferAccess ? kTestDataTransferData : "",
+          `[${aEv.type}]| ${
+            this.expectProtectedDataTransferAccess
+              ? "Successfully"
+              : "Unsuccessfully"
+          } read from DataTransfer`
+        );
+      }
+    } catch (ex) {
+      this.ok(false, "Handler did not throw an uncaught exception: " + ex);
     }
 
     if (

diff --git a/testing/mochitest/tests/SimpleTest/EventUtils.js b/testing/mochitest/tests/SimpleTest/EventUtils.js
@@ -4163,21 +4163,18 @@ async function synthesizeMockDragAndDrop(aParams) {
   //
   // dragstart and dragend are special because they target the drag-source,
   // not the drag-target.
-  let expectProtectedDataTransferAccess =
-    !SpecialPowers.getBoolPref("dom.events.dataTransfer.protected.enabled") &&
-    browsingContextsAreRelated(targetBrowsingCxt, sourceBrowsingCxt);
-
-  // expectProtectedDataTransferAccessDragendOnly overrides
-  // expectProtectedDataTransferAccess when it is true
-  let expectProtectedDataTransferAccessDragendOnly = !SpecialPowers.getBoolPref(
+  let expectProtectedDataTransferAccessSource = !SpecialPowers.getBoolPref(
     "dom.events.dataTransfer.protected.enabled"
   );
+  let expectProtectedDataTransferAccessTarget =
+    expectProtectedDataTransferAccessSource &&
+    browsingContextsAreRelated(targetBrowsingCxt, sourceBrowsingCxt);
 
   info(
-    `expectProtectedDataTransferAccess: ${expectProtectedDataTransferAccess}`
+    `expectProtectedDataTransferAccessSource: ${expectProtectedDataTransferAccessSource}`
   );
   info(
-    `expectProtectedDataTransferAccessDragendOnly: ${expectProtectedDataTransferAccessDragendOnly}`
+    `expectProtectedDataTransferAccessTarget: ${expectProtectedDataTransferAccessTarget}`
   );
 
   // Essentially the entire function is in a try block so that we can make sure
@@ -4225,18 +4222,20 @@ async function synthesizeMockDragAndDrop(aParams) {
       expectCancelDragStart,
       expectSrcElementDisconnected,
       expectNoDragEvents,
-      expectProtectedDataTransferAccessDragendOnly,
+      expectProtectedDataTransferAccess:
+        expectProtectedDataTransferAccessSource,
       dragElementId: srcElement,
     };
     const targetVars = {
       expectDragLeave,
       expectNoDragTargetEvents,
+      expectProtectedDataTransferAccess:
+        expectProtectedDataTransferAccessTarget,
       dragElementId: targetElement,
     };
     const bothVars = {
       contextLabel,
       throwOnExtraMessage,
-      expectProtectedDataTransferAccess,
       relevantEvents: [
         "mousedown",
         "mouseup",
@@ -4376,20 +4375,20 @@ async function synthesizeMockDragAndDrop(aParams) {
 
     // Another move creates the drag session in the parent process (but we need
     // to wait for the src process to get there).
-    info(`Moving to target element.`);
-    let currentTargetScreenPos = [
-      Math.ceil(targetPos.screenPos[0]),
-      Math.ceil(targetPos.screenPos[1]),
+    currentSrcScreenPos = [
+      currentSrcScreenPos[0] + step[0],
+      currentSrcScreenPos[1] + step[1],
     ];
-
+    info(
+      `third mousemove at ${currentSrcScreenPos[0]}, ${currentSrcScreenPos[1]}`
+    );
     dragController.sendEvent(
       sourceBrowsingCxt,
       Ci.nsIMockDragServiceController.eMouseMove,
-      currentTargetScreenPos[0],
-      currentTargetScreenPos[1]
+      currentSrcScreenPos[0],
+      currentSrcScreenPos[1]
     );
-
-    await sourceCxt.checkExpected();
+    info(`third mousemove sent`);
 
     ok(
       _getDOMWindowUtils(sourceBrowsingCxt.ownerGlobal).dragSession,
@@ -4406,26 +4405,60 @@ async function synthesizeMockDragAndDrop(aParams) {
       return;
     }
 
-    currentTargetScreenPos = [
-      currentTargetScreenPos[0] + step[0],
-      currentTargetScreenPos[1] + step[1],
+    await sourceCxt.checkExpected();
+
+    // Implementation detail: EventStateManager::GenerateDragDropEnterExit
+    // expects the source to get at least one dragover before leaving the
+    // widget or else it fails to send dragenter/dragleave events to the
+    // browsers.
+    info("synthesizing dragover inside source");
+    sourceCxt.expect("dragenter");
+    sourceCxt.expect("dragover");
+    currentSrcScreenPos = [
+      currentSrcScreenPos[0] + step[0],
+      currentSrcScreenPos[1] + step[1],
+    ];
+    info(`dragover at ${currentSrcScreenPos[0]}, ${currentSrcScreenPos[1]}`);
+    dragController.sendEvent(
+      sourceBrowsingCxt,
+      Ci.nsIMockDragServiceController.eDragOver,
+      currentSrcScreenPos[0],
+      currentSrcScreenPos[1]
+    );
+
+    info(`dragover sent`);
+    await sourceCxt.checkExpected();
+
+    let currentTargetScreenPos = [
+      Math.ceil(targetPos.screenPos[0]),
+      Math.ceil(targetPos.screenPos[1]),
     ];
 
-    // Send dragleave and dragenter only if we moved to another widget.
-    // If we moved in the same widget then dragenter does not involve
-    // the parent process.  This mirrors the native behavior.  Note that
-    // these events are not forwarded to the content process -- they
-    // are generated there by the EventStateManager when appropriate.
+    // The next step is to drag to the target element.
+    if (!expectNoDragTargetEvents) {
+      sourceCxt.expect("dragleave");
+    }
+
     if (
       sourceBrowsingCxt.top.embedderElement !==
       targetBrowsingCxt.top.embedderElement
     ) {
-      // Dragging from widget to widget
-      info("synthesizing dragleave and dragenter to enter new widget");
+      // Send dragexit and dragenter only if we are dragging to another widget.
+      // If we are dragging in the same widget then dragenter does not involve
+      // the parent process.  This mirrors the native behavior. In the
+      // widget-to-widget case, the source gets the dragexit immediately but
+      // the target won't get a dragenter in content until we send a dragover --
+      // this is because dragenters are generated by the EventStateManager and
+      // are not forwarded remotely.
+      // NB: dragleaves are synthesized by Gecko from dragexits.
+      info("synthesizing dragexit and dragenter to enter new widget");
+      if (!expectNoDragTargetEvents) {
+        info("This will generate dragleave on the source");
+      }
 
       dragController.sendEvent(
         sourceBrowsingCxt,
-        Ci.nsIMockDragServiceController.eDragLeave,
+        Ci.nsIMockDragServiceController.eDragExit,
         currentTargetScreenPos[0],
         currentTargetScreenPos[1]
       );
@@ -4437,21 +4470,20 @@ async function synthesizeMockDragAndDrop(aParams) {
         currentTargetScreenPos[1]
       );
 
+      await sourceCxt.synchronize();
+
       await sourceCxt.checkExpected();
       await targetCxt.checkExpected();
     }
 
-    info("synthesizing dragover to generate dragenter in DOM");
-
+    info(
+      "Synthesizing dragover over target.  This will first generate a dragenter."
+    );
     if (!expectNoDragTargetEvents) {
       targetCxt.expect("dragenter");
       targetCxt.expect("dragover");
     }
 
-    currentTargetScreenPos = [
-      currentTargetScreenPos[0] + step[0],
-      currentTargetScreenPos[1] + step[1],
-    ];
     dragController.sendEvent(
       targetBrowsingCxt,
       Ci.nsIMockDragServiceController.eDragOver,

diff --git a/widget/MockDragServiceController.cpp b/widget/MockDragServiceController.cpp
@@ -102,8 +102,8 @@ static EventMessage MockEventTypeToEventMessage(
       return EventMessage::eDragEnter;
     case EventType::eDragOver:
       return EventMessage::eDragOver;
-    case EventType::eDragLeave:
-      return EventMessage::eDragLeave;
+    case EventType::eDragExit:
+      return EventMessage::eDragExit;
     case EventType::eDrop:
       return EventMessage::eDrop;
     case EventType::eMouseDown:
@@ -184,7 +184,7 @@ MockDragServiceController::SendEvent(
       currentDragSession->SetDragAction(nsIDragService::DRAGDROP_ACTION_MOVE);
       widget->DispatchInputEvent(widgetEvent.get());
       break;
-    case EventType::eDragLeave: {
+    case EventType::eDragExit: {
       NS_ENSURE_TRUE(currentDragSession, NS_ERROR_UNEXPECTED);
       currentDragSession->SetDragAction(nsIDragService::DRAGDROP_ACTION_MOVE);
       widget->DispatchInputEvent(widgetEvent.get());

diff --git a/widget/nsIMockDragServiceController.idl b/widget/nsIMockDragServiceController.idl
@@ -21,7 +21,7 @@ interface nsIMockDragServiceController : nsISupports
   cenum EventType : 8 {
     eDragEnter = 0,
     eDragOver = 1,
-    eDragLeave = 2,
+    eDragExit = 2,
     eDrop = 3,
     eMouseDown = 4,
     eMouseMove = 5,