From 7d2dbaba8bd0fc0c97482f54b9e1ec3230cf607a Mon Sep 17 00:00:00 2001 From: Neil Deakin Date: Sun, 13 Feb 2011 07:28:00 -0500 Subject: [PATCH] Bug 630830 - "key" attribute changes to menuitems are not handled; r=joshmoz --- layout/reftests/reftest.list | 3 + layout/reftests/xul/menuitem-key-ref.xul | 19 ++++++ layout/reftests/xul/menuitem-key.xul | 46 +++++++++++++++ layout/reftests/xul/reftest.list | 1 + layout/xul/base/src/nsMenuFrame.cpp | 29 +++++++--- layout/xul/base/src/nsMenuFrame.h | 3 +- toolkit/content/tests/chrome/window_keys.xul | 52 +++++++++++++++++ widget/src/cocoa/nsMenuItemX.h | 2 +- widget/src/cocoa/nsMenuItemX.mm | 61 +++++++++++--------- 9 files changed, 177 insertions(+), 39 deletions(-) create mode 100644 layout/reftests/xul/menuitem-key-ref.xul create mode 100644 layout/reftests/xul/menuitem-key.xul create mode 100644 layout/reftests/xul/reftest.list diff --git a/layout/reftests/reftest.list b/layout/reftests/reftest.list index 3910a86f53b2..63d79c9877a1 100644 --- a/layout/reftests/reftest.list +++ b/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 diff --git a/layout/reftests/xul/menuitem-key-ref.xul b/layout/reftests/xul/menuitem-key-ref.xul new file mode 100644 index 000000000000..2f1573f61933 --- /dev/null +++ b/layout/reftests/xul/menuitem-key-ref.xul @@ -0,0 +1,19 @@ + + + + + + + + + + + + + + + + + + + diff --git a/layout/reftests/xul/menuitem-key.xul b/layout/reftests/xul/menuitem-key.xul new file mode 100644 index 000000000000..504071122470 --- /dev/null +++ b/layout/reftests/xul/menuitem-key.xul @@ -0,0 +1,46 @@ + + + + + + + + + + + + + + + + + + + + + + diff --git a/layout/reftests/xul/reftest.list b/layout/reftests/xul/reftest.list new file mode 100644 index 000000000000..c0512a0c13aa --- /dev/null +++ b/layout/reftests/xul/reftest.list @@ -0,0 +1 @@ +== menuitem-key.xul menuitem-key-ref.xul diff --git a/layout/xul/base/src/nsMenuFrame.cpp b/layout/xul/base/src/nsMenuFrame.cpp index 8fe246b1d87a..828015ac3778 100644 --- a/layout/xul/base/src/nsMenuFrame.cpp +++ b/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 diff --git a/layout/xul/base/src/nsMenuFrame.h b/layout/xul/base/src/nsMenuFrame.h index 141f5d21037e..950fbc872e4d 100644 --- a/layout/xul/base/src/nsMenuFrame.h +++ b/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. diff --git a/toolkit/content/tests/chrome/window_keys.xul b/toolkit/content/tests/chrome/window_keys.xul index 32cd1bb3eeaf..4ef27d3bd994 100644 --- a/toolkit/content/tests/chrome/window_keys.xul +++ b/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); + + +

diff --git a/widget/src/cocoa/nsMenuItemX.h b/widget/src/cocoa/nsMenuItemX.h index 0ca2780e5200..42b8fb233db7 100644 --- a/widget/src/cocoa/nsMenuItemX.h +++ b/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. diff --git a/widget/src/cocoa/nsMenuItemX.mm b/widget/src/cocoa/nsMenuItemX.mm index e766c0eb5c21..9bb92179c2ac 100644 --- a/widget/src/cocoa/nsMenuItemX.mm +++ b/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(); }