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

--HG--
extra : rebase_source : 1bdace324f22ee95002024fe68b572a16dd25aac

browser/components/customizableui/PanelMultiView.jsm

@@ -432,7 +432,15 @@ this.PanelMultiView = class {
         });
       }
     } 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) {
@@ -526,6 +534,8 @@ this.PanelMultiView = class {
         } 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);
@@ -534,13 +544,7 @@ this.PanelMultiView = class {
 
       // Now we have to transition the panel.
       if (this.panelViews && playTransition) {
-        if (aAnchor)
+        await this._transitionViews(previousViewNode, viewNode, reverse, previousRect, aAnchor);
-          aAnchor.setAttribute("open", true);
-
-        await this._transitionViews(previousViewNode, viewNode, reverse, previousRect);
-
-        if (aAnchor)
-          aAnchor.removeAttribute("open");
 
         this._dispatchViewEvent(viewNode, "ViewShown");
         this._updateKeyboardFocus(viewNode);
@@ -577,11 +581,10 @@ this.PanelMultiView = class {
    *                                     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
+   * @param {Element}   anchor           the anchor for which we're opening
-   *                                     transition is finished or when the
+   *                                     a new panelview, if any
-   *                                     operation was canceled (early return).
    */
-  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") {
@@ -595,9 +598,12 @@ this.PanelMultiView = class {
 
     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";
@@ -704,7 +710,7 @@ this.PanelMultiView = class {
     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
@@ -715,6 +721,9 @@ this.PanelMultiView = class {
       this._resetKeyNavigation(previousViewNode);
     this.descriptionHeightWorkaround(viewNode);
 
+    if (anchor)
+      anchor.removeAttribute("open");
+
     if (phase >= TRANSITION_PHASES.START) {
       this._panel.removeAttribute("width");
       this._panel.removeAttribute("height");
@@ -997,7 +1006,6 @@ this.PanelMultiView = class {
         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;

browser/components/extensions/ext-browserAction.js

@@ -195,12 +195,6 @@ this.browserAction = class extends ExtensionAPI {
         // 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);

browser/components/extensions/test/browser/browser-common.ini

@@ -45,7 +45,6 @@ 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/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js

@@ -156,9 +156,14 @@ async function testPopupSize(standardsMode, browserWin = window, arrowSide = "to
   let widget = getBrowserActionWidget(extension);
   CustomizableUI.addWidgetToArea(widget.id, getCustomizableUIPanelID());
 
-  let browser = await openPanel(extension, browserWin);
-
   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 origPanelRect = panel.getBoundingClientRect();
 
   // Check that the panel is still positioned as expected.
@@ -183,16 +188,10 @@ async function testPopupSize(standardsMode, browserWin = window, arrowSide = "to
   };
 
   await awaitBrowserLoaded(browser);
-
+  await shownPromise;
-  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);
   // Wait long enough to make sure the initial resize debouncing timer has
   // expired.
-  await delay(100);
+  await delay(500);
 
   let dims = await promiseContentDimensions(browser);