Hide and disable open_all/cut/copy/delete/properties when opening

bookmarks/history context menu with no selection

This resolves #882 (by not offering options that can't be used)

6 files changed, 68 insertions, 56 deletions

diff --git a/application/palemoon/components/places/content/browserPlacesViews.js b/application/palemoon/components/places/content/browserPlacesViews.js
index eec7274a4d..8b90dd280e 100644
--- a/application/palemoon/components/places/content/browserPlacesViews.js
+++ b/application/palemoon/components/places/content/browserPlacesViews.js
@@ -109,8 +109,15 @@ PlacesViewBase.prototype = {
 
   get selectedNode() {
     if (this._contextMenuShown) {
-      let popup = document.popupNode;
-      return popup._placesNode || popup.parentNode._placesNode || null;
+      let anchor = this._contextMenuShown.triggerNode;
+      if (!anchor)
+        return null;
+
+      if (anchor._placesNode)
+        return this._rootElt == anchor ? null : anchor._placesNode;
+
+      anchor = anchor.parentNode;
+      return this._rootElt == anchor ? null : (anchor._placesNode || null);
     }
     return null;
   },
@@ -176,13 +183,13 @@ PlacesViewBase.prototype = {
   },
 
   buildContextMenu: function PVB_buildContextMenu(aPopup) {
-    this._contextMenuShown = true;
+    this._contextMenuShown = aPopup;
     window.updateCommands("places");
     return this.controller.buildContextMenu(aPopup);
   },
 
   destroyContextMenu: function PVB_destroyContextMenu(aPopup) {
-    this._contextMenuShown = false;
+    this._contextMenuShown = null;
   },
 
   _cleanPopup: function PVB_cleanPopup(aPopup, aDelay) {
diff --git a/application/palemoon/components/places/content/controller.js b/application/palemoon/components/places/content/controller.js
index 7f5f7f6521..d098271632 100644
--- a/application/palemoon/components/places/content/controller.js
+++ b/application/palemoon/components/places/content/controller.js
@@ -334,20 +334,6 @@ PlacesController.prototype = {
   },
 
   /**
-   * Determines whether or not the root node for the view is selected
-   */
-  rootNodeIsSelected: function PC_rootNodeIsSelected() {
-    var nodes = this._view.selectedNodes;
-    var root = this._view.result.root;
-    for (var i = 0; i < nodes.length; ++i) {
-      if (nodes[i] == root)
-        return true;
-    }
-
-    return false;
-  },
-
-  /**
    * Looks at the data on the clipboard to see if it is paste-able.
    * Paste-able data is:
    *   - in a format that the view can receive
@@ -400,7 +386,7 @@ PlacesController.prototype = {
    * Gathers information about the selected nodes according to the following
    * rules:
    *    "link"              node is a URI
-   *    "bookmark"          node is a bookamrk
+   *    "bookmark"          node is a bookmark
    *    "livemarkChild"     node is a child of a livemark
    *    "tagChild"          node is a child of a tag
    *    "folder"            node is a folder
@@ -414,15 +400,10 @@ PlacesController.prototype = {
    *          node are set on its corresponding object as properties.
    * Notes:
    *   1) This can be slow, so don't call it anywhere performance critical!
-   *   2) A single-object array corresponding the root node is returned if
-   *      there's no selection.
    */
   _buildSelectionMetadata: function PC__buildSelectionMetadata() {
     var metadata = [];
-    var root = this._view.result.root;
     var nodes = this._view.selectedNodes;
-    if (nodes.length == 0)
-      nodes.push(root); // See the second note above
 
     for (var i = 0; i < nodes.length; i++) {
       var nodeData = {};
@@ -501,10 +482,23 @@ PlacesController.prototype = {
    */
   _shouldShowMenuItem: function PC__shouldShowMenuItem(aMenuItem, aMetaData) {
     var selectiontype = aMenuItem.getAttribute("selectiontype");
-    if (selectiontype == "multiple" && aMetaData.length == 1)
+    if (!selectiontype) {
+      selectiontype = "single|multiple";
+    }
+    var selectionTypes = selectiontype.split("|");
+    if (selectionTypes.indexOf("any") != -1) {
+      return true;
+    }
+    var count = aMetaData.length;
+    if (count > 1 && selectionTypes.indexOf("multiple") == -1)
       return false;
-    if (selectiontype == "single" && aMetaData.length != 1)
+    if (count == 1 && selectionTypes.indexOf("single") == -1)
       return false;
+    // NB: if there is no selection, we show the item if (and only if)
+    // the selectiontype includes 'none' - the metadata list will be
+    // empty so none of the other criteria will apply anyway.
+    if (count == 0)
+      return selectionTypes.indexOf("none") != -1;
 
     var forceHideAttr = aMenuItem.getAttribute("forcehideselection");
     if (forceHideAttr) {
@@ -551,9 +545,11 @@ PlacesController.prototype = {
    *  1) The "selectiontype" attribute may be set on a menu-item to "single"
    *     if the menu-item should be visible only if there is a single node
    *     selected, or to "multiple" if the menu-item should be visible only if
-   *     multiple nodes are selected. If the attribute is not set or if it is
-   *     set to an invalid value, the menu-item may be visible for both types of
-   *     selection.
+   *     multiple nodes are selected, or to "none" if the menuitems should be
+   *     visible for if there are no selected nodes, or to a |-separated
+   *     combination of these.
+   *     If the attribute is not set or set to an invalid value, the menu-item
+   *     may be visible irrespective of the selection.
    *  2) The "selection" attribute may be set on a menu-item to the various
    *     meta-data rules for which it may be visible. The rules should be
    *     separated with the | character.
@@ -584,7 +580,7 @@ PlacesController.prototype = {
 
     var separator = null;
     var visibleItemsBeforeSep = false;
-    var anyVisible = false;
+    var usableItemCount = 0;
     for (var i = 0; i < aPopup.childNodes.length; ++i) {
       var item = aPopup.childNodes[i];
       if (item.localName != "menuseparator") {
@@ -598,12 +594,13 @@ PlacesController.prototype = {
                                    (!/tree/i.test(this._view.localName) || ip);
         var hideIfPrivate = item.getAttribute("hideifprivatebrowsing") == "true" &&
                             PrivateBrowsingUtils.isWindowPrivate(window);
-        item.hidden = hideIfNoIP || hideIfPrivate || hideParentFolderItem ||
-                      !this._shouldShowMenuItem(item, metadata);
+        var shouldHideItem = hideIfNoIP || hideIfPrivate || hideParentFolderItem ||
+                             !this._shouldShowMenuItem(item, metadata);
+        item.hidden = item.disabled = shouldHideItem;
 
         if (!item.hidden) {
           visibleItemsBeforeSep = true;
-          anyVisible = true;
+          usableItemCount++;
 
           // Show the separator above the menu-item if any
           if (separator) {
@@ -627,21 +624,21 @@ PlacesController.prototype = {
     }
 
     // Set Open Folder/Links In Tabs items enabled state if they're visible
-    if (anyVisible) {
+    if (usableItemCount > 0) {
       var openContainerInTabsItem = document.getElementById("placesContext_openContainer:tabs");
-      if (!openContainerInTabsItem.hidden && this._view.selectedNode &&
-          PlacesUtils.nodeIsContainer(this._view.selectedNode)) {
-        openContainerInTabsItem.disabled =
-          !PlacesUtils.hasChildURIs(this._view.selectedNode);
-      }
-      else {
-        // see selectiontype rule in the overlay
-        var openLinksInTabsItem = document.getElementById("placesContext_openLinks:tabs");
-        openLinksInTabsItem.disabled = openLinksInTabsItem.hidden;
+      if (!openContainerInTabsItem.hidden) {
+        var containerToUse = this._view.selectedNode || this._view.result.root;
+        if (PlacesUtils.nodeIsContainer(containerToUse)) {
+          if (!PlacesUtils.hasChildURIs(containerToUse, true)) {
+            openContainerInTabsItem.disabled = true;
+            // Ensure that we don't display the menu if nothing is enabled:
+            usableItemCount--;
+          }
+        }
       }
     }
 
-    return anyVisible;
+    return usableItemCount > 0;
   },
 
   /**
@@ -707,10 +704,15 @@ PlacesController.prototype = {
    */
   openSelectionInTabs: function PC_openLinksInTabs(aEvent) {
     var node = this._view.selectedNode;
+    var nodes = this._view.selectedNodes;
+    // In the case of no selection, open the root node:
+    if (!node && !nodes.length) {
+      node = this._view.result.root;
+    }
     if (node && PlacesUtils.nodeIsContainer(node))
-      PlacesUIUtils.openContainerNodeInTabs(this._view.selectedNode, aEvent, this._view);
+      PlacesUIUtils.openContainerNodeInTabs(node, aEvent, this._view);
     else
-      PlacesUIUtils.openURINodesInTabs(this._view.selectedNodes, aEvent, this._view);
+      PlacesUIUtils.openURINodesInTabs(nodes, aEvent, this._view);
   },
 
   /**
diff --git a/application/palemoon/components/places/content/placesOverlay.xul b/application/palemoon/components/places/content/placesOverlay.xul
index dd4d50f017..745990a9c2 100644
--- a/application/palemoon/components/places/content/placesOverlay.xul
+++ b/application/palemoon/components/places/content/placesOverlay.xul
@@ -149,20 +149,20 @@
               command="placesCmd_new:bookmark"
               label="&cmd.new_bookmark.label;"
               accesskey="&cmd.new_bookmark.accesskey;"
-              selection="any"
+              selectiontype="any"
               hideifnoinsertionpoint="true"/>
     <menuitem id="placesContext_new:folder"
               command="placesCmd_new:folder"
               label="&cmd.new_folder.label;"
               accesskey="&cmd.context_new_folder.accesskey;"
-              selection="any"
+              selectiontype="any"
               hideifnoinsertionpoint="true"/>
     <menuitem id="placesContext_new:separator"
               command="placesCmd_new:separator"
               label="&cmd.new_separator.label;"
               accesskey="&cmd.new_separator.accesskey;"
               closemenu="single"
-              selection="any"
+              selectiontype="any"
               hideifnoinsertionpoint="true"/>
     <menuseparator id="placesContext_newSeparator"/>
     <menuitem id="placesContext_createBookmark"
@@ -182,14 +182,13 @@
               command="placesCmd_copy"
               label="&copyCmd.label;"
               closemenu="single"
-              accesskey="&copyCmd.accesskey;" 
-              selection="any"/>
+              accesskey="&copyCmd.accesskey;"/>
     <menuitem id="placesContext_paste"
               command="placesCmd_paste"
               label="&pasteCmd.label;"
               closemenu="single"
               accesskey="&pasteCmd.accesskey;"
-              selection="any"
+              selectiontype="any"
               hideifnoinsertionpoint="true"/>
     <menuseparator id="placesContext_editSeparator"/>
     <menuitem id="placesContext_delete"
diff --git a/application/palemoon/components/places/content/sidebarUtils.js b/application/palemoon/components/places/content/sidebarUtils.js
index 8ffb703485..06ed537531 100644
--- a/application/palemoon/components/places/content/sidebarUtils.js
+++ b/application/palemoon/components/places/content/sidebarUtils.js
@@ -40,7 +40,7 @@ var SidebarUtils = {
     var openInTabs = isContainer &&
                      (aEvent.button == 1 ||
                       (aEvent.button == 0 && modifKey)) &&
-                     PlacesUtils.hasChildURIs(tbo.view.nodeForTreeIndex(cell.row));
+                     PlacesUtils.hasChildURIs(tbo.view.nodeForTreeIndex(cell.row), true);
 
     if (aEvent.button == 0 && isContainer && !openInTabs) {
       tbo.view.toggleOpenState(cell.row);
diff --git a/toolkit/components/places/PlacesUtils.jsm b/toolkit/components/places/PlacesUtils.jsm
index 323fa41a1a..5f6e81f18a 100644
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -1341,7 +1341,7 @@ this.PlacesUtils = {
    *        The container node to search through.
    * @returns true if the node contains uri nodes, false otherwise.
    */
-  hasChildURIs: function PU_hasChildURIs(aNode) {
+  hasChildURIs: function PU_hasChildURIs(aNode, aMultiple=false) {
     if (!this.nodeIsContainer(aNode))
       return false;
 
@@ -1357,11 +1357,14 @@ this.PlacesUtils = {
       root.containerOpen = true;
     }
 
+    let foundFirst = !aMultiple;
     let found = false;
     for (let i = 0; i < root.childCount && !found; i++) {
       let child = root.getChild(i);
-      if (this.nodeIsURI(child))
-        found = true;
+      if (this.nodeIsURI(child)) {
+        found = foundFirst;
+        foundFirst = true;
+      }
     }
 
     if (!wasOpen) {
diff --git a/toolkit/components/places/tests/unit/test_utils_getURLsForContainerNode.js b/toolkit/components/places/tests/unit/test_utils_getURLsForContainerNode.js
index ecebce94a6..3e2f88c21d 100644
--- a/toolkit/components/places/tests/unit/test_utils_getURLsForContainerNode.js
+++ b/toolkit/components/places/tests/unit/test_utils_getURLsForContainerNode.js
@@ -165,6 +165,7 @@ function check_uri_nodes(aQuery, aOptions, aExpectedURINodes) {
   root.containerOpen = true;
   var node = root.getChild(0);
   do_check_eq(PU.hasChildURIs(node), aExpectedURINodes > 0);
+  do_check_eq(PU.hasChildURIs(node, true), aExpectedURINodes > 1);
   do_check_eq(PU.getURLsForContainerNode(node).length, aExpectedURINodes);
   root.containerOpen = false;
 }