Bug 1401383 - remove anchor state after transition even if the transition is canceled, and always set main view as current, r?Gijs draft
authorMike de Boer <mdeboer@mozilla.com>
Thu, 21 Sep 2017 22:18:07 +0200
changeset 668559 10c7201d82fa07c55ce2da85ad6cc1b68abe4381
parent 668245 136f970f019a88d820151dd2bd9372cba1e5953d
child 732734 a4063b3b5560f675541b589c331e2ac3ea2400fe
push id81078
push usermdeboer@mozilla.com
push dateThu, 21 Sep 2017 20:23:52 +0000
reviewersGijs
bugs1401383
milestone57.0a1
Bug 1401383 - remove anchor state after transition even if the transition is canceled, and always set main view as current, r?Gijs We weren't removing the 'open' attribute from the anchor if the transition didn't complete. This patch fixes this by moving the addition of 'open' into _transitionViews, and its removal into _cleanupTransitionPhase. MozReview-Commit-ID: TS0CcwsHVN
browser/components/customizableui/PanelMultiView.jsm
browser/components/extensions/ext-browserAction.js
browser/components/extensions/test/browser/browser-common.ini
browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -427,17 +427,25 @@ this.PanelMultiView = class {
       } else {
         this._transitionHeight(() => {
           viewNode.removeAttribute("current");
           this._currentSubView = null;
           this.node.setAttribute("viewtype", "main");
         });
       }
     } else if (this.panelViews) {
-      this._mainView.setAttribute("current", "true");
+      // Make sure to hide all subviews, except for the mainView.
+      let mainView = this._mainView;
+      for (let panelview of this._panelViews) {
+        if (panelview == mainView)
+          panelview.setAttribute("current", true);
+        else
+          panelview.removeAttribute("current");
+      }
+      this.node.setAttribute("viewtype", "main");
     }
 
     if (!this.panelViews) {
       this._shiftMainView();
     }
   }
 
   showSubView(aViewId, aAnchor, aPreviousView) {
@@ -521,31 +529,27 @@ this.PanelMultiView = class {
 
       this._currentSubView = viewNode;
       if (this.panelViews) {
         if (viewNode.id == this._mainViewId) {
           this.node.setAttribute("viewtype", "main");
         } else {
           this.node.setAttribute("viewtype", "subview");
         }
+        // If we've got an older transition still running, make sure to clean it up.
+        await this._cleanupTransitionPhase();
         if (!playTransition) {
           viewNode.setAttribute("current", true);
           this.descriptionHeightWorkaround(viewNode);
         }
       }
 
       // Now we have to transition the panel.
       if (this.panelViews && playTransition) {
-        if (aAnchor)
-          aAnchor.setAttribute("open", true);
-
-        await this._transitionViews(previousViewNode, viewNode, reverse, previousRect);
-
-        if (aAnchor)
-          aAnchor.removeAttribute("open");
+        await this._transitionViews(previousViewNode, viewNode, reverse, previousRect, aAnchor);
 
         this._dispatchViewEvent(viewNode, "ViewShown");
         this._updateKeyboardFocus(viewNode);
       } else if (!this.panelViews) {
         this._transitionHeight(() => {
           viewNode.setAttribute("current", true);
           if (viewNode.id == this._mainViewId) {
             this.node.setAttribute("viewtype", "main");
@@ -572,37 +576,39 @@ this.PanelMultiView = class {
    * @param {panelview} previousViewNode Node that is currently shown as active,
    *                                     but is about to be transitioned away.
    * @param {panelview} viewNode         Node that will becode the active view,
    *                                     after the transition has finished.
    * @param {Boolean}   reverse          Whether we're navigation back to a
    *                                     previous view or forward to a next view.
    * @param {Object}    previousRect     Rect object, with the same structure as
    *                                     a DOMRect, of the `previousViewNode`.
-   * @param {Function}  callback         Function that will be invoked when the
-   *                                     transition is finished or when the
-   *                                     operation was canceled (early return).
+   * @param {Element}   anchor           the anchor for which we're opening
+   *                                     a new panelview, if any
    */
-  async _transitionViews(previousViewNode, viewNode, reverse, previousRect) {
+  async _transitionViews(previousViewNode, viewNode, reverse, previousRect, anchor) {
     // There's absolutely no need to show off our epic animation skillz when
     // the panel's not even open.
     if (this._panel.state != "open") {
       return;
     }
 
     const {window, document} = this;
 
     if (this._autoResizeWorkaroundTimer)
       window.clearTimeout(this._autoResizeWorkaroundTimer);
 
     this._transitionDetails = {
       phase: TRANSITION_PHASES.START,
-      previousViewNode, viewNode, reverse
+      previousViewNode, viewNode, reverse, anchor
     };
 
+    if (anchor)
+      anchor.setAttribute("open", "true");
+
     // Set the viewContainer dimensions to make sure only the current view is
     // visible.
     this._viewContainer.style.height = Math.max(previousRect.height, this._mainViewHeight) + "px";
     this._viewContainer.style.width = previousRect.width + "px";
     // Lock the dimensions of the window that hosts the popup panel.
     let rect = this._panel.popupBoxObject.getOuterScreenRect();
     this._panel.setAttribute("width", rect.width);
     this._panel.setAttribute("height", rect.height);
@@ -699,27 +705,30 @@ this.PanelMultiView = class {
    * Attempt to clean up the attributes and properties set by `_transitionViews`
    * above. Which attributes and properties depends on the phase the transition
    * was left from - normally that'd be `TRANSITION_PHASES.END`.
    */
   async _cleanupTransitionPhase() {
     if (!this._transitionDetails)
       return;
 
-    let {phase, previousViewNode, viewNode, reverse, resolve, listener} = this._transitionDetails;
+    let {phase, previousViewNode, viewNode, reverse, resolve, listener, anchor} = this._transitionDetails;
     this._transitionDetails = null;
 
     // Do the things we _always_ need to do whenever the transition ends or is
     // interrupted.
     this._dispatchViewEvent(previousViewNode, "ViewHiding");
     previousViewNode.removeAttribute("current");
     if (reverse)
       this._resetKeyNavigation(previousViewNode);
     this.descriptionHeightWorkaround(viewNode);
 
+    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
       // another few milliseconds to remove the width and height 'fixtures',
       // to be sure we don't flicker annoyingly.
       // NB: HACK! Bug 1363756 is there to fix this.
       this._autoResizeWorkaroundTimer = this.window.setTimeout(() => {
@@ -992,17 +1001,16 @@ this.PanelMultiView = class {
       case "popuphidden":
         // WebExtensions consumers can hide the popup from viewshowing, or
         // mid-transition, which disrupts our state:
         this._viewShowing = null;
         this._transitioning = false;
         this.node.removeAttribute("panelopen");
         this.showMainView();
         if (this.panelViews) {
-          this._cleanupTransitionPhase();
           for (let panelView of this._viewStack.children) {
             if (panelView.nodeName != "children") {
               panelView.__lastKnownBoundingRect = null;
               panelView.style.removeProperty("min-width");
               panelView.style.removeProperty("max-width");
             }
           }
           this.window.removeEventListener("keydown", this);
--- a/browser/components/extensions/ext-browserAction.js
+++ b/browser/components/extensions/ext-browserAction.js
@@ -190,22 +190,16 @@ this.browserAction = class extends Exten
         let popupURL = this.getProperty(tab, "popup");
         this.tabManager.addActiveTabPermission(tab);
 
         // Popups are shown only if a popup URL is defined; otherwise
         // a "click" event is dispatched. This is done for compatibility with the
         // Google Chrome onClicked extension API.
         if (popupURL) {
           try {
-            if (event.target.closest("panelmultiview")) {
-              // FIXME: The line below needs to change eventually, but for now:
-              // ensure the view is _always_ visible _before_ `popup.attach()` is
-              // called. PanelMultiView.jsm dictates different behavior.
-              event.target.setAttribute("current", true);
-            }
             let popup = this.getPopup(document.defaultView, popupURL);
             let attachPromise = popup.attach(event.target);
             event.detail.addBlocker(attachPromise);
             await attachPromise;
             TelemetryStopwatch.finish(POPUP_OPEN_MS_HISTOGRAM, this);
             if (this.eventQueue.length) {
               let histogram = Services.telemetry.getHistogramById(POPUP_RESULT_HISTOGRAM);
               histogram.add("popupShown");
--- a/browser/components/extensions/test/browser/browser-common.ini
+++ b/browser/components/extensions/test/browser/browser-common.ini
@@ -40,17 +40,16 @@ skip-if = os == 'linux'
 [browser_ext_browserAction_disabled.js]
 [browser_ext_browserAction_pageAction_icon.js]
 [browser_ext_browserAction_pageAction_icon_permissions.js]
 [browser_ext_browserAction_popup.js]
 skip-if = debug && (os == 'linux' && bits == 32) # Bug 1313372
 [browser_ext_browserAction_popup_preload.js]
 skip-if = (os == 'win' && !debug) # bug 1352668
 [browser_ext_browserAction_popup_resize.js]
-skip-if = os == 'mac' # Bug 1374749 will re-enable this test again.
 [browser_ext_browserAction_simple.js]
 [browser_ext_browserAction_telemetry.js]
 [browser_ext_browserAction_theme_icons.js]
 [browser_ext_browsingData_formData.js]
 [browser_ext_browsingData_history.js]
 [browser_ext_browsingData_localStorage.js]
 [browser_ext_browsingData_pluginData.js]
 [browser_ext_browsingData_serviceWorkers.js]
--- a/browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js
+++ b/browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js
@@ -151,19 +151,24 @@ async function testPopupSize(standardsMo
     await closeBrowserAction(extension, browserWin);
   }
 
 
   // Test the PanelUI panel for a menu panel button.
   let widget = getBrowserActionWidget(extension);
   CustomizableUI.addWidgetToArea(widget.id, getCustomizableUIPanelID());
 
+  let panel = browserWin.PanelUI.overflowPanel;
+  let panelMultiView = panel.firstChild;
+  let widgetId = makeWidgetId(extension.id);
+  // The 'ViewShown' event is the only way to correctly determine when the extensions'
+  // panelview has finished transitioning and is fully in view.
+  let shownPromise = BrowserTestUtils.waitForEvent(panelMultiView, "ViewShown",
+    e => (e.originalTarget.id || "").includes(widgetId));
   let browser = await openPanel(extension, browserWin);
-
-  let panel = browserWin.PanelUI.overflowPanel;
   let origPanelRect = panel.getBoundingClientRect();
 
   // Check that the panel is still positioned as expected.
   let checkPanelPosition = () => {
     is(panel.getAttribute("side"), arrowSide, "Panel arrow is positioned as expected");
 
     let panelRect = panel.getBoundingClientRect();
     if (arrowSide == "top") {
@@ -178,26 +183,20 @@ async function testPopupSize(standardsMo
       ok(panelRect.top <= origPanelRect.top, `Panel has not shrunk from original size (${panelRect.top} <= ${origPanelRect.top})`);
 
       let panelTop = browserWin.mozInnerScreenY + panelRect.top;
       ok(panelTop >= browserWin.screen.availTop, `Top of popup should be on-screen. (${panelTop} >= ${browserWin.screen.availTop})`);
     }
   };
 
   await awaitBrowserLoaded(browser);
-
-  let panelview = browser.closest("panelview");
-  // Need to wait first for the forced panel width and for the panelview's border to be gone,
-  // then for layout to happen again. Otherwise the removal of the border between views in the
-  // panelmultiview trips up our width checking causing it to be off-by-one.
-  await BrowserTestUtils.waitForCondition(() => (!panel.hasAttribute("width") && (!panelview || !panelview.style.borderInlineStart)));
-  await promiseAnimationFrame(browserWin);
+  await shownPromise;
   // Wait long enough to make sure the initial resize debouncing timer has
   // expired.
-  await delay(100);
+  await delay(500);
 
   let dims = await promiseContentDimensions(browser);
 
   is(dims.isStandards, standardsMode, "Document has the expected compat mode");
 
   // If the browser's preferred height is smaller than the initial height of the
   // panel, then it will still take up the full available vertical space. Even
   // so, we need to check that we've gotten the preferred height calculation