From 926d4fce9cda8c095028e6d9324b7dcc257f75bb Mon Sep 17 00:00:00 2001
From: Dave Townsend <dtownsend@oxymoronical.com>
Date: Wed, 30 Jan 2013 15:37:34 -0800
Subject: [PATCH] Bug 836318: Items in submenus default to visible rather than
 using the PageContext.

---
 doc/module-source/sdk/context-menu.md | 11 +++-----
 lib/sdk/context-menu.js               | 13 +++++-----
 test/test-context-menu.js             | 37 +++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/doc/module-source/sdk/context-menu.md b/doc/module-source/sdk/context-menu.md
index 79a4bc023..650ff66e4 100644
--- a/doc/module-source/sdk/context-menu.md
+++ b/doc/module-source/sdk/context-menu.md
@@ -39,12 +39,6 @@ configurable with the `extensions.addon-sdk.context-menu.overflowThreshold`
 preference) all of the menu items will instead appear in an overflow menu to
 avoid making the context menu too large.
 
-Note that *context menu items are only displayed when the page has finished loading*.
-While the page is still loading, or if you cancel load, context menu items
-added using this module will not appear.
-[Bug 714914](https://bugzilla.mozilla.org/show_bug.cgi?id=714914) tracks
-this problem.
-
 Specifying Contexts
 -------------------
 
@@ -59,8 +53,9 @@ or the node the user clicked to open the menu.
 
 ### The Page Context
 
-First of all, you may not need to specify a context at all.  When an item does
-not specify a context, the page context applies.
+First of all, you may not need to specify a context at all.  When a top-level
+item does not specify a context, the page context applies.  An item that is in a
+submenu is visible unless you specify a context.
 
 The *page context* occurs when the user invokes the context menu on a
 non-interactive portion of the page.  Try right-clicking a blank spot in this
diff --git a/lib/sdk/context-menu.js b/lib/sdk/context-menu.js
index 390da87b3..47adeb3e5 100644
--- a/lib/sdk/context-menu.js
+++ b/lib/sdk/context-menu.js
@@ -335,11 +335,11 @@ function getCurrentWorkerContext(item, popupNode) {
 
 // Tests whether an item should be visible or not based on its contexts and
 // content scripts
-function isItemVisible(item, popupNode) {
+function isItemVisible(item, popupNode, defaultVisibility) {
   if (!item.context.length) {
     let worker = getItemWorkerForWindow(item, popupNode.ownerDocument.defaultView);
     if (!worker || !worker.anyContextListeners())
-      return PageContext().isCurrent(popupNode);
+      return defaultVisibility;
   }
 
   if (!hasMatchingContext(item.context, popupNode))
@@ -714,16 +714,16 @@ let MenuWrapper = Class({
 
   // Recurses through the menu setting the visibility of items. Returns true
   // if any of the items in this menu were visible
-  setVisibility: function setVisibility(menu, popupNode) {
+  setVisibility: function setVisibility(menu, popupNode, defaultVisibility) {
     let anyVisible = false;
 
     for (let item of internal(menu).children) {
-      let visible = isItemVisible(item, popupNode);
+      let visible = isItemVisible(item, popupNode, defaultVisibility);
 
       // Recurse through Menus, if none of the sub-items were visible then the
       // menu is hidden too.
       if (visible && (item instanceof Menu))
-        visible = this.setVisibility(item, popupNode);
+        visible = this.setVisibility(item, popupNode, true);
 
       let xulNode = this.getXULNodeForItem(item);
       xulNode.hidden = !visible;
@@ -967,7 +967,8 @@ let MenuWrapper = Class({
       let separator = this.separator;
       let popup = this.overflowMenu;
   
-      if (this.setVisibility(this.items, event.target.triggerNode)) {
+      let popupNode = event.target.triggerNode;
+      if (this.setVisibility(this.items, popupNode, PageContext().isCurrent(popupNode))) {
         // Some of this instance's items are visible so make sure the separator
         // and if necessary the overflow popup are visible
         separator.hidden = false;
diff --git a/test/test-context-menu.js b/test/test-context-menu.js
index 65f1d3b9a..e59f7d931 100644
--- a/test/test-context-menu.js
+++ b/test/test-context-menu.js
@@ -2031,6 +2031,43 @@ exports.testSubItemContextMatch = function (test) {
   });
 };
 
+
+// Child items should default to visible, not to PageContext
+exports.testSubItemDefaultVisible = function (test) {
+  test = new TestHelper(test);
+  let loader = test.newLoader();
+
+  let items = [
+    loader.cm.Menu({
+      label: "menu 1",
+      context: loader.cm.SelectorContext("img"),
+      items: [
+        loader.cm.Item({
+          label: "subitem 1"
+        }),
+        loader.cm.Item({
+          label: "subitem 2",
+          context: loader.cm.SelectorContext("img")
+        }),
+        loader.cm.Item({
+          label: "subitem 3",
+          context: loader.cm.SelectorContext("a")
+        })
+      ]
+    })
+  ];
+
+  // subitem 3 will be hidden
+  let hiddenItems = [items[0].items[2]];
+
+  test.withTestDoc(function (window, doc) {
+    test.showMenu(doc.getElementById("image"), function (popup) {
+      test.checkMenu(items, hiddenItems, []);
+      test.done();
+    });
+  });
+};
+
 exports.testSubItemClick = function (test) {
   test = new TestHelper(test);
   let loader = test.newLoader();