Bug 584965: Improve key event handling for Cocoa widgets, follow Cocoa event propagation model properly. Fixes bug 582052 and bug 379199. r=smichaud
☠☠ backed out by 2c69cdb5a98c ☠ ☠
authorJosh Aas <joshmoz@gmail.com>
Fri, 13 Aug 2010 02:38:40 -0400
changeset 50386 33f8c2bb77ca11f13bf368a193cfe51b569263cd
parent 50385 1887d4c886876f3a60000f34d9da5e9f6d5d1204
child 50387 452db8c688bad65a1003ff00b2606d28410c5373
child 50448 2c69cdb5a98c492d99f6fc0c3f05cb2796a84d1d
push id1
push userroot
push dateTue, 26 Apr 2011 22:38:44 +0000
treeherdermozilla-beta@bfdb6e623a36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmichaud
bugs584965, 582052, 379199
milestone2.0b4pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 584965: Improve key event handling for Cocoa widgets, follow Cocoa event propagation model properly. Fixes bug 582052 and bug 379199. r=smichaud
widget/src/cocoa/nsChildView.h
widget/src/cocoa/nsChildView.mm
widget/src/cocoa/nsCocoaWindow.mm
widget/src/cocoa/nsMenuBarX.h
widget/src/cocoa/nsMenuBarX.mm
--- a/widget/src/cocoa/nsChildView.h
+++ b/widget/src/cocoa/nsChildView.h
@@ -220,16 +220,18 @@ extern "C" long TSMProcessRawKeyEvent(Ev
 - (void) processPluginKeyEvent:(EventRef)aKeyEvent;
 #endif
 - (void)pluginRequestsComplexTextInputForCurrentEvent;
 
 - (void)update;
 - (void)lockFocus;
 - (void) _surfaceNeedsUpdate:(NSNotification*)notification;
 
+- (BOOL)isPluginView;
+
 // Simple gestures support
 //
 // XXX - The swipeWithEvent, beginGestureWithEvent, magnifyWithEvent,
 // rotateWithEvent, and endGestureWithEvent methods are part of a
 // PRIVATE interface exported by nsResponder and reverse-engineering
 // was necessary to obtain the methods' prototypes. Thus, Apple may
 // change the interface in the future without notice.
 //
--- a/widget/src/cocoa/nsChildView.mm
+++ b/widget/src/cocoa/nsChildView.mm
@@ -174,18 +174,16 @@ PRUint32 nsChildView::sLastInputEventCou
 - (BOOL)isPluginView;
 - (void)setPluginEventModel:(NPEventModel)eventModel;
 - (NPEventModel)pluginEventModel;
 
 - (BOOL)isRectObscuredBySubview:(NSRect)inRect;
 
 - (void)processPendingRedraws;
 
-- (PRBool)processKeyDownEvent:(NSEvent*)theEvent keyEquiv:(BOOL)isKeyEquiv;
-
 - (void)maybeInitContextMenuTracking;
 
 + (NSEvent*)makeNewCocoaEventWithType:(NSEventType)type fromEvent:(NSEvent*)theEvent;
 
 - (float)beginMaybeResetUnifiedToolbar;
 - (void)endMaybeResetUnifiedToolbar:(float)aOldHeight;
 
 #if USE_CLICK_HOLD_CONTEXTMENU
@@ -5109,17 +5107,17 @@ static const char* ToEscapedString(NSStr
   return aBuf.get();
 }
 #endif
 
 // Returns PR_TRUE if Gecko claims to have handled the event, PR_FALSE otherwise.
 // We only send Carbon plugin events with NS_KEY_DOWN gecko events, and only send
 // Cocoa plugin events with NS_KEY_PRESS gecko events. This is because we want to
 // send repeat key down events to Cocoa plugins but not Carbon plugins.
-- (PRBool)processKeyDownEvent:(NSEvent*)theEvent keyEquiv:(BOOL)isKeyEquiv
+- (PRBool)processKeyDownEvent:(NSEvent*)theEvent
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_RETURN;
 
   if (!mGeckoChild)
     return NO;
 
 #ifdef PR_LOGGING
   nsCAutoString str1;
@@ -5193,29 +5191,26 @@ static const char* ToEscapedString(NSStr
       mKeyPressHandled = mGeckoChild->DispatchWindowEvent(geckoEvent);
       mKeyPressSent = YES;
       if (!mGeckoChild)
         return (mKeyDownHandled || mKeyPressHandled);
     }
   }
 
   // Let Cocoa interpret the key events, caching IsIMEComposing first.
-  // We don't do it if this came from performKeyEquivalent because
-  // interpretKeyEvents isn't set up to handle those key combinations.
   PRBool wasComposing = mGeckoChild->TextInputHandler()->IsIMEComposing();
   PRBool interpretKeyEventsCalled = PR_FALSE;
-  if (!isKeyEquiv &&
-      (mGeckoChild->TextInputHandler()->IsIMEEnabled() ||
-       mGeckoChild->TextInputHandler()->IsASCIICapableOnly())) {
+  if (mGeckoChild->TextInputHandler()->IsIMEEnabled() ||
+      mGeckoChild->TextInputHandler()->IsASCIICapableOnly()) {
     [super interpretKeyEvents:[NSArray arrayWithObject:theEvent]];
     interpretKeyEventsCalled = PR_TRUE;
   }
 
   if (!mGeckoChild)
-    return (mKeyDownHandled || mKeyPressHandled);;
+    return (mKeyDownHandled || mKeyPressHandled);
 
   if (!mKeyPressSent && nonDeadKeyPress && !wasComposing &&
       !mGeckoChild->TextInputHandler()->IsIMEComposing()) {
     nsKeyEvent geckoEvent(PR_TRUE, NS_KEY_PRESS, nsnull);
     [self convertCocoaKeyEvent:theEvent toGeckoEvent:&geckoEvent];
 
     // If we called interpretKeyEvents and this isn't normal character input
     // then IME probably ate the event for some reason. We do not want to
@@ -5279,16 +5274,25 @@ static const char* ToEscapedString(NSStr
     ::UseInputWindow(mPluginTSMDoc, YES);
     ::ActivateTSMDocument(mPluginTSMDoc);
   } else if (::TSMGetActiveDocument() != mPluginTSMDoc) {
     ::ActivateTSMDocument(mPluginTSMDoc);
   }
 }
 #endif // NP_NO_CARBON
 
+// This is a private API that Cocoa uses.
+// Cocoa will call this after the menu system returns "NO" for "performKeyEquivalent:".
+// We want all they key events we can get so just return YES. In particular, this fixes
+// ctrl-tab - we don't get a "keyDown:" call for that without this.
+- (BOOL)_wantsKeyDownForEvent:(NSEvent*)event
+{
+  return YES;
+}
+
 - (void)keyDown:(NSEvent*)theEvent
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
   if (mGeckoChild && mIsPluginView) {
     // If a plugin has the focus, we need to use an alternate method for
     // handling NSKeyDown and NSKeyUp events (otherwise Carbon-based IME won't
     // work in plugins like the Flash plugin).  The same strategy is used by the
@@ -5343,17 +5347,17 @@ static const char* ToEscapedString(NSStr
     ::TSMSetDocumentProperty(mPluginTSMDoc, kFocusedChildViewTSMDocPropertyTag,
                              sizeof(ChildView *), &self);
     ::TSMProcessRawKeyEvent([theEvent _eventRef]);
     ::TSMRemoveDocumentProperty(mPluginTSMDoc, kFocusedChildViewTSMDocPropertyTag);
     return;
 #endif
   }
 
-  [self processKeyDownEvent:theEvent keyEquiv:NO];
+  [self processKeyDownEvent:theEvent];
 
   NS_OBJC_END_TRY_ABORT_BLOCK;
 }
 
 - (void)keyUp:(NSEvent*)theEvent
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
@@ -5426,164 +5430,16 @@ static const char* ToEscapedString(NSStr
   nsKeyEvent geckoEvent(PR_TRUE, NS_KEY_UP, nsnull);
   [self convertCocoaKeyEvent:theEvent toGeckoEvent:&geckoEvent];
 
   mGeckoChild->DispatchWindowEvent(geckoEvent);
 
   NS_OBJC_END_TRY_ABORT_BLOCK;
 }
 
-- (BOOL)performKeyEquivalent:(NSEvent*)theEvent
-{
-  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_RETURN;
-
-  // don't do anything if we don't have a gecko widget
-  if (!mGeckoChild)
-    return NO;
-
-  nsAutoRetainCocoaObject kungFuDeathGrip(self);
-
-  // If we're not the first responder and the first responder is an NSView
-  // object, pass the event on.  Otherwise (if, for example, the first
-  // responder is an NSWindow object) we should trust the OS to have called
-  // us correctly.
-  id firstResponder = [[self window] firstResponder];
-  if (firstResponder != self) {
-    // Special handling if the other first responder is a ChildView.
-    if ([firstResponder isKindOfClass:[ChildView class]])
-      return [(ChildView *)firstResponder performKeyEquivalent:theEvent];
-    if ([firstResponder isKindOfClass:[NSView class]])
-      return [super performKeyEquivalent:theEvent];
-  }
-
-  // don't process if we're composing, but don't consume the event
-  if (mGeckoChild->TextInputHandler()->IsIMEComposing()) {
-    return NO;
-  }
-
-  UInt32 modifierFlags = [theEvent modifierFlags] & NSDeviceIndependentModifierFlagsMask;
-
-  // Set to true if embedding menus handled the event when a plugin has focus.
-  // We give menus a crack at handling commands before Gecko in the plugin case.
-  BOOL handledByEmbedding = NO;
-
-  // Perform native menu UI feedback even if we stop the event from propagating to it normally.
-  // Recall that the menu system won't actually execute any commands for keyboard command invocations.
-  //
-  // If this is a plugin, we do actually perform the action on keyboard commands. See bug 428047.
-  // If the action on plugins here changes the first responder, don't continue.
-  NSMenu* mainMenu = [NSApp mainMenu];
-  if (mIsPluginView) {
-    if ([mainMenu isKindOfClass:[GeckoNSMenu class]]) {
-      // Maintain a list of cmd+key combinations that we never act on (in the
-      // browser) when the keyboard focus is in a plugin.  What a particular
-      // cmd+key combo means here (to the browser) is governed by browser.dtd,
-      // which "contains the browser main menu items".
-      PRBool dontActOnKeyEquivalent = PR_FALSE;
-      if (modifierFlags == NSCommandKeyMask) {
-        NSString *unmodchars = [theEvent charactersIgnoringModifiers];
-        if ([unmodchars length] == 1) {
-          if ([unmodchars characterAtIndex:0] ==
-              nsMenuBarX::GetLocalizedAccelKey("key_selectAll"))
-            dontActOnKeyEquivalent = PR_TRUE;
-        }
-      }
-      if (dontActOnKeyEquivalent) {
-        [(GeckoNSMenu*)mainMenu performMenuUserInterfaceEffectsForEvent:theEvent];
-      } else {
-        [(GeckoNSMenu*)mainMenu actOnKeyEquivalent:theEvent];
-      }
-    }
-    else {
-      // This is probably an embedding situation. If the native menu handle the event
-      // then return YES from pKE no matter what Gecko or the plugin does.
-      handledByEmbedding = [mainMenu performKeyEquivalent:theEvent];
-    }
-    if ([[self window] firstResponder] != self)
-      return YES;
-  }
-  else {
-    if ([mainMenu isKindOfClass:[GeckoNSMenu class]])
-      [(GeckoNSMenu*)mainMenu performMenuUserInterfaceEffectsForEvent:theEvent];
-  }
-
-  // With Cmd key or Ctrl+Tab or Ctrl+Esc, keyDown will be never called.
-  // Therefore, we need to call processKeyDownEvent from performKeyEquivalent.
-  UInt32 keyCode = [theEvent keyCode];
-  PRBool keyDownNeverFiredEvent = (modifierFlags & NSCommandKeyMask) ||
-           ((modifierFlags & NSControlKeyMask) &&
-            (keyCode == kEscapeKeyCode || keyCode == kTabKeyCode));
-
-  // don't handle this if certain modifiers are down - those should
-  // be sent as normal key up/down events and cocoa will do so automatically
-  // if we reject here
-  if (!keyDownNeverFiredEvent &&
-      (modifierFlags & (NSFunctionKeyMask| NSNumericPadKeyMask)))
-    return handledByEmbedding;
-
-  // Control and option modifiers are used when changing input sources in the
-  // input menu. We need to send such key events via "keyDown:", which will
-  // happen if we return NO here. This only applies to Mac OS X 10.5 and higher,
-  // previous OS versions just call "keyDown:" and not "performKeyEquivalent:"
-  // for such events.
-  if (!keyDownNeverFiredEvent &&
-      (modifierFlags & (NSControlKeyMask | NSAlternateKeyMask)))
-    return handledByEmbedding;
-
-  // At this point we're about to hand the event off to "processKeyDownEvent:keyEquiv:".
-  // Don't bother if this is a Cocoa plugin event, just handle it directly.
-  if (mGeckoChild && mIsPluginView && mPluginEventModel == NPEventModelCocoa) {
-    // Reset complex text input request.
-    mPluginComplexTextInputRequested = NO;
-
-    // Send key down event.
-    nsGUIEvent pluginEvent(PR_TRUE, NS_NON_RETARGETED_PLUGIN_EVENT, mGeckoChild);
-    NPCocoaEvent cocoaEvent;
-    ConvertCocoaKeyEventToNPCocoaEvent(theEvent, cocoaEvent);
-    pluginEvent.pluginEvent = &cocoaEvent;
-    mGeckoChild->DispatchWindowEvent(pluginEvent);
-    if (!mGeckoChild)
-      return YES;
-
-    if (!mPluginComplexTextInputRequested) {
-      // Ideally we'd cancel any TSM composition here.
-      return YES;
-    }
-
-    // We send these events to Carbon TSM but not to the ComplexTextInputPanel.
-    // We assume that events coming in through pKE: are only relevant to TSM.
-#ifndef NP_NO_CARBON
-    [self activatePluginTSMDoc];
-    // We use the active TSM document to pass a pointer to ourselves (the
-    // currently focused ChildView) to PluginKeyEventsHandler().  Because this
-    // pointer is weak, we should retain and release ourselves around the call
-    // to TSMProcessRawKeyEvent().
-    nsAutoRetainCocoaObject kungFuDeathGrip(self);
-    ::TSMSetDocumentProperty(mPluginTSMDoc, kFocusedChildViewTSMDocPropertyTag,
-                             sizeof(ChildView *), &self);
-    ::TSMProcessRawKeyEvent([theEvent _eventRef]);
-    ::TSMRemoveDocumentProperty(mPluginTSMDoc, kFocusedChildViewTSMDocPropertyTag);
-#endif
-
-    return YES;
-  }
-
-  if ([theEvent type] == NSKeyDown) {
-    // We trust the Gecko handled status for cmd key events. See bug 417466 for more info.
-    if (modifierFlags & NSCommandKeyMask)
-      return ([self processKeyDownEvent:theEvent keyEquiv:YES] || handledByEmbedding);
-    else
-      [self processKeyDownEvent:theEvent keyEquiv:YES];
-  }
-
-  return YES;
-
-  NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(NO);
-}
-
 - (void)flagsChanged:(NSEvent*)theEvent
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
   if (!mGeckoChild)
     return;
 
   nsAutoRetainCocoaObject kungFuDeathGrip(self);
--- a/widget/src/cocoa/nsCocoaWindow.mm
+++ b/widget/src/cocoa/nsCocoaWindow.mm
@@ -2039,16 +2039,24 @@ static const NSString* kStateShowsToolba
 }
 
 - (void)invalidateShadow
 {
   [super invalidateShadow];
   mScheduledShadowInvalidation = NO;
 }
 
+- (void) doCommandBySelector:(SEL)aSelector
+{
+  // We override this so that it won't beep if it can't act.
+  // We want to control the beeping for missing or disabled
+  // commands ourselves.
+  [self tryToPerform:aSelector with:nil];
+}
+
 @end
 
 // This class allows us to have a "unified toolbar" style window. It works like this:
 // 1) We set the window's style to textured.
 // 2) Because of this, the background color applies to the entire window, including
 //     the titlebar area. For normal textured windows, the default pattern is a 
 //    "brushed metal" image on Tiger and a unified gradient on Leopard.
 // 3) We set the background color to a custom NSColor subclass that knows how tall the window is.
--- a/widget/src/cocoa/nsMenuBarX.h
+++ b/widget/src/cocoa/nsMenuBarX.h
@@ -64,18 +64,16 @@ public:
 
 // Objective-C class used to allow us to have keyboard commands
 // look like they are doing something but actually do nothing.
 // We allow mouse actions to work normally.
 @interface GeckoNSMenu : NSMenu
 {
 }
 - (BOOL)performKeyEquivalent:(NSEvent*)theEvent;
-- (void)actOnKeyEquivalent:(NSEvent*)theEvent;
-- (void)performMenuUserInterfaceEffectsForEvent:(NSEvent*)theEvent;
 @end
 
 // Objective-C class used as action target for menu items
 @interface NativeMenuItemTarget : NSObject
 {
 }
 -(IBAction)menuItemHit:(id)sender;
 @end
--- a/widget/src/cocoa/nsMenuBarX.mm
+++ b/widget/src/cocoa/nsMenuBarX.mm
@@ -41,16 +41,17 @@
 
 #include "nsMenuBarX.h"
 #include "nsMenuX.h"
 #include "nsMenuItemX.h"
 #include "nsMenuUtilsX.h"
 #include "nsCocoaUtils.h"
 #include "nsCocoaWindow.h"
 #include "nsToolkit.h"
+#include "nsChildView.h"
 
 #include "nsCOMPtr.h"
 #include "nsString.h"
 #include "nsWidgetAtoms.h"
 #include "nsGUIEvent.h"
 #include "nsObjCExceptions.h"
 #include "nsHashtable.h"
 #include "nsThreadUtils.h"
@@ -724,134 +725,135 @@ void nsMenuBarX::SetParent(nsIWidget* aP
 
 
 //
 // Objective-C class used to allow us to have keyboard commands
 // look like they are doing something but actually do nothing.
 // We allow mouse actions to work normally.
 //
 
-// This tells us whether or not pKE is on the stack from a GeckoNSMenu. If it
-// is nil, it is not on the stack. The non-nil value is the object that put it
-// on the stack first.
-static GeckoNSMenu* gPerformKeyEquivOnStack = nil;
-// If this is YES, act on key equivs.
-static BOOL gActOnKeyEquiv = NO;
-// When this variable is set to NO, don't do special command processing.
-static BOOL gActOnSpecialCommands = YES;
+// Controls whether or not native menu items should invoke their commands.
+static BOOL gMenuItemsExecuteCommands = YES;
 
 @implementation GeckoNSMenu
 
 - (BOOL)performKeyEquivalent:(NSEvent *)theEvent
 {
-  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_RETURN;
-
-  NS_ASSERTION(gPerformKeyEquivOnStack != self, "GeckoNSMenu pKE re-entering for the same object!");
+  // If there is no window open we will act on the menu item here.
+  if (![NSApp mainWindow]) {
+    // We've noticed that Mac OS X expects this check in subclasses.
+    // Do it before any actual interaction with this menu.
+    if ([self numberOfItems] <= 0) {
+      return YES;
+    }
 
-  // Don't bother doing this if we don't have any items. It appears as though
-  // the OS will sometimes expect this sort of check.
-  if ([self numberOfItems] <= 0)
-    return NO;
+    return [super performKeyEquivalent:theEvent];
+  }
 
-  if (!gPerformKeyEquivOnStack)
-    gPerformKeyEquivOnStack = self;
-  BOOL rv = [super performKeyEquivalent:theEvent];
-  if (gPerformKeyEquivOnStack == self)
-    gPerformKeyEquivOnStack = nil;
-  return rv;
+  BOOL invokeMenuItemCommand = NO;
 
-  NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(NO);
-}
+  // Plugins normally eat all keyboard commands, this hack
+  // mitigates the problem.
+  NSResponder *firstResponder = [[NSApp keyWindow] firstResponder];
+  if (firstResponder &&
+      [firstResponder isKindOfClass:[ChildView class]] &&
+      [(ChildView*)firstResponder isPluginView]) {
+    invokeMenuItemCommand = YES;
+    // Maintain a list of cmd+key combinations that we never act on (in the
+    // browser) when the keyboard focus is in a plugin.  What a particular
+    // cmd+key combo means here (to the browser) is governed by browser.dtd,
+    // which "contains the browser main menu items".
+    UInt32 modifierFlags = [theEvent modifierFlags] & NSDeviceIndependentModifierFlagsMask;
+    if (modifierFlags == NSCommandKeyMask) {
+      NSString *unmodchars = [theEvent charactersIgnoringModifiers];
+      if ([unmodchars length] == 1) {
+        if ([unmodchars characterAtIndex:0] == nsMenuBarX::GetLocalizedAccelKey("key_selectAll")) {
+          invokeMenuItemCommand = NO;
+        }
+      }
+    }
+  }
 
--(void)actOnKeyEquivalent:(NSEvent *)theEvent
-{
-  gActOnKeyEquiv = YES;
-  [self performKeyEquivalent:theEvent];
-  gActOnKeyEquiv = NO;
-}
+  if (invokeMenuItemCommand) {
+    [super performKeyEquivalent:theEvent];
+  } else {
+    // Show menu interface effects by invoking the native menu
+    // but not actually invoking the XUL command.
+    gMenuItemsExecuteCommands = NO;
+    [super performKeyEquivalent:theEvent];
+    gMenuItemsExecuteCommands = YES;
+  }
 
-- (void)performMenuUserInterfaceEffectsForEvent:(NSEvent*)theEvent
-{
-  gActOnSpecialCommands = NO;
-  [self performKeyEquivalent:theEvent];
-  gActOnSpecialCommands = YES;
+  // Return NO so that we can handle the event via NSView's "keyDown:".
+  return NO;
 }
 
 @end
 
 //
 // Objective-C class used as action target for menu items
 //
 
 @implementation NativeMenuItemTarget
 
 // called when some menu item in this menu gets hit
 -(IBAction)menuItemHit:(id)sender
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
+  if (!gMenuItemsExecuteCommands) {
+    return;
+  }
+
   int tag = [sender tag];
 
   MenuItemInfo* info = [sender representedObject];
   if (!info)
     return;
 
   nsMenuGroupOwnerX* menuGroupOwner = [info menuGroupOwner];
   if (!menuGroupOwner)
     return;
 
   nsMenuBarX* menuBar = nsnull;
   if (menuGroupOwner->MenuObjectType() == eMenuBarObjectType)
     menuBar = static_cast<nsMenuBarX*>(menuGroupOwner);
 
-  // We want to avoid processing app-global commands when we are asked to
-  // perform native menu effects only. This avoids sending events twice,
-  // which can lead to major problems.
-  if (gActOnSpecialCommands) {
-    // Do special processing if this is for an app-global command.
-    if (tag == eCommand_ID_About) {
-      nsIContent* mostSpecificContent = sAboutItemContent;
-      if (menuBar && menuBar->mAboutItemContent)
-        mostSpecificContent = menuBar->mAboutItemContent;
-      nsMenuUtilsX::DispatchCommandTo(mostSpecificContent);
-    }
-    else if (tag == eCommand_ID_Prefs) {
-      nsIContent* mostSpecificContent = sPrefItemContent;
-      if (menuBar && menuBar->mPrefItemContent)
-        mostSpecificContent = menuBar->mPrefItemContent;
+  // Do special processing if this is for an app-global command.
+  if (tag == eCommand_ID_About) {
+    nsIContent* mostSpecificContent = sAboutItemContent;
+    if (menuBar && menuBar->mAboutItemContent)
+      mostSpecificContent = menuBar->mAboutItemContent;
+    nsMenuUtilsX::DispatchCommandTo(mostSpecificContent);
+  }
+  else if (tag == eCommand_ID_Prefs) {
+    nsIContent* mostSpecificContent = sPrefItemContent;
+    if (menuBar && menuBar->mPrefItemContent)
+      mostSpecificContent = menuBar->mPrefItemContent;
+    nsMenuUtilsX::DispatchCommandTo(mostSpecificContent);
+  }
+  else if (tag == eCommand_ID_Quit) {
+    nsIContent* mostSpecificContent = sQuitItemContent;
+    if (menuBar && menuBar->mQuitItemContent)
+      mostSpecificContent = menuBar->mQuitItemContent;
+    // If we have some content for quit we execute it. Otherwise we send a native app terminate
+    // message. If you want to stop a quit from happening, provide quit content and return
+    // the event as unhandled.
+    if (mostSpecificContent) {
       nsMenuUtilsX::DispatchCommandTo(mostSpecificContent);
     }
-    else if (tag == eCommand_ID_Quit) {
-      nsIContent* mostSpecificContent = sQuitItemContent;
-      if (menuBar && menuBar->mQuitItemContent)
-        mostSpecificContent = menuBar->mQuitItemContent;
-      // If we have some content for quit we execute it. Otherwise we send a native app terminate
-      // message. If you want to stop a quit from happening, provide quit content and return
-      // the event as unhandled.
-      if (mostSpecificContent) {
-        nsMenuUtilsX::DispatchCommandTo(mostSpecificContent);
-      }
-      else {
-        [NSApp terminate:nil];
-        return;
-      }
+    else {
+      [NSApp terminate:nil];
+      return;
     }
-    // Quit now if the "active" menu bar has changed (as the result of
-    // processing an app-global command above).  This resolves bmo bug
-    // 430506.
-    if (menuBar != nsnull && menuBar != nsMenuBarX::sLastGeckoMenuBarPainted)
-      return;
   }
-
-  // Don't do anything unless this is not a keyboard command and
-  // this isn't for the hidden window menu. We assume that if there
-  // is no main window then the hidden window menu bar is up, even
-  // if that isn't true for some reason we better play it safe if
-  // there is no main window.
-  if (gPerformKeyEquivOnStack && !gActOnKeyEquiv && [NSApp mainWindow])
+  // Quit now if the "active" menu bar has changed (as the result of
+  // processing an app-global command above).  This resolves bmo bug
+  // 430506.
+  if (menuBar && menuBar != nsMenuBarX::sLastGeckoMenuBarPainted)
     return;
 
   // given the commandID, look it up in our hashtable and dispatch to
   // that menu item.
   if (menuGroupOwner) {
     nsMenuItemX* menuItem = menuGroupOwner->GetMenuItemForCommandID(static_cast<PRUint32>(tag));
     if (menuItem)
       menuItem->DoCommand();
@@ -859,37 +861,37 @@ static BOOL gActOnSpecialCommands = YES;
 
   NS_OBJC_END_TRY_ABORT_BLOCK;
 }
 
 @end
 
 // Objective-C class used for menu items on the Services menu to allow Gecko
 // to override their standard behavior in order to stop key equivalents from
-// firing in certain instances. When gActOnSpecialCommands is NO, we return
+// firing in certain instances. When gMenuItemsExecuteCommands is NO, we return
 // a dummy target and action instead of the actual target and action.
 
 @implementation GeckoServicesNSMenuItem
 
 - (id) target
 {
   id realTarget = [super target];
-  if (gActOnSpecialCommands)
+  if (gMenuItemsExecuteCommands)
     return realTarget;
   else
-    return realTarget != nil ? self : nil;
+    return realTarget ? self : nil;
 }
 
 - (SEL) action
 {
   SEL realAction = [super action];
-  if (gActOnSpecialCommands)
+  if (gMenuItemsExecuteCommands)
     return realAction;
   else
-    return realAction != NULL ? @selector(_doNothing:) : NULL;
+    return realAction ? @selector(_doNothing:) : NULL;
 }
 
 - (void) _doNothing:(id)sender
 {
 }
 
 @end