Bug 1484275 - Fix opening the main menu while another popup is open on Windows. r=Gijs
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Thu, 23 Aug 2018 14:04:43 +0100
changeset 433210 abd29769ce45f110299ef6bf4c667d5df496d0c1
parent 433209 f433ed4ff1de863250d6ebf82b5bc5afcea9b6c7
child 433211 3eff5274ee785a1f8ecaa2467ee061323962cbe8
push id107003
push userpaolo.mozmail@amadzone.org
push dateFri, 24 Aug 2018 07:09:32 +0000
treeherdermozilla-inbound@abd29769ce45 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1484275
milestone63.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 1484275 - Fix opening the main menu while another popup is open on Windows. r=Gijs Differential Revision: https://phabricator.services.mozilla.com/D3979
browser/components/customizableui/PanelMultiView.jsm
browser/components/customizableui/test/browser.ini
browser/components/customizableui/test/browser_1484275_PanelMultiView_toggle_with_other_popup.js
browser/components/customizableui/test/browser_PanelMultiView.js
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -511,16 +511,25 @@ var PanelMultiView = class extends Assoc
       }
       // We have to set canCancel to false before opening the popup because the
       // hidePopup method of PanelMultiView can be re-entered by event handlers.
       // If the openPopup call fails, however, we still have to dispatch the
       // "popuphidden" event even if canCancel was set to false.
       try {
         canCancel = false;
         this._panel.openPopup(...args);
+
+        // On Windows, if another popup is hiding while we call openPopup, the
+        // call won't fail but the popup won't open. In this case, we have to
+        // dispatch an artificial "popuphidden" event to reset our state.
+        if (this._panel.state == "closed" && this.openViews.length) {
+          this.dispatchCustomEvent("popuphidden");
+          return false;
+        }
+
         return true;
       } catch (ex) {
         this.dispatchCustomEvent("popuphidden");
         throw ex;
       }
     });
   }
 
@@ -1051,18 +1060,20 @@ var PanelMultiView = class extends Assoc
     // sense for all platforms. If the arrow visuals change significantly,
     // this value will be easy to adjust.
     const EXTRA_MARGIN_PX = 20;
     maxHeight -= EXTRA_MARGIN_PX;
     return maxHeight;
   }
 
   handleEvent(aEvent) {
-    if (aEvent.type.startsWith("popup") && aEvent.target != this._panel) {
-      // Shouldn't act on e.g. context menus being shown from within the panel.
+    // Only process actual popup events from the panel or events we generate
+    // ourselves, but not from menus being shown from within the panel.
+    if (aEvent.type.startsWith("popup") && aEvent.target != this._panel &&
+                                           aEvent.target != this.node) {
       return;
     }
     switch (aEvent.type) {
       case "keydown":
         // Since we start listening for the "keydown" event when the popup is
         // already showing and stop listening when the panel is hidden, we
         // always have at least one view open.
         let currentView = this.openViews[this.openViews.length - 1];
--- a/browser/components/customizableui/test/browser.ini
+++ b/browser/components/customizableui/test/browser.ini
@@ -139,16 +139,17 @@ skip-if = os == "linux" # crashing on Li
 [browser_1087303_button_fullscreen.js]
 tags = fullscreen
 skip-if = os == "mac"
 [browser_1087303_button_preferences.js]
 [browser_1089591_still_customizable_after_reset.js]
 [browser_1096763_seen_widgets_post_reset.js]
 [browser_1161838_inserted_new_default_buttons.js]
 skip-if = verify
+[browser_1484275_PanelMultiView_toggle_with_other_popup.js]
 [browser_allow_dragging_removable_false.js]
 [browser_bootstrapped_custom_toolbar.js]
 [browser_currentset_post_reset.js]
 [browser_customizemode_contextmenu_menubuttonstate.js]
 [browser_customizemode_dragspace.js]
 skip-if = os == "linux" # linux doesn't get drag space (no tabsintitlebar)
 [browser_customizemode_uidensity.js]
 [browser_drag_outside_palette.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/customizableui/test/browser_1484275_PanelMultiView_toggle_with_other_popup.js
@@ -0,0 +1,87 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const TEST_URL = "data:text/html,<html><body></body></html>";
+
+// This code can be consolidated in the EventUtils module (bug 1126772).
+const isWindows = AppConstants.platform == "win";
+const isMac = AppConstants.platform == "macosx";
+const mouseDown = isWindows ? 2 : isMac ? 1 : 4;
+const mouseUp = isWindows ? 4 : isMac ? 2 : 7;
+const utils = window.windowUtils;
+const scale = utils.screenPixelsPerCSSPixel;
+function synthesizeNativeMouseClick(aElement) {
+  let rect = aElement.getBoundingClientRect();
+  let win = aElement.ownerGlobal;
+  let x = win.mozInnerScreenX + (rect.left + rect.right) / 2;
+  let y = win.mozInnerScreenY + (rect.top + rect.bottom) / 2;
+
+  // Wait for the mouseup event to occur before continuing.
+  return new Promise((resolve, reject) => {
+    function eventOccurred(e) {
+      aElement.removeEventListener("mouseup", eventOccurred, true);
+      resolve();
+    }
+
+    aElement.addEventListener("mouseup", eventOccurred, true);
+
+    utils.sendNativeMouseEvent(x * scale, y * scale, mouseDown, 0, null);
+    utils.sendNativeMouseEvent(x * scale, y * scale, mouseUp, 0, null);
+  });
+}
+
+/**
+ * Test steps that may lead to the panel being stuck on Windows (bug 1484275).
+ */
+add_task(async function test_PanelMultiView_toggle_with_other_popup() {
+  // For proper cleanup, create a bookmark that we will remove later.
+  let bookmark = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    url: TEST_URL,
+  });
+  registerCleanupFunction(() => PlacesUtils.bookmarks.remove(bookmark));
+
+  await BrowserTestUtils.withNewTab({
+    gBrowser,
+    url: TEST_URL,
+  }, async function(browser) {
+    // 1. Open the main menu.
+    await gCUITestUtils.openMainMenu();
+
+    // 2. Open another popup not managed by PanelMultiView.
+    let bookmarkPanel = document.getElementById("editBookmarkPanel");
+    let shown = BrowserTestUtils.waitForEvent(bookmarkPanel, "popupshown");
+    let hidden = BrowserTestUtils.waitForEvent(bookmarkPanel, "popuphidden");
+    EventUtils.synthesizeKey("D", { accelKey: true });
+    await shown;
+
+    // 3. Click the button to which the main menu is anchored. We need a native
+    // mouse event to simulate the exact platform behavior with popups.
+    let clickFn = () => synthesizeNativeMouseClick(
+      document.getElementById("PanelUI-button"));
+
+    if (AppConstants.platform == "win") {
+      // On Windows, the operation will close both popups.
+      await gCUITestUtils.hidePanelMultiView(PanelUI.panel, clickFn);
+      await new Promise(resolve => executeSoon(resolve));
+
+      // 4. Test that the popup can be opened again after it's been closed.
+      await gCUITestUtils.openMainMenu();
+      Assert.equal(PanelUI.panel.state, "open");
+    } else {
+      // On other platforms, the operation will close both popups and reopen the
+      // main menu immediately, so we wait for the reopen to occur.
+      shown = BrowserTestUtils.waitForEvent(PanelUI.mainView, "ViewShown");
+      clickFn();
+      await shown;
+    }
+
+    await gCUITestUtils.hideMainMenu();
+
+    // Make sure the events for the bookmarks panel have also been processed
+    // before closing the tab and removing the bookmark.
+    await hidden;
+  });
+});
--- a/browser/components/customizableui/test/browser_PanelMultiView.js
+++ b/browser/components/customizableui/test/browser_PanelMultiView.js
@@ -401,16 +401,17 @@ add_task(async function test_cancel_main
                            "bottomcenter topright");
   await promiseHidden;
 
   stopRecordingEvents(gPanels[0]);
 
   Assert.deepEqual(recordArray, [
     "panelview-0: ViewShowing",
     "panelview-0: ViewHiding",
+    "panelmultiview-0: PanelMultiViewHidden",
     "panelmultiview-0: popuphidden",
   ]);
 });
 
 /**
  * Tests the event sequence when opening a subview is canceled.
  */
 add_task(async function test_cancel_subview_event_sequence() {
@@ -470,18 +471,19 @@ add_task(async function test_close_while
                            "bottomcenter topright");
   await promiseHiding;
   await promiseHidden;
 
   stopRecordingEvents(gPanels[0]);
 
   Assert.deepEqual(recordArray, [
     "panelview-0: ViewShowing",
+    "panelview-0: ViewShowing > panelview-0: ViewHiding",
+    "panelview-0: ViewShowing > panelmultiview-0: PanelMultiViewHidden",
     "panelview-0: ViewShowing > panelmultiview-0: popuphidden",
-    "panelview-0: ViewShowing > panelview-0: ViewHiding",
   ]);
 });
 
 /**
  * Tests the event sequence when closing the panel while opening a subview.
  */
 add_task(async function test_close_while_showing_subview_event_sequence() {
   let recordArray = [];