Bug 555547 - A command of placesContextMenu is carried out for a wrong bookmark item.
authorAsaf Romano <aromano@mozilla.com>
Wed, 02 Feb 2011 17:00:52 +0100
changeset 61780 58138404336ff2ad6d5ae9dcfc4fadcefa3cfae5
parent 61779 17b4a1ed16af5fa748ff9557b7cd05333516c623
child 61781 7abdd2abd9cbf3976fbccd5de41c2f162cdc9c8a
push id18486
push usermak77@bonardo.net
push dateWed, 02 Feb 2011 16:02:40 +0000
treeherdermozilla-central@7abdd2abd9cb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs555547
milestone2.0b11pre
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 555547 - A command of placesContextMenu is carried out for a wrong bookmark item. r=mak a=blocking
browser/components/places/content/browserPlacesViews.js
browser/components/places/content/controller.js
browser/components/places/content/tree.xml
browser/components/places/tests/browser/Makefile.in
browser/components/places/tests/browser/browser_555547.js
--- a/browser/components/places/content/browserPlacesViews.js
+++ b/browser/components/places/content/browserPlacesViews.js
@@ -52,16 +52,18 @@ function PlacesViewBase(aPlace) {
   this._viewElt.controllers.appendController(this._controller);
 }
 
 PlacesViewBase.prototype = {
   // The xul element that holds the entire view.
   _viewElt: null,
   get viewElt() this._viewElt,
 
+  get controllers() this._viewElt.controllers,
+
   // The xul element that represents the root container.
   _rootElt: null,
 
   // Set to true for views that are represented by native widgets (i.e.
   // the native mac menu).
   _nativeView: false,
 
   QueryInterface: XPCOMUtils.generateQI(
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -1579,30 +1579,32 @@ function goUpdatePlacesCommands() {
   updatePlacesCommand("placesCmd_cut");
   updatePlacesCommand("placesCmd_copy");
   updatePlacesCommand("placesCmd_paste");
   updatePlacesCommand("placesCmd_delete");
 }
 
 function doGetPlacesControllerForCommand(aCommand)
 {
+  // A context menu may be built for non-focusable views.  Thus, we first try
+  // to look for a view associated with document.popupNode
+  let popupNode = document.popupNode;
+  if (popupNode) {
+    let view = PlacesUIUtils.getViewForNode(popupNode);
+    if (view && view._contextMenuShown)
+      return view.controllers.getControllerForCommand(aCommand);
+  }
+
+  // When we're not building a context menu, only focusable views
+  // are possible.  Thus, we can safely use the command dispatcher.
   let controller = top.document.commandDispatcher
                       .getControllerForCommand(aCommand);
   if (controller)
     return controller;
 
-  // If building commands for a context menu, look for an element in the
-  // current popup.
-  let element = document.popupNode;
-  if (element) {
-    let view = PlacesUIUtils.getViewForNode(element);
-    if (view && view._contextMenuShown)
-      return view.viewElt.controllers.getControllerForCommand(aCommand);
-  }
-
   return null;
 }
 
 function goDoPlacesCommand(aCommand)
 {
   let controller = doGetPlacesControllerForCommand(aCommand);
   if (controller && controller.isCommandEnabled(aCommand))
     controller.doCommand(aCommand);
--- a/browser/components/places/content/tree.xml
+++ b/browser/components/places/content/tree.xml
@@ -660,25 +660,29 @@
             if (index == Ci.nsINavHistoryResultTreeViewer.INDEX_INVISIBLE)
               continue;
             selection.rangedSelect(index, index, true);
           }
           selection.selectEventsSuppressed = false;
         ]]></body>
       </method>
 
+      <field name="_contextMenuShown">false</field>
+
       <method name="buildContextMenu">
         <parameter name="aPopup"/>
         <body><![CDATA[
+          this._contextMenuShown = true;
           return this.controller.buildContextMenu(aPopup);
         ]]></body>
       </method>
 
       <method name="destroyContextMenu">
         <parameter name="aPopup"/>
+          this._contextMenuShown = false;
         <body/>
       </method>
     </implementation>
     <handlers>
       <handler event="focus"><![CDATA[
         this._cachedInsertionPoint = undefined;
 
         // See select handler. We need the sidebar's places commandset to be
--- a/browser/components/places/tests/browser/Makefile.in
+++ b/browser/components/places/tests/browser/Makefile.in
@@ -67,12 +67,13 @@ include $(topsrcdir)/config/rules.mk
 	sidebarpanels_click_test_page.html \
 	browser_library_infoBox.js \
 	browser_markPageAsFollowedLink.js \
 	framedPage.html \
 	frameLeft.html \
 	frameRight.html \
 	browser_toolbar_migration.js \
 	browser_library_batch_delete.js \
+	browser_555547.js \
 	$(NULL)
 
 libs:: $(_BROWSER_TEST_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/browser/$(relativesrcdir)
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_555547.js
@@ -0,0 +1,59 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+
+function test() {
+  waitForExplicitFinish();
+
+  let sidebarBox = document.getElementById("sidebar-box");
+  is(sidebarBox.hidden, true, "The sidebar should be hidden");
+
+  // Uncollapse the personal toolbar if needed.
+  let toolbar = document.getElementById("PersonalToolbar");
+  let wasCollapsed = toolbar.collapsed;
+  if (wasCollapsed)
+    setToolbarVisibility(toolbar, true);
+
+  let sidebar = document.getElementById("sidebar");
+  sidebar.addEventListener("load", function() {
+    sidebar.removeEventListener("load", arguments.callee, true);
+    let tree = sidebar.contentDocument.getElementById("bookmarks-view");
+
+    // The Places view is build on load, so we enqueue our test.
+    executeSoon(function() {
+      // Focus the tree and check if its controller is returned.
+      tree.focus();
+
+      let controller = doGetPlacesControllerForCommand("placesCmd_copy");
+      let treeController = tree.controllers
+                               .getControllerForCommand("placesCmd_copy");
+      ok(controller == treeController, "tree controller was returned");
+
+      // Open the context menu for a toolbar item, and check if the toolbar's
+      // controller is returned.
+      let toolbarItems = document.getElementById("PlacesToolbarItems");
+      EventUtils.synthesizeMouse(toolbarItems.childNodes[0],
+                                 4, 4, { type: "contextmenu", button: 2 },
+                                 window);
+      controller = doGetPlacesControllerForCommand("placesCmd_copy");
+      let toolbarController = document.getElementById("PlacesToolbar")
+                                      .controllers
+                                      .getControllerForCommand("placesCmd_copy");
+      ok(controller == toolbarController, "the toolbar controller was returned");
+
+      document.getElementById("placesContext").hidePopup();
+
+      // Now that the context menu is closed, try to get the tree controller again.
+      tree.focus();
+      controller = doGetPlacesControllerForCommand("placesCmd_copy");
+      ok(controller == treeController, "tree controller was returned");
+
+      toggleSidebar();
+      if (wasCollapsed)
+        setToolbarVisibility(toolbar, false);
+
+      finish();
+    });
+  }, true);
+  toggleSidebar("viewBookmarksSidebar", true);
+}