Bug 772808, Menuitems should inherit hidden attribute from commands, r=neil,smichaud

layout/xul/base/src/nsXULPopupManager.cpp

@@ -1623,7 +1623,7 @@ nsXULPopupManager::UpdateMenuItems(nsIContent* aPopup)
           else
             grandChild->UnsetAttr(kNameSpaceID_None, nsGkAtoms::disabled, true);
 
-          // The menu's label, accesskey and checked states need to be updated
+          // The menu's label, accesskey checked and hidden states need to be updated
           // to match the command. Note that unlike the disabled state if the
           // command has *no* value, we assume the menu is supplying its own.
           if (commandContent->GetAttr(kNameSpaceID_None, nsGkAtoms::label, commandValue))
@@ -1634,6 +1634,9 @@ nsXULPopupManager::UpdateMenuItems(nsIContent* aPopup)
 
           if (commandContent->GetAttr(kNameSpaceID_None, nsGkAtoms::checked, commandValue))
             grandChild->SetAttr(kNameSpaceID_None, nsGkAtoms::checked, commandValue, true);
+
+          if (commandContent->GetAttr(kNameSpaceID_None, nsGkAtoms::hidden, commandValue))
+            grandChild->SetAttr(kNameSpaceID_None, nsGkAtoms::hidden, commandValue, true);
         }
       }
     }

toolkit/content/tests/chrome/Makefile.in

@@ -156,6 +156,7 @@ MOCHITEST_CHROME_FILES = 	findbar_window.xul \
 		test_mousescroll.xul \
 		test_mousecapture.xul \
 		test_arrowpanel.xul \
+		test_menuitem_commands.xul \
 		$(NULL)
 
 # test_panel_focus.xul won't work if the Full Keyboard Access preference is set to

toolkit/content/tests/chrome/test_menuitem_commands.xul

@@ -0,0 +1,104 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
+<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?>
+
+<window title="Menuitem Commands Test"
+        onload="runOrOpen()"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+  <script type="application/javascript"
+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"/>
+
+<script>
+<![CDATA[
+SimpleTest.waitForExplicitFinish();
+
+function checkAttributes(elem, label, accesskey, disabled, hidden, isAfter)
+{
+  var is = window.opener.wrappedJSObject.SimpleTest.is;
+
+  is(elem.getAttribute("label"), label, elem.id + " label " + (isAfter ? "after" : "before") + " open");
+  is(elem.getAttribute("accesskey"), accesskey, elem.id + " accesskey " + (isAfter ? "after" : "before") + " open");
+  is(elem.getAttribute("disabled"), disabled, elem.id + " disabled " + (isAfter ? "after" : "before") + " open");
+  is(elem.getAttribute("hidden"), hidden, elem.id + " hidden " + (isAfter ? "after" : "before") + " open");
+}
+
+function runOrOpen()
+{
+  if (window.opener) {
+    SimpleTest.waitForFocus(runTest);
+  }
+  else {
+    window.open("test_menuitem_commands.xul", "", "chrome");
+  }
+}
+
+function runTest()
+{
+  runTestSet("");
+  runTestSet("bar");
+  window.close();
+  window.opener.wrappedJSObject.SimpleTest.finish();
+}
+
+function runTestSet(suffix)
+{
+  var isMac = (navigator.platform.indexOf("Mac") >= 0);
+
+  var one = $("one" + suffix);
+  var two = $("two" + suffix);
+  var three = $("three" + suffix);
+  var four = $("four" + suffix);
+
+  checkAttributes(one, "One", "", "", "true", false);
+  checkAttributes(two, "", "", "false", "", false);
+  checkAttributes(three, "Three", "T", "true", "", false);
+  checkAttributes(four, "Four", "F", "", "", false);
+
+  if (isMac && suffix) {
+    var utils = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
+                       getInterface(Components.interfaces.nsIDOMWindowUtils);
+    utils.forceUpdateNativeMenuAt("0");
+  }
+  else {
+    $("menu" + suffix).open = true;
+  }
+
+  checkAttributes(one, "One", "", "", "false", true);
+  checkAttributes(two, "Cat", "C", "", "", true);
+  checkAttributes(three, "Dog", "D", "false", "true", true);
+  checkAttributes(four, "Four", "F", "true", "", true);
+
+  $("menu" + suffix).open = false;
+}
+]]>
+</script>
+
+<command id="cmd_one" hidden="false"/>
+<command id="cmd_two" label="Cat" accesskey="C"/>
+<command id="cmd_three" label="Dog" accesskey="D" disabled="false" hidden="true"/>
+<command id="cmd_four" disabled="true"/>
+
+<button id="menu" type="menu">
+  <menupopup>
+    <menuitem id="one" label="One" hidden="true" command="cmd_one"/>
+    <menuitem id="two" disabled="false" command="cmd_two"/>
+    <menuitem id="three" label="Three" accesskey="T"  disabled="true" command="cmd_three"/>
+    <menuitem id="four" label="Four" accesskey="F" command="cmd_four"/>
+  </menupopup>
+</button>
+
+<menubar>
+  <menu id="menubar" label="Sample">
+    <menupopup>
+      <menuitem id="onebar" label="One" hidden="true" command="cmd_one"/>
+      <menuitem id="twobar" disabled="false" command="cmd_two"/>
+      <menuitem id="threebar" label="Three" accesskey="T"  disabled="true" command="cmd_three"/>
+      <menuitem id="fourbar" label="Four" accesskey="F" command="cmd_four"/>
+    </menupopup>
+  </menu>
+</menubar>
+
+<body xmlns="http://www.w3.org/1999/xhtml"><p id="display"/></body>
+
+</window>

widget/cocoa/nsMenuX.mm

@@ -35,6 +35,7 @@
 #include "nsIDOMElement.h"
 #include "nsBindingManager.h"
 #include "nsIServiceManager.h"
+#include "nsXULPopupManager.h"
 
 #include "jsapi.h"
 #include "nsIScriptGlobalObject.h"
@@ -573,7 +574,7 @@ bool nsMenuX::OnOpen()
     return false;
 
   // If the open is going to succeed we need to walk our menu items, checking to
-  // see if any of them have a command attribute. If so, several apptributes
+  // see if any of them have a command attribute. If so, several attributes
   // must potentially be updated.
 
   // Get new popup content first since it might have changed as a result of the
@@ -582,56 +583,9 @@ bool nsMenuX::OnOpen()
   if (!popupContent)
     return true;
 
-  nsCOMPtr<nsIDOMDocument> domDoc(do_QueryInterface(popupContent->GetDocument()));
-  if (!domDoc)
-    return true;
-
-  uint32_t count = popupContent->GetChildCount();
-  for (uint32_t i = 0; i < count; i++) {
-    nsIContent *grandChild = popupContent->GetChildAt(i);
-    if (grandChild->Tag() == nsGkAtoms::menuitem) {
-      // See if we have a command attribute.
-      nsAutoString command;
-      grandChild->GetAttr(kNameSpaceID_None, nsGkAtoms::command, command);
-      if (!command.IsEmpty()) {
-        // We do! Look it up in our document
-        nsCOMPtr<nsIDOMElement> commandElt;
-        domDoc->GetElementById(command, getter_AddRefs(commandElt));
-        nsCOMPtr<nsIContent> commandContent(do_QueryInterface(commandElt));
-        
-        if (commandContent) {
-          nsAutoString commandDisabled, menuDisabled;
-          commandContent->GetAttr(kNameSpaceID_None, nsGkAtoms::disabled, commandDisabled);
-          grandChild->GetAttr(kNameSpaceID_None, nsGkAtoms::disabled, menuDisabled);
-          if (!commandDisabled.Equals(menuDisabled)) {
-            // The menu's disabled state needs to be updated to match the command.
-            if (commandDisabled.IsEmpty()) 
-              grandChild->UnsetAttr(kNameSpaceID_None, nsGkAtoms::disabled, true);
-            else
-              grandChild->SetAttr(kNameSpaceID_None, nsGkAtoms::disabled, commandDisabled, true);
-          }
-          
-          // The menu's value and checked states need to be updated to match the command.
-          // Note that (unlike the disabled state) if the command has *no* value for either, we
-          // assume the menu is supplying its own.
-          nsAutoString commandChecked, menuChecked;
-          commandContent->GetAttr(kNameSpaceID_None, nsGkAtoms::checked, commandChecked);
-          grandChild->GetAttr(kNameSpaceID_None, nsGkAtoms::checked, menuChecked);
-          if (!commandChecked.Equals(menuChecked)) {
-            if (!commandChecked.IsEmpty()) 
-              grandChild->SetAttr(kNameSpaceID_None, nsGkAtoms::checked, commandChecked, true);
-          }
-          
-          nsAutoString commandValue, menuValue;
-          commandContent->GetAttr(kNameSpaceID_None, nsGkAtoms::label, commandValue);
-          grandChild->GetAttr(kNameSpaceID_None, nsGkAtoms::label, menuValue);
-          if (!commandValue.Equals(menuValue)) {
-            if (!commandValue.IsEmpty()) 
-              grandChild->SetAttr(kNameSpaceID_None, nsGkAtoms::label, commandValue, true);
-          }
-        }
-      }
-    }
+  nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
+  if (pm) {
+    pm->UpdateMenuItems(popupContent);
   }
 
   return true;