Bug 969780 - Australis - places controller gets lost if bookmarks button is moved to overflow or moved outside of customize mode, r=mak
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Tue, 18 Feb 2014 18:29:51 +0000
changeset 169377 4e44d3d8917ae74d6ecd2509f06ddb8a09989b10
parent 169376 92375c1f07ef09c152e69a4185d878fb1f270eb9
child 169378 42218965c4ae72f1fed3e78e4b7abee32d4395e5
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersmak
bugs969780
milestone30.0a1
Bug 969780 - Australis - places controller gets lost if bookmarks button is moved to overflow or moved outside of customize mode, r=mak
browser/base/content/browser-customization.js
browser/base/content/browser-places.js
browser/base/content/browser.js
browser/components/places/content/browserPlacesViews.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_toolbarbutton_menu_context.js
--- a/browser/base/content/browser-customization.js
+++ b/browser/base/content/browser-customization.js
@@ -36,31 +36,29 @@ let CustomizationHandler = {
     let cmd = document.getElementById("cmd_CustomizeToolbars");
     cmd.setAttribute("disabled", "true");
 
     UpdateUrlbarSearchSplitterState();
 
     CombinedStopReload.uninit();
     CombinedBackForward.uninit();
     PlacesToolbarHelper.customizeStart();
-    BookmarkingUI.customizeStart();
     DownloadsButton.customizeStart();
 
     // The additional padding on the sides of the browser
     // can cause the customize tab to get clipped.
     let tabContainer = gBrowser.tabContainer;
     if (tabContainer.getAttribute("overflow") == "true") {
       let tabstrip = tabContainer.mTabstrip;
       tabstrip.ensureElementIsVisible(gBrowser.selectedTab, true);
     }
   },
 
   _customizationChange: function() {
     gHomeButton.updatePersonalToolbarStyle();
-    BookmarkingUI.customizeChange();
     PlacesToolbarHelper.customizeChange();
   },
 
   _customizationEnding: function(aDetails) {
     // Update global UI elements that may have been added or removed
     if (aDetails.changed) {
       gURLBar = document.getElementById("urlbar");
 
@@ -79,17 +77,16 @@ let CustomizationHandler = {
       if (!window.__lookupGetter__("PopupNotifications")) {
         PopupNotifications.iconBox =
           document.getElementById("notification-popup-box");
       }
 
     }
 
     PlacesToolbarHelper.customizeDone();
-    BookmarkingUI.customizeDone();
     DownloadsButton.customizeDone();
 
     // The url bar splitter state is dependent on whether stop/reload
     // and the location bar are combined, so we need this ordering
     CombinedStopReload.init();
     CombinedBackForward.init();
     UpdateUrlbarSearchSplitterState();
 
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -1148,34 +1148,64 @@ let BookmarkingUI = {
   _uninitView: function BUI__uninitView() {
     // When an element with a placesView attached is removed and re-inserted,
     // XBL reapplies the binding causing any kind of issues and possible leaks,
     // so kill current view and let popupshowing generate a new one.
     if (this.button._placesView)
       this.button._placesView.uninit();
   },
 
-  customizeStart: function BUI_customizeStart() {
-    this._uninitView();
+  onCustomizeStart: function BUI_customizeStart(aWindow) {
+    if (aWindow == window) {
+      this._uninitView();
+      this._isCustomizing = true;
+    }
   },
 
-  customizeChange: function BUI_customizeChange() {
+  onWidgetAdded: function BUI_widgetAdded(aWidgetId) {
+    if (aWidgetId != "bookmarks-menu-button") {
+      return;
+    }
+
     let usedToUpdateStarState = this._shouldUpdateStarState();
     this._updateCustomizationState();
-    if (usedToUpdateStarState != this._shouldUpdateStarState()) {
+    if (!usedToUpdateStarState && this._shouldUpdateStarState()) {
       this.updateStarState();
+    } else if (usedToUpdateStarState && !this._shouldUpdateStarState()) {
+      this._updateStar();
+    }
+    // If we're moved outside of customize mode, we need to uninit
+    // our view so it gets reconstructed.
+    if (!this._isCustomizing) {
+      this._uninitView();
     }
     this._updateToolbarStyle();
   },
 
-  customizeDone: function BUI_customizeDone() {
-    this.onToolbarVisibilityChange();
+  onWidgetRemoved: function BUI_widgetRemoved(aWidgetId) {
+    if (aWidgetId != "bookmarks-menu-button") {
+      return;
+    }
+    // If we're moved outside of customize mode, we need to uninit
+    // our view so it gets reconstructed.
+    if (!this._isCustomizing) {
+      this._uninitView();
+    }
+    this._updateCustomizationState();
     this._updateToolbarStyle();
   },
 
+  onCustomizeEnd: function BUI_customizeEnd(aWindow) {
+    if (aWindow == window) {
+      this._isCustomizing = false;
+      this.onToolbarVisibilityChange();
+      this._updateToolbarStyle();
+    }
+  },
+
   init: function() {
     CustomizableUI.addListener(this);
     this._updateCustomizationState();
   },
 
   _hasBookmarksObserver: false,
   _itemIds: [],
   uninit: function BUI_uninit() {
@@ -1189,21 +1219,24 @@ let BookmarkingUI = {
     }
 
     if (this._pendingStmt) {
       this._pendingStmt.cancel();
       delete this._pendingStmt;
     }
   },
 
-  updateStarState: function BUI_updateStarState() {
+  onLocationChange: function BUI_onLocationChange() {
     if (this._uri && gBrowser.currentURI.equals(this._uri)) {
       return;
     }
+    this.updateStarState();
+  },
 
+  updateStarState: function BUI_updateStarState() {
     // Reset tracked values.
     this._uri = gBrowser.currentURI;
     this._itemIds = [];
 
     if (this._pendingStmt) {
       this._pendingStmt.cancel();
       delete this._pendingStmt;
     }
@@ -1482,20 +1515,19 @@ let BookmarkingUI = {
     }
   },
 
   onWidgetUnderflow: function(aNode, aContainer) {
     let win = aNode.ownerDocument.defaultView;
     if (aNode.id != "bookmarks-menu-button" || win != window)
       return;
 
-    // If the button hasn't been in the overflow panel before, we may ignore
-    // this event.
-    if (!this._starButtonLabel)
-      return;
+    // The view gets broken by being removed and reinserted. Uninit
+    // here so popupshowing will generate a new one:
+    this._uninitView();
 
     if (aNode.getAttribute("label") != this._starButtonLabel)
       aNode.setAttribute("label", this._starButtonLabel);
   },
 
   QueryInterface: XPCOMUtils.generateQI([
     Ci.nsINavBookmarkObserver
   ])
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -3605,18 +3605,17 @@ var XULBrowserWindow = {
         this.reloadCommand.setAttribute("disabled", "true");
       } else {
         this.reloadCommand.removeAttribute("disabled");
       }
 
       if (gURLBar) {
         URLBarSetURI(aLocationURI);
 
-        // Update starring UI
-        BookmarkingUI.updateStarState();
+        BookmarkingUI.onLocationChange();
         SocialUI.updateState();
       }
 
       // Utility functions for disabling find
       var shouldDisableFind = function shouldDisableFind(aDocument) {
         let docElt = aDocument.documentElement;
         return docElt && docElt.getAttribute("disablefastfind") == "true";
       }
--- a/browser/components/places/content/browserPlacesViews.js
+++ b/browser/components/places/content/browserPlacesViews.js
@@ -716,18 +716,26 @@ PlacesViewBase.prototype = {
       this._result.removeObserver(this);
       this._resultNode.containerOpen = false;
       this._resultNode = null;
       this._result = null;
     }
 
     if (this._controller) {
       this._controller.terminate();
-      this._viewElt.controllers.removeController(this._controller);
-      this._controller = null;
+      // Removing the controller will fail if it is already no longer there.
+      // This can happen if the view element was removed/reinserted without
+      // our knowledge. There is no way to check for that having happened
+      // without the possibility of an exception. :-(
+      try {
+        this._viewElt.controllers.removeController(this._controller);
+      } catch (ex) {
+      } finally {
+        this._controller = null;
+      }
     }
 
     delete this._viewElt._placesView;
   },
 
   get isRTL() {
     if ("_isRTL" in this)
       return this._isRTL;
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -40,8 +40,9 @@ skip-if = true
 [browser_library_infoBox.js]
 [browser_markPageAsFollowedLink.js]
 [browser_toolbar_migration.js]
 [browser_library_batch_delete.js]
 [browser_555547.js]
 [browser_416459_cut.js]
 [browser_library_downloads.js]
 [browser_library_left_pane_select_hierarchy.js]
+[browser_toolbarbutton_menu_context.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_toolbarbutton_menu_context.js
@@ -0,0 +1,51 @@
+let bookmarksMenuButton = document.getElementById("bookmarks-menu-button");
+let BMB_menuPopup = document.getElementById("BMB_bookmarksPopup");
+let BMB_showAllBookmarks = document.getElementById("BMB_bookmarksShowAll");
+let contextMenu = document.getElementById("placesContext");
+let newBookmarkItem = document.getElementById("placesContext_new:bookmark");
+
+waitForExplicitFinish();
+add_task(function testPopup() {
+  info("Checking popup context menu before moving the bookmarks button");
+  yield checkPopupContextMenu();
+  let pos = CustomizableUI.getPlacementOfWidget("bookmarks-menu-button").position;
+  CustomizableUI.addWidgetToArea("bookmarks-menu-button", CustomizableUI.AREA_PANEL);
+  CustomizableUI.addWidgetToArea("bookmarks-menu-button", CustomizableUI.AREA_NAVBAR, pos);
+  info("Checking popup context menu after moving the bookmarks button");
+  yield checkPopupContextMenu();
+});
+
+function* checkPopupContextMenu() {
+  let dropmarker = document.getAnonymousElementByAttribute(bookmarksMenuButton, "anonid", "dropmarker");
+  let popupShownPromise = onPopupEvent(BMB_menuPopup, "shown");
+  EventUtils.synthesizeMouseAtCenter(dropmarker, {});
+  info("Waiting for bookmarks menu to be shown.");
+  yield popupShownPromise;
+  let contextMenuShownPromise = onPopupEvent(contextMenu, "shown");
+  EventUtils.synthesizeMouseAtCenter(BMB_showAllBookmarks, {type: "contextmenu", button: 2 });
+  info("Waiting for context menu on bookmarks menu to be shown.");
+  yield contextMenuShownPromise;
+  ok(!newBookmarkItem.hasAttribute("disabled"), "New bookmark item shouldn't be disabled");
+  let contextMenuHiddenPromise = onPopupEvent(contextMenu, "hidden");
+  contextMenu.hidePopup();
+  info("Waiting for context menu on bookmarks menu to be hidden.");
+  yield contextMenuHiddenPromise;
+  let popupHiddenPromise = onPopupEvent(BMB_menuPopup, "hidden");
+  // Can't use synthesizeMouseAtCenter because the dropdown panel is in the way
+  EventUtils.synthesizeMouse(dropmarker, 2, 2, {});
+  info("Waiting for bookmarks menu to be hidden.");
+  yield popupHiddenPromise;
+}
+
+function onPopupEvent(popup, evt) {
+  let fullEvent = "popup" + evt;
+  let deferred = new Promise.defer();
+  let onPopupHandler = (e) => {
+    if (e.target == popup) {
+      popup.removeEventListener(fullEvent, onPopupHandler);
+      deferred.resolve();
+    }
+  };
+  popup.addEventListener(fullEvent, onPopupHandler);
+  return deferred.promise;
+}