Bug 1354117 - only dispatch view events once, and fix synced tabs button test, r?jaws draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Tue, 01 Aug 2017 12:45:55 +0100
changeset 621134 502b4cb476d3c1717f9fa8523054181e5c9aadd8
parent 621133 3ebe83d2578d183ae67569ea40e8b1ed96fedbc3
child 621135 e4b19cf307976aa52e2117dedb30f11155dc0719
push id72277
push usergijskruitbosch@gmail.com
push dateFri, 04 Aug 2017 12:03:53 +0000
reviewersjaws
bugs1354117
milestone57.0a1
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();
 });