Bug 1354117 - only dispatch view events once, and fix synced tabs button test, r=jaws
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Tue, 01 Aug 2017 12:45:55 +0100
changeset 422379 ecf4c3d8440440aa853aca499524ccd8e70ec83d
parent 422378 06eeb43124facd718fb95cf2a92b658902f32c4b
child 422380 6e1861380a559c3fbdb2f0aae3ff7bd9459eae98
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs1354117
milestone57.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 1354117 - only dispatch view events once, and fix synced tabs button test, r=jaws Prior to this patch, both CustomizableUI itself and the PanelMultiView module tried to ensure that onViewShowing/Shown/Hiding/Hidden listeners were invoked when the relevant DOM events fired. PanelMultiView was doing this manually because CUI was only adding listeners once the corresponding widget was created. Now that the relevant views can be accessed without the corresponding widgets (via the fixed appMenu), there was no guarantee that the listeners would be attached, and this caused empty subviews. Unfortunately, if the widget *was* present, it caused events to fire more than once, which understandably broke consumers like the sync remote tabs widget, which broke the test we're fixing up here. For other views, even if they were not completely broken it at least did busy-work. This patch removes the manual event invocation, and delegates the event listener work to CUI from the PanelMultiView side. This ensures events fire, and fire only once. MozReview-Commit-ID: 94GhcrdcBuB
browser/components/customizableui/CustomizableUI.jsm
browser/components/customizableui/CustomizableWidgets.jsm
browser/components/customizableui/PanelMultiView.jsm
browser/components/customizableui/test/browser_synced_tabs_menu.js
--- a/browser/components/customizableui/CustomizableUI.jsm
+++ b/browser/components/customizableui/CustomizableUI.jsm
@@ -1448,40 +1448,51 @@ var CustomizableUIInternal = {
       if (aWidget.type == "view") {
         log.debug("Widget " + aWidget.id + " has a view. Auto-registering event handlers.");
         let viewNode = aDocument.getElementById(aWidget.viewId);
 
         if (viewNode) {
           // PanelUI relies on the .PanelUI-subView class to be able to show only
           // one sub-view at a time.
           viewNode.classList.add("PanelUI-subView");
-
-          for (let eventName of kSubviewEvents) {
-            let handler = "on" + eventName;
-            if (typeof aWidget[handler] == "function") {
-              viewNode.addEventListener(eventName, aWidget[handler]);
-            }
-          }
-
-          log.debug("Widget " + aWidget.id + " showing and hiding event handlers set.");
+          this.ensureSubviewListeners(viewNode);
         } else {
           log.error("Could not find the view node with id: " + aWidget.viewId +
                     ", for widget: " + aWidget.id + ".");
         }
       }
 
       if (aWidget.onCreated) {
         aWidget.onCreated(node);
       }
     }
 
     aWidget.instances.set(aDocument, node);
     return node;
   },
 
+  ensureSubviewListeners(viewNode) {
+    if (viewNode._addedEventListeners) {
+      return;
+    }
+    let viewId = viewNode.id;
+    let widget = [...gPalette.values()].find(w => w.viewId == viewId);
+    if (!widget) {
+      return;
+    }
+    for (let eventName of kSubviewEvents) {
+      let handler = "on" + eventName;
+      if (typeof widget[handler] == "function") {
+        viewNode.addEventListener(eventName, widget[handler]);
+      }
+    }
+    viewNode._addedEventListeners = true;
+    log.debug("Widget " + widget.id + " showing and hiding event handlers set.");
+  },
+
   getLocalizedProperty(aWidget, aProp, aFormatArgs, aDef) {
     const kReqStringProps = ["label"];
 
     if (typeof aWidget == "string") {
       aWidget = gPalette.get(aWidget);
     }
     if (!aWidget) {
       throw new Error("getLocalizedProperty was passed a non-widget to work with.");
@@ -3509,16 +3520,27 @@ this.CustomizableUI = {
    *     or if the area is not currently known to CustomizableUI.
    */
   getWidgetsInArea(aArea) {
     return this.getWidgetIdsInArea(aArea).map(
       CustomizableUIInternal.wrapWidget,
       CustomizableUIInternal
     );
   },
+
+  /**
+   * Ensure the customizable widget that matches up with this view node
+   * will get the right subview showing/shown/hiding/hidden events when
+   * they fire.
+   * @param aViewNode the view node to add listeners to if they haven't
+   *                  been added already.
+   */
+  ensureSubviewListeners(aViewNode) {
+    return CustomizableUIInternal.ensureSubviewListeners(aViewNode);
+  },
   /**
    * Obtain an array of all the area IDs known to CustomizableUI.
    * This array is created for you, so is modifiable without CustomizableUI
    * being affected.
    */
   get areas() {
     return [...gAreas.keys()];
   },
--- a/browser/components/customizableui/CustomizableWidgets.jsm
+++ b/browser/components/customizableui/CustomizableWidgets.jsm
@@ -965,16 +965,19 @@ const CustomizableWidgets = [
         if (elem.value.toLowerCase() == aCurrentItem.toLowerCase()) {
           elem.setAttribute("checked", "true");
         } else {
           elem.removeAttribute("checked");
         }
       }
     },
     onViewShowing(aEvent) {
+      if (!this._inited) {
+        this.onInit();
+      }
       let document = aEvent.target.ownerDocument;
 
       let autoDetectLabelId = "PanelUI-characterEncodingView-autodetect-label";
       let autoDetectLabel = document.getElementById(autoDetectLabelId);
       if (!autoDetectLabel.hasAttribute("value")) {
         let label = CharsetBundle.GetStringFromName("charsetMenuAutodet");
         autoDetectLabel.setAttribute("value", label);
         this.populateList(document,
@@ -1057,16 +1060,17 @@ const CustomizableWidgets = [
           CustomizableUI.removeListener(listener);
           getPanel().removeEventListener("popupshowing", updateButton);
         }
       };
       CustomizableUI.addListener(listener);
       this.onInit();
     },
     onInit() {
+      this._inited = true;
       if (!this.charsetInfo) {
         this.charsetInfo = CharsetMenu.getData();
       }
     }
   }, {
     id: "email-link-button",
     tooltiptext: "email-link-button.tooltiptext3",
     onCommand(aEvent) {
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -6,18 +6,18 @@
 
 this.EXPORTED_SYMBOLS = ["PanelMultiView"];
 
 const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "AppConstants",
   "resource://gre/modules/AppConstants.jsm");
-XPCOMUtils.defineLazyModuleGetter(this, "CustomizableWidgets",
-  "resource:///modules/CustomizableWidgets.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
+  "resource:///modules/CustomizableUI.jsm");
 
 /**
  * Simple implementation of the sliding window pattern; panels are added to a
  * linked list, in-order, and the currently shown panel is remembered using a
  * marker. The marker shifts as navigation between panels is continued, where
  * the panel at index 0 is always the starting point:
  *           ┌────┬────┬────┬────┐
  *           │▓▓▓▓│    │    │    │ Start
@@ -671,24 +671,19 @@ this.PanelMultiView = class {
    * @param  {String}    eventName Name of the event to dispatch.
    * @param  {DOMNode}   [anchor]  Node where the panel is anchored to. Optional.
    * @param  {Object}    [detail]  Event detail object. Optional.
    * @return {Boolean} `true` if the event was canceled by an event handler, `false`
    *                   otherwise.
    */
   _dispatchViewEvent(viewNode, eventName, anchor, detail) {
     let cancel = false;
-    if (this.panelViews) {
-      let custWidget = CustomizableWidgets.find(widget => widget.viewId == viewNode.id);
-      let method = "on" + eventName;
-      if (custWidget && custWidget[method]) {
-        if (anchor && custWidget.onInit)
-          custWidget.onInit(anchor);
-        custWidget[method]({ target: viewNode, preventDefault: () => cancel = true, detail });
-      }
+    if (eventName != "PanelMultiViewHidden" && eventName != "destructed") {
+      // Don't need to do this for PanelMultiViewHidden or "destructed" events
+      CustomizableUI.ensureSubviewListeners(viewNode);
     }
 
     let evt = new this.window.CustomEvent(eventName, {
       detail,
       bubbles: true,
       cancelable: eventName == "ViewShowing"
     });
     viewNode.dispatchEvent(evt);
--- a/browser/components/customizableui/test/browser_synced_tabs_menu.js
+++ b/browser/components/customizableui/test/browser_synced_tabs_menu.js
@@ -34,17 +34,16 @@ let mockedInternal = {
   get isConfiguredToSyncTabs() { return true; },
   getTabClients() { return Promise.resolve([]); },
   syncTabs() { return Promise.resolve(); },
   hasSyncedThisSession: false,
 };
 
 
 add_task(async function setup() {
-  await SpecialPowers.pushPrefEnv({set: [["browser.photon.structure.enabled", false]]});
   let oldInternal = SyncedTabs._internal;
   SyncedTabs._internal = mockedInternal;
 
   // This test hacks some observer states to simulate a user being signed
   // in to Sync - restore them when the test completes.
   let initialObserverStates = {};
   for (let id of ["sync-reauth-state", "sync-setup-state", "sync-syncnow-state"]) {
     initialObserverStates[id] = document.getElementById(id).hidden;
@@ -57,32 +56,34 @@ add_task(async function setup() {
     }
   });
 });
 
 // The test expects the about:preferences#sync page to open in the current tab
 async function openPrefsFromMenuPanel(expectedPanelId, entryPoint) {
   info("Check Sync button functionality");
   Services.prefs.setCharPref("identity.fxaccounts.remote.signup.uri", "http://example.com/");
+  CustomizableUI.addWidgetToArea("sync-button", CustomizableUI.AREA_FIXED_OVERFLOW_PANEL);
 
   // check the button's functionality
-  await PanelUI.show();
+  await document.getElementById("nav-bar").overflowable.show();
 
   if (entryPoint == "uitour") {
     UITour.tourBrowsersByWindow.set(window, new Set());
     UITour.tourBrowsersByWindow.get(window).add(gBrowser.selectedBrowser);
   }
 
   let syncButton = document.getElementById("sync-button");
   ok(syncButton, "The Sync button was added to the Panel Menu");
 
   let tabsUpdatedPromise = promiseObserverNotified("synced-tabs-menu:test:tabs-updated");
+  let syncPanel = document.getElementById("PanelUI-remotetabs");
+  let viewShownPromise = BrowserTestUtils.waitForEvent(syncPanel, "ViewShown");
   syncButton.click();
-  await tabsUpdatedPromise;
-  let syncPanel = document.getElementById("PanelUI-remotetabs");
+  await Promise.all([tabsUpdatedPromise, viewShownPromise]);
   ok(syncPanel.getAttribute("current"), "Sync Panel is in view");
 
   // Sync is not configured - verify that state is reflected.
   let subpanel = document.getElementById(expectedPanelId)
   ok(!subpanel.hidden, "sync setup element is visible");
 
   // Find and click the "setup" button.
   let setupButton = subpanel.querySelector(".PanelUI-remotetabs-prefs-button");
@@ -100,26 +101,26 @@ async function openPrefsFromMenuPanel(ex
     }
     gBrowser.selectedBrowser.addEventListener("load", handler, true);
 
   });
   newTab = gBrowser.selectedTab;
 
   is(gBrowser.currentURI.spec, "about:preferences?entrypoint=" + entryPoint + "#sync",
     "Firefox Sync preference page opened with `menupanel` entrypoint");
-  ok(!isPanelUIOpen(), "The panel closed");
+  ok(!isOverflowOpen(), "The panel closed");
 
-  if (isPanelUIOpen()) {
-    await panelUIHide();
+  if (isOverflowOpen()) {
+    await hideOverflow();
   }
 }
 
-function panelUIHide() {
-  let panelHidePromise = promisePanelHidden(window);
-  PanelUI.hide();
+function hideOverflow() {
+  let panelHidePromise = promiseOverflowHidden(window);
+  PanelUI.overflowPanel.hidePopup();
   return panelHidePromise;
 }
 
 async function asyncCleanup() {
   Services.prefs.clearUserPref("identity.fxaccounts.remote.signup.uri");
   // reset the panel UI to the default state
   await resetCustomization();
   ok(CustomizableUI.inDefaultState, "The panel UI is in default state again.");
@@ -161,20 +162,20 @@ add_task(async function() {
   let syncPanel = document.getElementById("PanelUI-remotetabs");
   let links = syncPanel.querySelectorAll(".remotetabs-promo-link");
 
   is(links.length, 2, "found 2 links as expected");
 
   // test each link and left and middle mouse buttons
   for (let link of links) {
     for (let button = 0; button < 2; button++) {
-      await PanelUI.show();
+      await document.getElementById("nav-bar").overflowable.show();
       EventUtils.sendMouseEvent({ type: "click", button }, link, window);
       // the panel should have been closed.
-      ok(!isPanelUIOpen(), "click closed the panel");
+      ok(!isOverflowOpen(), "click closed the panel");
       // should be a new tab - wait for the load.
       is(gBrowser.tabs.length, 2, "there's a new tab");
       await new Promise(resolve => {
         if (gBrowser.selectedBrowser.currentURI.spec == "about:blank") {
           gBrowser.selectedBrowser.addEventListener("load", function(e) {
             resolve();
           }, {capture: true, once: true});
           return;
@@ -187,41 +188,43 @@ add_task(async function() {
       let os = link.getAttribute("mobile-promo-os");
       let expectedUrl = `http://example.com/?os=${os}&tail=synced-tabs`;
       is(gBrowser.selectedBrowser.currentURI.spec, expectedUrl, "correct URL");
       gBrowser.removeTab(gBrowser.selectedTab);
     }
   }
 
   // test each link and right mouse button - should be a noop.
-  await PanelUI.show();
+  await document.getElementById("nav-bar").overflowable.show();
   for (let link of links) {
     EventUtils.sendMouseEvent({ type: "click", button: 2 }, link, window);
     // the panel should still be open
-    ok(isPanelUIOpen(), "panel remains open after right-click");
+    ok(isOverflowOpen(), "panel remains open after right-click");
     is(gBrowser.tabs.length, 1, "no new tab was opened");
   }
-  await panelUIHide();
+  await hideOverflow();
 
   Services.prefs.clearUserPref("identity.mobilepromo.android");
   Services.prefs.clearUserPref("identity.mobilepromo.ios");
 });
 
 // Test the "Sync Now" button
 add_task(async function() {
   // configure our broadcasters so we are in the right state.
   document.getElementById("sync-reauth-state").hidden = true;
   document.getElementById("sync-setup-state").hidden = true;
   document.getElementById("sync-syncnow-state").hidden = false;
 
-  await PanelUI.show();
+  await document.getElementById("nav-bar").overflowable.show();
   let tabsUpdatedPromise = promiseObserverNotified("synced-tabs-menu:test:tabs-updated");
-  document.getElementById("sync-button").click();
-  await tabsUpdatedPromise;
   let syncPanel = document.getElementById("PanelUI-remotetabs");
+  let viewShownPromise = BrowserTestUtils.waitForEvent(syncPanel, "ViewShown");
+  let syncButton = document.getElementById("sync-button");
+  syncButton.click();
+  await Promise.all([tabsUpdatedPromise, viewShownPromise]);
   ok(syncPanel.getAttribute("current"), "Sync Panel is in view");
 
   let subpanel = document.getElementById("PanelUI-remotetabs-main")
   ok(!subpanel.hidden, "main pane is visible");
   let deck = document.getElementById("PanelUI-remotetabs-deck");
 
   // The widget is still fetching tabs, as we've neutered everything that
   // provides them
@@ -339,17 +342,17 @@ add_task(async function() {
   // There is a single node saying there's no tabs for the client.
   node = node.nextSibling;
   is(node.nodeName, "label", "node is a label");
   is(node.getAttribute("itemtype"), "", "node is neither a tab nor a client");
 
   node = node.nextSibling;
   is(node, null, "no more entries");
 
-  await panelUIHide();
+  await hideOverflow();
 });
 
 // Test the pagination capabilities (Show More/All tabs)
 add_task(async function() {
   mockedInternal.getTabClients = () => {
     return Promise.resolve([
       {
         id: "guid_desktop",
@@ -370,23 +373,25 @@ add_task(async function() {
     ]);
   };
 
   // configure our broadcasters so we are in the right state.
   document.getElementById("sync-reauth-state").hidden = true;
   document.getElementById("sync-setup-state").hidden = true;
   document.getElementById("sync-syncnow-state").hidden = false;
 
-  await PanelUI.show();
+  await document.getElementById("nav-bar").overflowable.show();
   let tabsUpdatedPromise = promiseObserverNotified("synced-tabs-menu:test:tabs-updated");
-  document.getElementById("sync-button").click();
-  await tabsUpdatedPromise;
+  let syncPanel = document.getElementById("PanelUI-remotetabs");
+  let viewShownPromise = BrowserTestUtils.waitForEvent(syncPanel, "ViewShown");
+  let syncButton = document.getElementById("sync-button");
+  syncButton.click();
+  await Promise.all([tabsUpdatedPromise, viewShownPromise]);
 
   // Check pre-conditions
-  let syncPanel = document.getElementById("PanelUI-remotetabs");
   ok(syncPanel.getAttribute("current"), "Sync Panel is in view");
   let subpanel = document.getElementById("PanelUI-remotetabs-main")
   ok(!subpanel.hidden, "main pane is visible");
   let deck = document.getElementById("PanelUI-remotetabs-deck");
   is(deck.selectedIndex, DECKINDEX_TABS, "we should be showing tabs");
 
   function checkTabsPage(tabsShownCount, showMoreLabel) {
     let tabList = document.getElementById("PanelUI-remotetabs-tabslist");
@@ -423,10 +428,10 @@ add_task(async function() {
   showMoreButton = checkTabsPage(50, "Show More");
   await clickShowMoreButton();
 
   showMoreButton = checkTabsPage(72, "Show All");
   await clickShowMoreButton();
 
   checkTabsPage(77, null);
 
-  await panelUIHide();
+  await hideOverflow();
 });