Bug 1545296 - Suppress mouse button events firing in content when clicking on the Picture-in-Picture toggle. r=jaws
authorMike Conley <mconley@mozilla.com>
Thu, 02 May 2019 17:51:30 +0000
changeset 472346 a3cfd676266ff5e83cc3721444f12e888338bcb7
parent 472345 49a573cb12dbf46b578966b93e0d878d73c9b73f
child 472347 ca33e95075c3cfee3e5846c53dc6ec7cbb296508
push id35954
push userrgurzau@mozilla.com
push dateFri, 03 May 2019 04:14:31 +0000
treeherdermozilla-central@d7b02bc7cf44 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs1545296
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1545296 - Suppress mouse button events firing in content when clicking on the Picture-in-Picture toggle. r=jaws Differential Revision: https://phabricator.services.mozilla.com/D29422
toolkit/actors/PictureInPictureChild.jsm
toolkit/modules/ActorManagerParent.jsm
--- a/toolkit/actors/PictureInPictureChild.jsm
+++ b/toolkit/actors/PictureInPictureChild.jsm
@@ -69,16 +69,23 @@ class PictureInPictureToggleChild extend
         // The number of videos that are supposedly visible, according to the
         // IntersectionObserver
         visibleVideosCount: 0,
         // The DeferredTask that we'll arm every time a mousemove event occurs
         // on a page where we have one or more visible videos.
         mousemoveDeferredTask: null,
         // A weak reference to the last video we displayed the toggle over.
         weakOverVideo: null,
+        // True if the user is in the midst of clicking the toggle.
+        isClickingToggle: false,
+        // Set to the original target element on pointerdown if the user is clicking
+        // the toggle - this way, we can determine if a "click" event will need to be
+        // suppressed ("click" events don't fire if a "mouseup" occurs on a different
+        // element from the "pointerdown" / "mousedown" event).
+        clickedElement: null,
       };
       this.weakDocStates.set(this.content.document, state);
     }
 
     return state;
   }
 
   handleEvent(event) {
@@ -93,24 +100,37 @@ class PictureInPictureToggleChild extend
         if (this.toggleEnabled &&
             event.target instanceof this.content.HTMLVideoElement &&
             !event.target.controls &&
             event.target.ownerDocument == this.content.document) {
           this.registerVideo(event.target);
         }
         break;
       }
-      case "mousedown": {
-        this.onMouseDown(event);
+      case "mousedown":
+      case "pointerup":
+      case "mouseup":
+      case "click": {
+        this.onMouseButtonEvent(event);
+        break;
+      }
+      case "pointerdown": {
+        this.onPointerDown(event);
         break;
       }
       case "mousemove": {
         this.onMouseMove(event);
         break;
       }
+      case "pagehide": {
+        if (event.target.top == event.target) {
+          this.removeMouseButtonListeners();
+        }
+        break;
+      }
     }
   }
 
   /**
    * Adds a <video> to the IntersectionObserver so that we know when it becomes
    * visible.
    *
    * @param {Element} video The <video> element to register.
@@ -195,16 +215,50 @@ class PictureInPictureToggleChild extend
       } else {
         this.content.requestIdleCallback(() => {
           this.stopTrackingMouseOverVideos();
         });
       }
     }
   }
 
+  addMouseButtonListeners() {
+    // We want to try to cancel the mouse events from continuing
+    // on into content if the user has clicked on the toggle, so
+    // we don't use the mozSystemGroup here, and add the listener
+    // to the parent target of the window, which in this case,
+    // is the windowRoot. Since this event listener is attached to
+    // part of the outer window, we need to also remove it in a
+    // pagehide event listener in the event that the page unloads
+    // before stopTrackingMouseOverVideos fires.
+    this.content.windowRoot.addEventListener("pointerdown", this,
+                                             { capture: true });
+    this.content.windowRoot.addEventListener("mousedown", this,
+                                             { capture: true });
+    this.content.windowRoot.addEventListener("mouseup", this,
+                                             { capture: true });
+    this.content.windowRoot.addEventListener("pointerup", this,
+                                             { capture: true });
+    this.content.windowRoot.addEventListener("click", this,
+                                             { capture: true });
+  }
+
+  removeMouseButtonListeners() {
+    this.content.windowRoot.removeEventListener("pointerdown", this,
+                                                { capture: true });
+    this.content.windowRoot.removeEventListener("mousedown", this,
+                                                { capture: true });
+    this.content.windowRoot.removeEventListener("mouseup", this,
+                                                { capture: true });
+    this.content.windowRoot.removeEventListener("pointerup", this,
+                                                { capture: true });
+    this.content.windowRoot.removeEventListener("click", this,
+                                                { capture: true });
+  }
+
   /**
    * One of the challenges of displaying this toggle is that many sites put
    * things over top of <video> elements, like custom controls, or images, or
    * all manner of things that might intercept mouseevents that would normally
    * fire directly on the <video>. In order to properly detect when the mouse
    * is over top of one of the <video> elements in this situation, we currently
    * add a mousemove event handler to the entire document, and stash the most
    * recent mousemove that fires. At periodic intervals, that stashed mousemove
@@ -217,54 +271,50 @@ class PictureInPictureToggleChild extend
     let state = this.docState;
     if (!state.mousemoveDeferredTask) {
       state.mousemoveDeferredTask = new DeferredTask(() => {
         this.checkLastMouseMove();
       }, MOUSEMOVE_PROCESSING_DELAY_MS);
     }
     this.content.document.addEventListener("mousemove", this,
                                            { mozSystemGroup: true, capture: true });
-    // We want to try to cancel the mouse events from continuing
-    // on into content if the user has clicked on the toggle, so
-    // we don't use the mozSystemGroup here.
-    this.content.document.addEventListener("mousedown", this,
-                                           { capture: true });
+    this.addMouseButtonListeners();
   }
 
   /**
    * If we no longer have any interesting videos in the viewport, we deregister
    * the mousemove and click listeners, and also remove any toggles that might
    * be on the page still.
    */
   stopTrackingMouseOverVideos() {
     let state = this.docState;
     state.mousemoveDeferredTask.disarm();
     this.content.document.removeEventListener("mousemove", this,
                                               { mozSystemGroup: true, capture: true });
-    this.content.document.removeEventListener("mousedown", this,
-                                              { capture: true });
+    this.removeMouseButtonListeners();
     let oldOverVideo = state.weakOverVideo && state.weakOverVideo.get();
     if (oldOverVideo) {
       this.onMouseLeaveVideo(oldOverVideo);
     }
   }
 
   /**
-   * If we're tracking <video> elements, this mousedown event handler is run anytime
-   * a mousedown occurs on the document. This function is responsible for checking
+   * If we're tracking <video> elements, this pointerdown event handler is run anytime
+   * a pointerdown occurs on the document. This function is responsible for checking
    * if the user clicked on the Picture-in-Picture toggle. It does this by first
    * checking if the video is visible beneath the point that was clicked. Then
-   * it tests whether or not the mousedown occurred within the rectangle of the
-   * toggle. If so, the event's default behaviour and propagation are stopped,
-   * and Picture-in-Picture is triggered.
+   * it tests whether or not the pointerdown occurred within the rectangle of the
+   * toggle. If so, the event's propagation is stopped, and Picture-in-Picture is
+   * triggered.
    *
    * @param {Event} event The mousemove event.
    */
-  onMouseDown(event) {
+  onPointerDown(event) {
     let state = this.docState;
+
     let video = state.weakOverVideo && state.weakOverVideo.get();
     if (!video) {
       return;
     }
 
     let shadowRoot = video.openOrClosedShadowRoot;
     if (!shadowRoot) {
       return;
@@ -282,30 +332,67 @@ class PictureInPictureToggleChild extend
     let elements = winUtils.nodesFromRect(clientX, clientY, 1, 1, 1, 1, true,
                                           false, true /* aOnlyVisible */);
     if (!Array.from(elements).includes(video)) {
       return;
     }
 
     let toggle = shadowRoot.getElementById("pictureInPictureToggleButton");
     if (this.isMouseOverToggle(toggle, event)) {
-      event.preventDefault();
-      event.stopPropagation();
+      state.isClickingToggle = true;
+      state.clickedElement = Cu.getWeakReference(event.originalTarget);
+      event.stopImmediatePropagation();
       let pipEvent =
         new this.content.CustomEvent("MozTogglePictureInPicture", {
           bubbles: true,
         });
       video.dispatchEvent(pipEvent);
+
       // Since we've initiated Picture-in-Picture, we can go ahead and
       // hide the toggle now.
       this.onMouseLeaveVideo(video);
     }
   }
 
   /**
+   * Called for mousedown, pointerup, mouseup and click events. If we
+   * detected that the user is clicking on the Picture-in-Picture toggle,
+   * these events are cancelled in the capture-phase before they reach
+   * content. The state for suppressing these events is cleared on the
+   * click event (unless the mouseup occurs on a different element from
+   * the mousedown, in which case, the state is cleared on mouseup).
+   *
+   * @param {Event} event A mousedown, pointerup, mouseup or click event.
+   */
+  onMouseButtonEvent(event) {
+    let state = this.docState;
+    if (state.isClickingToggle) {
+      event.stopImmediatePropagation();
+
+      // If this is a mouseup event, check to see if we have a record of what
+      // the original target was on pointerdown. If so, and if it doesn't match
+      // the mouseup original target, that means we won't get a click event, and
+      // we can clear the "clicking the toggle" state right away.
+      //
+      // Otherwise, we wait for the click event to do that.
+      let isMouseUpOnOtherElement =
+        event.type == "mouseup" &&
+        (!state.clickedElement ||
+         state.clickedElement.get() != event.originalTarget);
+
+      if (isMouseUpOnOtherElement || event.type == "click") {
+        // The click is complete, so now we reset the state so that
+        // we stop suppressing these events.
+        state.isClickingToggle = false;
+        state.clickedElement = null;
+      }
+    }
+  }
+
+  /**
    * Called for each mousemove event when we're tracking those events to
    * determine if the cursor is hovering over a <video>.
    *
    * @param {Event} event The mousemove event.
    */
   onMouseMove(event) {
     let state = this.docState;
     state.lastMouseMoveEvent = event;
--- a/toolkit/modules/ActorManagerParent.jsm
+++ b/toolkit/modules/ActorManagerParent.jsm
@@ -235,16 +235,17 @@ let LEGACY_ACTORS = {
   },
 
   PictureInPictureToggle: {
     child: {
       allFrames: true,
       module: "resource://gre/actors/PictureInPictureChild.jsm",
       events: {
         "canplay": {capture: true, mozSystemGroup: true},
+        "pagehide": {capture: true},
       },
     },
   },
 
   PopupBlocking: {
     child: {
       module: "resource://gre/actors/PopupBlockingChild.jsm",
       events: {