Bug 1439358 - Part 6 - Decouple view events from view visibility. r=Gijs draft
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Thu, 22 Feb 2018 14:20:11 +0000
changeset 758543 5c90cbbc4b194ec4bb4e28f50082097371e3086d
parent 758542 31184a13b82c1f8fd60b6551a65aae6e4e11a8fe
child 758544 5b50ac03f80902ccbf64c4dcad6e0505a0cdbfa4
push id100095
push userpaolo.mozmail@amadzone.org
push dateThu, 22 Feb 2018 17:13:44 +0000
reviewersGijs
bugs1439358
milestone60.0a1
Bug 1439358 - Part 6 - Decouple view events from view visibility. r=Gijs This stops redundant ViewHiding and late ViewShown events from being dispatched when the panel is closed during a ViewShowing event or a transition, and stops dispatching ViewHiding events when a view becomes invisible but is still open. The panelMultiView property on "panelview" nodes is now set to null when the view is closed, indicating that the view can be immediately reused in a different panel. The Places view had to be updated so it doesn't rely on this property during the PanelMultiViewHidden event. MozReview-Commit-ID: B1yU6si3eD3
browser/components/customizableui/PanelMultiView.jsm
browser/components/places/content/browserPlacesViews.js
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -624,16 +624,17 @@ this.PanelMultiView = class extends this
     // The constrained width of subviews may also vary between panels.
     nextPanelView.minMaxWidth = prevPanelView.knownWidth;
 
     if (anchor) {
       viewNode.classList.add("PanelUI-subView");
     }
 
     await this._transitionViews(prevPanelView.node, viewNode, false, anchor);
+    this._viewShown(nextPanelView);
   }
 
   /**
    * Navigates backwards by sliding out the most recent subview.
    */
   goBack() {
     this._goBack().catch(Cu.reportError);
   }
@@ -645,17 +646,19 @@ this.PanelMultiView = class extends this
     }
 
     let prevPanelView = this.openViews[this.openViews.length - 1];
     let nextPanelView = this.openViews[this.openViews.length - 2];
 
     prevPanelView.captureKnownSize();
     await this._transitionViews(prevPanelView.node, nextPanelView.node, true);
 
-    this.openViews.pop();
+    this._closeLatestView();
+
+    this._viewShown(nextPanelView);
   }
 
   /**
    * Prepares the main view before showing the panel.
    */
   async showMainView() {
     if (!this.node || !this._mainViewId) {
       return false;
@@ -670,16 +673,17 @@ this.PanelMultiView = class extends this
     // reset all the properties that may be set on a subview.
     nextPanelView.mainview = true;
     nextPanelView.headerText = "";
     nextPanelView.minMaxWidth = 0;
 
     await this._cleanupTransitionPhase();
     this.hideAllViewsExcept(nextPanelView);
 
+    this._viewShown(nextPanelView);
     return true;
   }
 
   /**
    * Ensures that all the panelviews, that are currently part of this instance,
    * are hidden, except one specifically.
    *
    * @param {panelview} [nextPanelView]
@@ -722,26 +726,48 @@ this.PanelMultiView = class extends this
     // point the ViewHiding event has already been dispatched for all of them.
     if (!this.openViews.length) {
       return false;
     }
 
     // Check if the event requested cancellation but the panel is still open.
     if (canceled) {
       // Handlers for ViewShowing can't know if a different handler requested
-      // cancellation, so we dispatch an event to give a chance to clean up.
-      panelView.dispatchCustomEvent("ViewHiding");
-      this.openViews.pop();
+      // cancellation, so this will dispatch a ViewHiding event to give a chance
+      // to clean up.
+      this._closeLatestView();
       return false;
     }
 
     return true;
   }
 
   /**
+   * Raises the ViewShown event if the specified view is still open.
+   */
+  _viewShown(panelView) {
+    if (panelView.node.panelMultiView == this.node) {
+      panelView.dispatchCustomEvent("ViewShown");
+    }
+  }
+
+  /**
+   * Closes the most recent PanelView and raises the ViewHiding event.
+   *
+   * @note The ViewHiding event is not cancelable and should probably be renamed
+   *       to ViewHidden or ViewClosed instead, see bug 1438507.
+   */
+  _closeLatestView() {
+    let panelView = this.openViews.pop();
+    panelView.clearNavigation();
+    panelView.dispatchCustomEvent("ViewHiding");
+    panelView.node.panelMultiView = null;
+  }
+
+  /**
    * Apply a transition to 'slide' from the currently active view to the next
    * one.
    * Sliding the next subview in means that the previous panelview stays where it
    * is and the active panelview slides in from the left in LTR mode, right in
    * RTL mode.
    *
    * @param {panelview} previousViewNode Node that is currently shown as active,
    *                                     but is about to be transitioned away.
@@ -919,25 +945,22 @@ this.PanelMultiView = class extends this
     if (!details || !this.node)
       return;
 
     let {phase, previousViewNode, viewNode, reverse, resolve, listener, cancelListener, anchor} = details;
     if (details == this._transitionDetails)
       this._transitionDetails = null;
 
     let nextPanelView = PanelView.forNode(viewNode);
-    let prevPanelView = PanelView.forNode(previousViewNode);
 
     // Do the things we _always_ need to do whenever the transition ends or is
     // interrupted.
     this.hideAllViewsExcept(nextPanelView);
     previousViewNode.removeAttribute("in-transition");
     viewNode.removeAttribute("in-transition");
-    if (reverse)
-      prevPanelView.clearNavigation();
 
     if (anchor)
       anchor.removeAttribute("open");
 
     if (phase >= TRANSITION_PHASES.START) {
       this._panel.removeAttribute("width");
       this._panel.removeAttribute("height");
       // Myeah, panel layout auto-resizing is a funky thing. We'll wait
@@ -1062,23 +1085,24 @@ this.PanelMultiView = class extends this
         // description elements it contains.
         PanelView.forNode(this._mainView).descriptionHeightWorkaround();
         break;
       case "popuphidden": {
         // WebExtensions consumers can hide the popup from viewshowing, or
         // mid-transition, which disrupts our state:
         this._transitioning = false;
         this.node.removeAttribute("panelopen");
-        // Raise the ViewHiding event for the current view.
         this._cleanupTransitionPhase();
         this.hideAllViewsExcept(null);
         this.window.removeEventListener("keydown", this);
         this._panel.removeEventListener("mousemove", this);
-        this.openViews.forEach(panelView => panelView.clearNavigation());
-        this.openViews = [];
+        // Raise ViewHiding events for open views in reverse order.
+        while (this.openViews.length) {
+          this._closeLatestView();
+        }
 
         // Clear the main view size caches. The dimensions could be different
         // when the popup is opened again, e.g. through touch mode sizing.
         this._viewContainer.style.removeProperty("min-height");
         this._viewStack.style.removeProperty("max-height");
         this._viewContainer.style.removeProperty("width");
         this._viewContainer.style.removeProperty("height");
 
@@ -1107,20 +1131,18 @@ this.PanelView = class extends this.Asso
     }
   }
 
   set current(value) {
     if (value) {
       if (!this.node.hasAttribute("current")) {
         this.node.setAttribute("current", true);
         this.descriptionHeightWorkaround();
-        this.dispatchCustomEvent("ViewShown");
       }
-    } else if (this.node.hasAttribute("current")) {
-      this.dispatchCustomEvent("ViewHiding");
+    } else {
       this.node.removeAttribute("current");
     }
   }
 
   /**
    * Constrains the width of this view using the "min-width" and "max-width"
    * styles. Setting this to zero removes the constraints.
    */
--- a/browser/components/places/content/browserPlacesViews.js
+++ b/browser/components/places/content/browserPlacesViews.js
@@ -2112,24 +2112,16 @@ this.PlacesPanelview = class extends Pla
   }
 
   get events() {
     if (this._events)
       return this._events;
     return this._events = ["click", "command", "dragend", "dragstart", "ViewHiding", "ViewShown"];
   }
 
-  get panel() {
-    return this.panelMultiView.parentNode;
-  }
-
-  get panelMultiView() {
-    return this._viewElt.panelMultiView;
-  }
-
   handleEvent(event) {
     switch (event.type) {
       case "click":
         // For middle clicks, fall through to the command handler.
         if (event.button != 1) {
           break;
         }
       case "command":
@@ -2191,16 +2183,17 @@ this.PlacesPanelview = class extends Pla
 
     this._controller.setDataTransfer(event);
     event.stopPropagation();
   }
 
   uninit(event) {
     this._removeEventListeners(this.panelMultiView, this.events);
     this._removeEventListeners(window, ["unload"]);
+    delete this.panelMultiView;
     super.uninit(event);
   }
 
   _createDOMNodeForPlacesNode(placesNode) {
     this._domNodes.delete(placesNode);
 
     let element;
     let type = placesNode.type;
@@ -2271,16 +2264,17 @@ this.PlacesPanelview = class extends Pla
     }
   }
 
   _onPopupShowing(event) {
     // If the event came from the root element, this is the first time
     // we ever get here.
     if (event.originalTarget == this._rootElt) {
       // Start listening for events from all panels inside the panelmultiview.
+      this.panelMultiView = this._viewElt.panelMultiView;
       this._addEventListeners(this.panelMultiView, this.events);
     }
     super._onPopupShowing(event);
   }
 
   _onViewShown(event) {
     if (event.originalTarget != this._viewElt)
       return;