Bug 630830 - "key" attribute changes to menuitems are not handled; r=joshmoz

layout/reftests/reftest.list

@@ -273,6 +273,9 @@ include ../../content/test/reftest/xml-stylesheet/reftest.list
 # xul-document-load/
 include xul-document-load/reftest.list
 
+# xul/
+include xul/reftest.list
+
 # xul grid
 include ../xul/base/src/grid/reftests/reftest.list
 

layout/reftests/xul/menuitem-key-ref.xul

@@ -0,0 +1,19 @@
+<?xml version="1.0"?>
+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+<menuitem id="m1" label="Menu1" key="keytwo"/>
+<menuitem id="m2" label="Menu2"/>
+<menuitem id="m3" label="Menu3" key="keythree"/>
+<menuitem id="m4" label="Menu4" acceltext="Text"/>
+<menuitem id="m5" label="Menu5" acceltext="Text"/>
+<menuitem id="m6" label="Menu6" acceltext="Text"/>
+<menuitem id="m7" label="Menu7" key="keythree"/>
+<menuitem id="m8" label="Menu8"/>
+<menuitem id="m9" label="Menu9"/>
+
+<keyset>
+  <key id="keytwo" key="Y" modifiers="control"/>
+  <key id="keythree" key="X" modifiers="accel"/>
+</keyset>
+
+</window>

layout/reftests/xul/menuitem-key.xul

@@ -0,0 +1,46 @@
+<?xml version="1.0"?>
+<window class="reftest-wait" onload="changeKeys()"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+<menuitem id="m1" label="Menu1" key="keyone"/>
+<menuitem id="m2" label="Menu2" key="keytwo"/>
+<menuitem id="m3" label="Menu3"/>
+<menuitem id="m4" label="Menu4"/>
+<menuitem id="m5" label="Menu5"/>
+<menuitem id="m6" label="Menu6" key="keythree" acceltext="Text"/>
+<menuitem id="m7" label="Menu7" key="keythree" acceltext="Text"/>
+<menuitem id="m8" label="Menu8" key="keythree" acceltext="Text"/>
+<menuitem id="m9" label="Menu9" key="keytwo"/>
+
+<script>
+function changeKeys()
+{
+  document.getElementById("m1").setAttribute("key", "keytwo");
+  document.getElementById("m2").removeAttribute("key");
+  document.getElementById("m3").setAttribute("key", "keythree");
+
+  document.getElementById("m4").setAttribute("key", "keyone");
+  document.getElementById("m4").setAttribute("acceltext", "Text");
+
+  document.getElementById("m5").setAttribute("acceltext", "Text");
+  document.getElementById("m5").setAttribute("key", "keytwo");
+
+  document.getElementById("m6").removeAttribute("key");
+  document.getElementById("m7").removeAttribute("acceltext");
+
+  document.getElementById("m8").removeAttribute("key");
+  document.getElementById("m8").removeAttribute("acceltext");
+
+  document.getElementById("m9").removeAttribute("key");
+
+  document.documentElement.className='';
+}
+</script>
+
+<keyset>
+  <key id="keyone" key="Z" modifiers="control"/>
+  <key id="keytwo" key="Y" modifiers="control"/>
+  <key id="keythree" key="X" modifiers="accel"/>
+</keyset>
+
+</window>

layout/reftests/xul/reftest.list

@@ -0,0 +1 @@
+== menuitem-key.xul menuitem-key-ref.xul

layout/xul/base/src/nsMenuFrame.cpp

@@ -170,10 +170,11 @@ public:
     } else if (mAttr == nsGkAtoms::acceltext) {
       // someone reset the accelText attribute,
       // so clear the bit that says *we* set it
-      frame->AddStateBits(NS_STATE_ACCELTEXT_IS_DERIVED);
-      frame->BuildAcceleratorText();
-    } else if (mAttr == nsGkAtoms::key) {
-      frame->BuildAcceleratorText();
+      frame->RemoveStateBits(NS_STATE_ACCELTEXT_IS_DERIVED);
+      frame->BuildAcceleratorText(PR_TRUE);
+    }
+    else if (mAttr == nsGkAtoms::key) {
+      frame->BuildAcceleratorText(PR_TRUE);
     } else if (mAttr == nsGkAtoms::type || mAttr == nsGkAtoms::name) {
       frame->UpdateMenuType(frame->PresContext());
     }
@@ -224,6 +225,7 @@ nsMenuFrame::nsMenuFrame(nsIPresShell* aShell, nsStyleContext* aContext):
   nsBoxFrame(aShell, aContext),
     mIsMenu(PR_FALSE),
     mChecked(PR_FALSE),
+    mIgnoreAccelTextChange(PR_FALSE),
     mType(eMenuType_Normal),
     mMenuParent(nsnull),
     mPopupFrame(nsnull),
@@ -334,7 +336,7 @@ nsMenuFrame::Init(nsIContent*      aContent,
     gModifierSeparator = new nsString(modifierSeparator);    
   }
 
-  BuildAcceleratorText();
+  BuildAcceleratorText(PR_FALSE);
   nsIReflowCallback* cb = new nsASyncMenuInitialization(this);
   NS_ENSURE_TRUE(cb, NS_ERROR_OUT_OF_MEMORY);
   PresContext()->PresShell()->PostReflowCallback(cb);
@@ -699,6 +701,11 @@ nsMenuFrame::AttributeChanged(PRInt32 aNameSpaceID,
                               nsIAtom* aAttribute,
                               PRInt32 aModType)
 {
+  if (aAttribute == nsGkAtoms::acceltext && mIgnoreAccelTextChange) {
+    // Reset the flag so that only one change is ignored.
+    mIgnoreAccelTextChange = PR_FALSE;
+    return NS_OK;
+  }
 
   if (aAttribute == nsGkAtoms::checked ||
       aAttribute == nsGkAtoms::acceltext ||
@@ -1015,7 +1022,7 @@ nsMenuFrame::UpdateMenuSpecialState(nsPresContext* aPresContext)
 }
 
 void 
-nsMenuFrame::BuildAcceleratorText()
+nsMenuFrame::BuildAcceleratorText(PRBool aNotify)
 {
   nsAutoString accelText;
 
@@ -1031,7 +1038,7 @@ nsMenuFrame::BuildAcceleratorText()
 
   // If anything below fails, just leave the accelerator text blank.
   nsWeakFrame weakFrame(this);
-  mContent->UnsetAttr(kNameSpaceID_None, nsGkAtoms::acceltext, PR_FALSE);
+  mContent->UnsetAttr(kNameSpaceID_None, nsGkAtoms::acceltext, aNotify);
   ENSURE_TRUE(weakFrame.IsAlive());
 
   // See if we have a key node and use that instead.
@@ -1156,8 +1163,12 @@ nsMenuFrame::BuildAcceleratorText()
   nsMemory::Free(str);
 
   accelText += accelString;
-  
-  mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::acceltext, accelText, PR_FALSE);
+
+  mIgnoreAccelTextChange = PR_TRUE;
+  mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::acceltext, accelText, aNotify);
+  ENSURE_TRUE(weakFrame.IsAlive());
+
+  mIgnoreAccelTextChange = PR_FALSE;
 }
 
 void

layout/xul/base/src/nsMenuFrame.h

@@ -238,7 +238,7 @@ protected:
   void UpdateMenuSpecialState(nsPresContext* aPresContext);
 
   // Examines the key node and builds the accelerator.
-  void BuildAcceleratorText();
+  void BuildAcceleratorText(PRBool aNotify);
 
   // Called to execute our command handler. This method can destroy the frame.
   void Execute(nsGUIEvent *aEvent);
@@ -265,6 +265,7 @@ protected:
 
   PRPackedBool mIsMenu; // Whether or not we can even have children or not.
   PRPackedBool mChecked;              // are we checked?
+  PRPackedBool mIgnoreAccelTextChange; // temporarily set while determining the accelerator key
   nsMenuType mType;
 
   nsMenuParent* mMenuParent; // Our parent menu.

toolkit/content/tests/chrome/window_keys.xul

@@ -62,6 +62,50 @@ function runTest()
   document.documentElement.appendChild(keyset);
   iterateKeys(true, "appended");
 
+  var accelText = function(menuitem) menuitem.getAttribute("acceltext").toLowerCase();
+
+  $("menubutton").open = true;
+
+  // now check if a menu updates its accelerator text when a key attribute is changed
+  var menuitem1 = $("menuitem1");
+  ok(accelText(menuitem1).indexOf("d") >= 0, "menuitem1 accelText before");
+  if (navigator.platform.indexOf("Win") != -1) {
+    ok(accelText(menuitem1).indexOf("alt") >= 0, "menuitem1 accelText modifier before");
+  }
+
+  menuitem1.setAttribute("key", "k-s-c");
+  ok(accelText(menuitem1).indexOf("s") >= 0, "menuitem1 accelText after");
+  if (navigator.platform.indexOf("Win") != -1) {
+    ok(accelText(menuitem1).indexOf("ctrl") >= 0, "menuitem1 accelText modifier after");
+  }
+
+  menuitem1.setAttribute("acceltext", "custom");
+  is(accelText(menuitem1), "custom", "menuitem1 accelText set custom");
+  menuitem1.removeAttribute("acceltext");
+  ok(accelText(menuitem1).indexOf("s") >= 0, "menuitem1 accelText remove");
+  if (navigator.platform.indexOf("Win") != -1) {
+    ok(accelText(menuitem1).indexOf("ctrl") >= 0, "menuitem1 accelText modifier remove");
+  }
+
+  var menuitem2 = $("menuitem2");
+  is(accelText(menuitem2), "", "menuitem2 accelText before");
+  menuitem2.setAttribute("key", "k-s-c");
+  ok(accelText(menuitem2).indexOf("s") >= 0, "menuitem2 accelText before");
+  if (navigator.platform.indexOf("Win") != -1) {
+    ok(accelText(menuitem2).indexOf("ctrl") >= 0, "menuitem2 accelText modifier before");
+  }
+
+  menuitem2.setAttribute("key", "k-h-l");
+  ok(accelText(menuitem2).indexOf("h") >= 0, "menuitem2 accelText after");
+  if (navigator.platform.indexOf("Win") != -1) {
+    ok(accelText(menuitem2).indexOf("ctrl") >= 0, "menuitem2 accelText modifier after");
+  }
+
+  menuitem2.removeAttribute("key");
+  is(accelText(menuitem2), "", "menuitem2 accelText after remove");
+
+  $("menubutton").open = false;
+
   window.close();
   window.opener.wrappedJSObject.SimpleTest.finish();
 }
@@ -116,8 +160,16 @@ SimpleTest.waitForFocus(runTest);
 
 <keyset id="keyset2">
   <key id="k-d-a" key="d" modifiers="alt" oncommand="checkKey(event)"/>
+  <key id="k-s-c" key="s" modifiers="control" oncommand="checkKey(event)"/>
 </keyset>
 
+<button id="menubutton" label="Menu" type="menu">
+  <menupopup>
+    <menuitem id="menuitem1" label="Item 1" key="k-d-a"/>
+    <menuitem id="menuitem2" label="Item 2"/>
+  </menupopup>
+</button>
+
 <body xmlns="http://www.w3.org/1999/xhtml">
 <p id="display">
 </p>

widget/src/cocoa/nsMenuItemX.h

@@ -92,7 +92,7 @@ public:
 
 protected:
   void UncheckRadioSiblings(nsIContent* inCheckedElement);
-  void SetKeyEquiv(PRUint8 aModifiers, const nsString &aText);
+  void SetKeyEquiv();
 
   EMenuItemType             mType;
   // nsMenuItemX objects should always have a valid native menu item.

widget/src/cocoa/nsMenuItemX.mm

@@ -143,25 +143,7 @@ nsresult nsMenuItemX::Create(nsMenuX* aParent, const nsString& aLabel, EMenuItem
 
     SetChecked(mContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::checked,
                                      nsWidgetAtoms::_true, eCaseMatters));
-
-    // Set key shortcut and modifiers
-    if (doc) {
-      nsAutoString keyValue;
-      mContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::key, keyValue);
-      if (!keyValue.IsEmpty()) {
-        nsIContent *keyContent = doc->GetElementById(keyValue);
-        if (keyContent) {
-          nsAutoString keyChar(NS_LITERAL_STRING(" "));
-          keyContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::key, keyChar);
-
-          nsAutoString modifiersStr;
-          keyContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::modifiers, modifiersStr);
-          PRUint8 modifiers = nsMenuUtilsX::GeckoModifiersForNodeAttribute(modifiersStr);
-
-          SetKeyEquiv(modifiers, keyChar);
-        }
-      }
-    }
+    SetKeyEquiv();
   }
 
   mIcon = new nsMenuItemIconX(this, mContent, mNativeMenuItem);
@@ -287,19 +269,39 @@ void nsMenuItemX::UncheckRadioSiblings(nsIContent* inCheckedContent)
   }
 }
 
-void nsMenuItemX::SetKeyEquiv(PRUint8 aModifiers, const nsString &aText)
+void nsMenuItemX::SetKeyEquiv()
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
-  unsigned int macModifiers = nsMenuUtilsX::MacModifiersForGeckoModifiers(aModifiers);
-  [mNativeMenuItem setKeyEquivalentModifierMask:macModifiers];
+  // Set key shortcut and modifiers
+  nsAutoString keyValue;
+  mContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::key, keyValue);
+  if (!keyValue.IsEmpty() && mContent->GetCurrentDoc()) {
+    nsIContent *keyContent = mContent->GetCurrentDoc()->GetElementById(keyValue);
+    if (keyContent) {
+      nsAutoString keyChar(NS_LITERAL_STRING(" "));
+      keyContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::key, keyChar);
 
-  NSString *keyEquivalent = [[NSString stringWithCharacters:(unichar*)aText.get()
-                                                     length:aText.Length()] lowercaseString];
-  if ([keyEquivalent isEqualToString:@" "])
-    [mNativeMenuItem setKeyEquivalent:@""];
-  else
-    [mNativeMenuItem setKeyEquivalent:keyEquivalent];
+      nsAutoString modifiersStr;
+      keyContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::modifiers, modifiersStr);
+      PRUint8 modifiers = nsMenuUtilsX::GeckoModifiersForNodeAttribute(modifiersStr);
+
+      unsigned int macModifiers = nsMenuUtilsX::MacModifiersForGeckoModifiers(modifiers);
+      [mNativeMenuItem setKeyEquivalentModifierMask:macModifiers];
+
+      NSString *keyEquivalent = [[NSString stringWithCharacters:(unichar*)keyChar.get()
+                                                         length:keyChar.Length()] lowercaseString];
+      if ([keyEquivalent isEqualToString:@" "])
+        [mNativeMenuItem setKeyEquivalent:@""];
+      else
+        [mNativeMenuItem setKeyEquivalent:keyEquivalent];
+
+      return;
+    }
+  }
+
+  // if the key was removed, clear the key
+  [mNativeMenuItem setKeyEquivalent:@""];
 
   NS_OBJC_END_TRY_ABORT_BLOCK;
 }
@@ -332,6 +334,9 @@ nsMenuItemX::ObserveAttributeChanged(nsIDocument *aDocument, nsIContent *aConten
              aAttribute == nsWidgetAtoms::label) {
       mMenuParent->SetRebuild(PR_TRUE);
     }
+    else if (aAttribute == nsWidgetAtoms::key) {
+      SetKeyEquiv();
+    }
     else if (aAttribute == nsWidgetAtoms::image) {
       SetupIcon();
     }