Fixes a visibility logic bug. Also extends support for the collapsed attribute to all menu node types (menu, menu item, and separator). Apparently there was a previous assumption that collapsed would only be used on the menu type, but that is nonsense. In XUL collapsed=true is the same as visibility:collapsed, and according to CSS2 visibility:collapsed outside the context of a table is the same as hidden:true. Cleans up the code for hidden and collapsed attribute checks. b=373730 r=mano sr=pav

widget/src/cocoa/nsMenuBarX.h

@@ -58,6 +58,8 @@ class nsIDOMNode;
 
 extern "C" MenuRef _NSGetCarbonMenu(NSMenu* aMenu);
 
+PRBool NodeIsHiddenOrCollapsed(nsIContent* inContent);
+
 namespace MenuHelpersX
 {
   nsEventStatus DispatchCommandTo(nsIWeakReference* aDocShellWeakRef,

widget/src/cocoa/nsMenuBarX.mm

@@ -87,6 +87,15 @@ enum {
 };
 
 
+PRBool NodeIsHiddenOrCollapsed(nsIContent* inContent)
+{
+  return (inContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::hidden,
+                                 nsWidgetAtoms::_true, eCaseMatters) ||
+          inContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::collapsed,
+                                 nsWidgetAtoms::_true, eCaseMatters));
+}
+
+
 nsMenuBarX::nsMenuBarX()
 : mParent(nsnull), mIsMenuBarAdded(PR_FALSE), mCurrentCommandID(eCommand_ID_Last), mDocument(nsnull)
 {
@@ -502,9 +511,8 @@ NS_IMETHODIMP nsMenuBarX::AddMenu(nsIMenu * aMenu)
   
   nsCOMPtr<nsIContent> menu;
   aMenu->GetMenuContent(getter_AddRefs(menu));
-  if (!menu->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::hidden,
+  if (menu->GetChildCount() > 0 &&
-                         nsWidgetAtoms::_true, eCaseMatters) &&
+      !NodeIsHiddenOrCollapsed(menu)) {
-      menu->GetChildCount() > 0) {
     NSMenuItem* newMenuItem = [[[NSMenuItem alloc] initWithTitle:@"SomeMenuItem" action:NULL keyEquivalent:@""] autorelease];
     [mRootMenu addItem:newMenuItem];
     [newMenuItem setSubmenu:menuRef];

widget/src/cocoa/nsMenuX.h

@@ -135,9 +135,9 @@ protected:
     PRBool OnDestroyed();
     PRBool OnCreated();
 
-    void LoadMenuItem(nsIMenu* pParentMenu, nsIContent* menuitemContent);  
+    void LoadMenuItem(nsIMenu* pParentMenu, nsIContent* inMenuItemContent);  
-    void LoadSubMenu(nsIMenu * pParentMenu, nsIContent* menuitemContent);
+    void LoadSubMenu(nsIMenu * pParentMenu, nsIContent* inMenuContent);
-    void LoadSeparator(nsIContent* menuitemContent);
+    void LoadSeparator(nsIContent* inSeparatorContent);
 
     NSMenu* CreateMenuWithGeckoString(nsString& menuTitle);
 

widget/src/cocoa/nsMenuX.mm

@@ -153,10 +153,7 @@ nsMenuX::Create(nsISupports * aParent, const nsAString &aLabel, const nsAString
   // SetLabel will create the native menu if it has not been created yet
   SetLabel(aLabel);
 
-  if (mMenuContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::hidden,
+  if (NodeIsHiddenOrCollapsed(mMenuContent))
-                                nsWidgetAtoms::_true, eCaseMatters) ||
-      mMenuContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::collapsed,
-                                nsWidgetAtoms::_true, eCaseMatters))
     mVisible = PR_FALSE;
 
   if (menubar && mMenuContent->GetChildCount() == 0)
@@ -614,9 +611,7 @@ void nsMenuX::LoadMenuItem(nsIMenu* inParentMenu, nsIContent* inMenuItemContent)
   if (!inMenuItemContent)
     return;
 
-  // if menu should be hidden, bail
+  if (NodeIsHiddenOrCollapsed(inMenuItemContent))
-  if (inMenuItemContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::hidden,
-                                     nsWidgetAtoms::_true, eCaseMatters))
     return;
 
   // create nsMenuItem
@@ -687,15 +682,13 @@ void nsMenuX::LoadMenuItem(nsIMenu* inParentMenu, nsIContent* inMenuItemContent)
 }
 
 
-void nsMenuX::LoadSubMenu(nsIMenu * pParentMenu, nsIContent* inMenuItemContent)
+void nsMenuX::LoadSubMenu(nsIMenu * pParentMenu, nsIContent* inMenuContent)
 {
-  // if menu should be hidden, bail
+  if (NodeIsHiddenOrCollapsed(inMenuContent))
-  if (inMenuItemContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::hidden,
-                                     nsWidgetAtoms::_true, eCaseMatters))
     return;
   
   nsAutoString menuName; 
-  inMenuItemContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::label, menuName);
+  inMenuContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::label, menuName);
   //printf("Creating Menu [%s] \n", NS_LossyConvertUTF16toASCII(menuName).get());
 
   // Create nsMenu
@@ -706,11 +699,11 @@ void nsMenuX::LoadSubMenu(nsIMenu * pParentMenu, nsIContent* inMenuItemContent)
     if (!docShell)
         return;
     nsCOMPtr<nsISupports> supports(do_QueryInterface(pParentMenu));
-    pnsMenu->Create(supports, menuName, EmptyString(), mManager, docShell, inMenuItemContent);
+    pnsMenu->Create(supports, menuName, EmptyString(), mManager, docShell, inMenuContent);
 
     // set if it's enabled or disabled
-    if (inMenuItemContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::disabled,
+    if (inMenuContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::disabled,
-                                       nsWidgetAtoms::_true, eCaseMatters))
+                                   nsWidgetAtoms::_true, eCaseMatters))
       pnsMenu->SetEnabled(PR_FALSE);
     else
       pnsMenu->SetEnabled(PR_TRUE);
@@ -722,11 +715,9 @@ void nsMenuX::LoadSubMenu(nsIMenu * pParentMenu, nsIContent* inMenuItemContent)
 }
 
 
-void nsMenuX::LoadSeparator(nsIContent* inMenuItemContent) 
+void nsMenuX::LoadSeparator(nsIContent* inSeparatorContent) 
 {
-  // if item should be hidden, bail
+  if (NodeIsHiddenOrCollapsed(inSeparatorContent))
-  if (inMenuItemContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::hidden,
-                                     nsWidgetAtoms::_true, eCaseMatters))
     return;
 
   AddSeparator();
@@ -968,30 +959,22 @@ nsresult nsMenuX::CountVisibleBefore(PRUint32* outVisibleBefore)
   for (PRUint32 i = 0; i < numMenus; i++) {
     nsCOMPtr<nsIMenu> currMenu;
     menubarParent->GetMenuAt(i, *getter_AddRefs(currMenu));
-    
-    // we found ourselves, break out
     if (currMenu == NS_STATIC_CAST(nsIMenu*, this)) {
+      // we found ourselves, break out
       gotThisMenu = PR_TRUE;
       break;
     }
 
-    // check the current menu to see if it is visible (not hidden, not collapsed). If
-    // it is, count it.
     if (currMenu) {
       nsCOMPtr<nsIContent> menuContent;
       currMenu->GetMenuContent(getter_AddRefs(menuContent));
-      if (menuContent) {
+      if (menuContent &&
-        if (menuContent->GetChildCount() > 0 ||
+          menuContent->GetChildCount() > 0 &&
-            !menuContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::hidden,
+          !NodeIsHiddenOrCollapsed(menuContent)) {
-                                      nsWidgetAtoms::_true, eCaseMatters) &&
+        ++(*outVisibleBefore);
-            !menuContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::collapsed,
-                                      nsWidgetAtoms::_true, eCaseMatters)) {
-          ++(*outVisibleBefore);
-        }
       }
     }
-    
+  }
-  } // for each menu
 
   return gotThisMenu ? NS_OK : NS_ERROR_FAILURE;
 } // CountVisibleBefore
@@ -1061,10 +1044,7 @@ NS_IMETHODIMP nsMenuX::AttributeChanged(nsIDocument *aDocument, PRInt32 aNameSpa
   else if (aAttribute == nsWidgetAtoms::hidden || aAttribute == nsWidgetAtoms::collapsed) {
     SetRebuild(PR_TRUE);
 
-    if (mMenuContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::hidden,
+    if (NodeIsHiddenOrCollapsed(mMenuContent)) {
-                                  nsWidgetAtoms::_true, eCaseMatters) ||
-        mMenuContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::collapsed,
-                                  nsWidgetAtoms::_true, eCaseMatters)) {
       if (mVisible) {
         if (menubarParent) {
           PRUint32 indexToRemove = 0;