Bug 1424264 - Part 2 - Open views before the transition and close them after it. r=Gijs draft
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Tue, 23 Jan 2018 14:42:06 +0000
changeset 723562 3cc6462adf9cb763c740c62c26ec73bee5243495
parent 723561 b9ce4556b3479e93c8e9b80d6da3bd17a378e66a
child 723563 7dab2689ad85d29e8b01465b8378cfe4adf4bff0
push id96468
push userpaolo.mozmail@amadzone.org
push dateTue, 23 Jan 2018 15:06:51 +0000
reviewersGijs
bugs1424264
milestone59.0a1
Bug 1424264 - Part 2 - Open views before the transition and close them after it. r=Gijs This is done to make the state of subviews more consistent at any given time. The showSubView and goBack methods now align with their callers and don't return a Promise, but check for re-entrancy. This check is not exhaustive and still subject to existing race conditions, but is an improvement over the current situation. The showMainView method still returns a Promise because at the moment it is used externally for asynchronous cleanup. MozReview-Commit-ID: 1VoIImxVTDN
browser/components/customizableui/PanelMultiView.jsm
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -11,33 +11,74 @@
  * declared using <panelview> elements that are usually children of the main
  * <panelmultiview> element, although they don't need to be, as views can also
  * be imported into the panel from other panels or popup sets.
  *
  * The main view can be declared using the mainViewId attribute, and specific
  * subviews can slide in using the showSubView method. Backwards navigation can
  * be done using the goBack method or through a button in the subview headers.
  *
+ * The process of displaying the main view or a new subview requires multiple
+ * steps to be completed, hence at any given time the <panelview> element may
+ * be in different states:
+ *
+ * -- Open or closed
+ *
+ *    All views start "closed" and are considered "open" as soon as they are
+ *    associated to a <panelmultiview> element for displaying. When a view is
+ *    opened, it is ensured that the <panelview> element is a descendant of the
+ *    <panelmultiview> element, moving it if necessary.
+ *
+ *    The "ViewShowing" event is fired at this point, when the view is not
+ *    visible yet. In the case of subviews, the event is allowed to cancel the
+ *    operation, in which case the view is closed immediately.
+ *
+ *    Closing the view does not move the node back to its original position.
+ *
+ * -- Visible or invisible
+ *
+ *    This indicates whether the view is visible in the document from a layout
+ *    perspective, regardless of whether it is currently scrolled into view. In
+ *    fact, all subviews are already visible before they start sliding in.
+ *
+ *    Before scrolling into view, a view may become visible but be placed in a
+ *    special off-screen area of the document where layout and measurements can
+ *    take place asyncronously.
+ *
+ *    When navigating forward, an open view may become invisible but stay open
+ *    after sliding out of view. The last known size of these views is still
+ *    taken into account for determining the overall panel size.
+ *
+ *    When navigating backwards, an open subview will first become invisible and
+ *    then will be closed.
+ *
+ * -- Navigating with the keyboard
+ *
+ *    An open view may keep state related to keyboard navigation, even if it is
+ *    invisible. When a view is closed, keyboard navigation state is cleared.
+ *
  * This diagram shows how <panelview> nodes move during navigation:
  *
  *   In this <panelmultiview>     In other panels    Action
  *             ┌───┬───┬───┐        ┌───┬───┐
  *             │(A)│ B │ C │        │ D │ E │          Open panel
  *             └───┴───┴───┘        └───┴───┘
  *         ┌───┬───┬───┐            ┌───┬───┐
- *         │ A │(C)│ B │            │ D │ E │          Show subview C
+ *         │{A}│(C)│ B │            │ D │ E │          Show subview C
  *         └───┴───┴───┘            └───┴───┘
  *     ┌───┬───┬───┬───┐            ┌───┐
- *     │ A │ C │(D)│ B │            │ E │              Show subview D
+ *     │{A}│{C}│(D)│ B │            │ E │              Show subview D
  *     └───┴───┴───┴───┘            └───┘
- *         ┌───┬───┬───┬───┐        ┌───┐
- *         │ A │(C)│ D │ B │        │ E │              Go back
- *         └───┴───┴───┴───┘        └───┘
- *               │
- *               └── Currently visible view
+ *       │ ┌───┬───┬───┬───┐        ┌───┐
+ *       │ │{A}│(C)│ D │ B │        │ E │              Go back
+ *       │ └───┴───┴───┴───┘        └───┘
+ *       │   │   │
+ *       │   │   └── Currently visible view
+ *       │   │   │
+ *       └───┴───┴── Open views
  *
  * If the <panelmultiview> element is "ephemeral", imported subviews will be
  * moved out again to the element specified by the viewCacheId attribute, so
  * that the panel element can be removed safely.
  */
 
 "use strict";
 
@@ -286,33 +327,73 @@ this.PanelMultiView = class extends this
     let subviews = Array.from(viewNodeContainer.childNodes);
     for (let subview of subviews) {
       // XBL lists the 'children' XBL element explicitly. :-(
       if (subview.nodeName != "children")
         this._panelViewCache.appendChild(subview);
     }
   }
 
+  /**
+   * Slides in the specified view as a subview.
+   *
+   * This method has no effect while a transition is in progress.
+   *
+   * @param viewIdOrNode
+   *        DOM element or string ID of the <panelview> to display.
+   * @param anchor
+   *        DOM element that triggered the subview, which will be highlighted
+   *        and whose "label" attribute will be used for the title of the
+   *        subview when a "title" attribute is not specified.
+   */
+  showSubView(viewIdOrNode, anchor) {
+    if (this._transitioning) {
+      return;
+    }
+
+    let viewNode = typeof viewIdOrNode == "string" ?
+                   this.document.getElementById(viewIdOrNode) : viewIdOrNode;
+    if (!viewNode) {
+      throw new Error(`Subview ${viewIdOrNode} doesn't exist.`);
+    }
+
+    let nextPanelView = PanelView.forNode(viewNode);
+    if (this.openViews.includes(nextPanelView)) {
+      throw new Error(`Subview ${viewNode.id} is already open.`);
+    }
+
+    this._showView(nextPanelView, anchor);
+  }
+
+  /**
+   * Navigates backwards by sliding out the most recent subview.
+   *
+   * This method has no effect while a transition is in progress.
+   */
   goBack() {
-    if (this.openViews.length < 2) {
+    if (this._transitioning || this.openViews.length < 2) {
       // This may be called by keyboard navigation or external code when only
       // the main view is open.
       return;
     }
 
-    let previous = this.openViews.pop().node;
-    let current = this._currentSubView;
-    this.showSubView(current, null, previous);
+    this._showView(this.openViews[this.openViews.length - 2], null, true);
   }
 
+  /**
+   * Resets the panel state so that the main view will be shown next time.
+   */
   showMainView() {
     if (!this.node || !this._mainViewId)
       return Promise.resolve();
 
-    return this.showSubView(this._mainView);
+    this.openViews.forEach(panelView => panelView.clearNavigation());
+    this.openViews = [];
+
+    return this._showView(PanelView.forNode(this._mainView));
   }
 
   /**
    * Ensures that all the panelviews, that are currently part of this instance,
    * are hidden, except one specifically.
    *
    * @param {panelview} [nextPanelView]
    *        The PanelView object to ensure is visible. Optional.
@@ -325,44 +406,37 @@ this.PanelMultiView = class extends this
       panelView.current = false;
     }
 
     this._viewShowing = null;
 
     if (!this.node || !nextPanelView)
       return;
 
-    if (!this.openViews.includes(nextPanelView))
-      this.openViews.push(nextPanelView);
-
     nextPanelView.current = true;
     this.showingSubView = nextPanelView.node.id != this._mainViewId;
   }
 
-  showSubView(aViewId, aAnchor, aPreviousView) {
+  _showView(nextPanelView, anchor, reverse) {
     this._currentShowPromise = (async () => {
-      // Support passing in the node directly.
-      let viewNode = typeof aViewId == "string" ? this.node.querySelector("#" + aViewId) : aViewId;
-      if (!viewNode) {
-        viewNode = this.document.getElementById(aViewId);
-        if (viewNode) {
-          this._viewStack.appendChild(viewNode);
-        } else {
-          throw new Error(`Subview ${aViewId} doesn't exist!`);
-        }
-      } else if (viewNode.parentNode == this._panelViewCache) {
-        this._viewStack.appendChild(viewNode);
+      let viewNode = nextPanelView.node;
+      let previousViewNode = this._currentSubView;
+
+      this._viewShowing = viewNode;
+      if (!reverse) {
+        this.openViews.push(nextPanelView);
       }
-
-      let nextPanelView = PanelView.forNode(viewNode);
       this.knownViews.add(nextPanelView);
 
       viewNode.panelMultiView = this.node;
 
-      let previousViewNode = aPreviousView || this._currentSubView;
+      if (viewNode.parentNode != this._viewStack) {
+        this._viewStack.appendChild(viewNode);
+      }
+
       // If the panelview to show is the same as the previous one, the 'ViewShowing'
       // event has already been dispatched. Don't do it twice.
       let showingSameView = viewNode == previousViewNode;
 
       let previousRect = previousViewNode.__lastKnownBoundingRect =
           this._dwu.getBoundsWithoutFlushing(previousViewNode);
       // Cache the measures that have the same caching lifetime as the width
       // or height of the main view, i.e. whilst the panel is shown and/ or
@@ -370,32 +444,29 @@ this.PanelMultiView = class extends this
       if (!this._mainViewWidth) {
         this._mainViewWidth = previousRect.width;
       }
       if (!this._mainViewHeight) {
         this._mainViewHeight = previousRect.height;
         this._viewContainer.style.minHeight = this._mainViewHeight + "px";
       }
 
-      this._viewShowing = viewNode;
-
-      let reverse = !!aPreviousView;
       if (!reverse) {
         // We are opening a new view, either because we are navigating forward
         // or because we are showing the main view. We have to make sure that
         // the view appears properly based on how it's been opened and on the
         // size of the previous views.
         let isMainView = viewNode.id == this._mainViewId;
         nextPanelView.mainview = isMainView;
         nextPanelView.minMaxWidthPx = isMainView ? 0 : this._mainViewWidth;
         nextPanelView.headerText = viewNode.getAttribute("title") ||
-                                   (aAnchor && aAnchor.getAttribute("label"));
+                                   (anchor && anchor.getAttribute("label"));
       }
 
-      if (aAnchor) {
+      if (anchor) {
         viewNode.classList.add("PanelUI-subView");
       }
 
       if (!showingSameView || !viewNode.hasAttribute("current")) {
         // Emit the ViewShowing event so that the widget definition has a chance
         // to lazily populate the subview with things or perhaps even cancel this
         // whole operation.
         let detail = {
@@ -411,30 +482,40 @@ this.PanelMultiView = class extends this
             cancel = cancel || results.some(val => val === false);
           } catch (e) {
             Cu.reportError(e);
             cancel = true;
           }
         }
 
         if (cancel) {
+          // Close the view that was not displayed, considering that the panel
+          // may have been hidden in the meantime.
+          if (this.openViews.length) {
+            this.openViews.pop();
+          }
           this._viewShowing = null;
           return;
         }
       }
 
       // Now we have to transition the panel. If we've got an older transition
       // still running, make sure to clean it up.
       await this._cleanupTransitionPhase();
       if (!showingSameView && this._panel.state == "open") {
-        await this._transitionViews(previousViewNode, viewNode, reverse, previousRect, aAnchor);
+        await this._transitionViews(previousViewNode, viewNode, reverse, previousRect, anchor);
         nextPanelView.focusSelectedElement();
       } else {
         this.hideAllViewsExcept(nextPanelView);
       }
+
+      // The panel may have been hidden in the meantime.
+      if (reverse && this.openViews.length) {
+        this.openViews.pop();
+      }
     })().catch(e => Cu.reportError(e));
     return this._currentShowPromise;
   }
 
   /**
    * 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
@@ -758,18 +839,16 @@ this.PanelMultiView = class extends this
         this.showMainView();
         for (let panelView of this._viewStack.children) {
           if (panelView.nodeName != "children") {
             panelView.__lastKnownBoundingRect = null;
           }
         }
         this.window.removeEventListener("keydown", this);
         this._panel.removeEventListener("mousemove", this);
-        this.openViews.forEach(panelView => panelView.clearNavigation());
-        this.openViews = [];
 
         // Clear the main view size caches. The dimensions could be different
         // when the popup is opened again, e.g. through touch mode sizing.
         this._mainViewHeight = 0;
         this._mainViewWidth = 0;
         this._viewContainer.style.removeProperty("min-height");
         this._viewStack.style.removeProperty("max-height");
         this._viewContainer.style.removeProperty("min-width");