Bug 1855346: Ensure that menu items added by macOS also don't execute commands when we expect all commands to be suppressed. Inspired by research by :chesterbr and :bintoro, and a patch by :chesterbr. r=mstange a=RyanVM

toolkit/xre/MacApplicationDelegate.mm

@@ -165,7 +165,7 @@ nsTArray<nsCString> TakeStartupURLs() { return std::move(StartupURLs()); }
     if (![NSApp windowsMenu]) {
       // If the application has a windows menu, it will keep it up to date and
       // prepend the window list to the Dock menu automatically.
-      NSMenu* windowsMenu = [[NSMenu alloc] initWithTitle:@"Window"];
+      NSMenu* windowsMenu = [[GeckoNSMenu alloc] initWithTitle:@"Window"];
       [NSApp setWindowsMenu:windowsMenu];
       [windowsMenu release];
     }
@@ -209,7 +209,7 @@ nsTArray<nsCString> TakeStartupURLs() { return std::move(StartupURLs()); }
   NS_OBJC_BEGIN_TRY_BLOCK_RETURN;
 
   // Create the NSMenu that will contain the dock menu items.
-  NSMenu* menu = [[[NSMenu alloc] initWithTitle:@""] autorelease];
+  NSMenu* menu = [[[GeckoNSMenu alloc] initWithTitle:@""] autorelease];
   [menu setAutoenablesItems:NO];
 
   // Add application-specific dock menu items. On error, do not insert the

widget/cocoa/nsCocoaUtils.mm

@@ -347,7 +347,7 @@ void nsCocoaUtils::PrepareForNativeAppModalDialog() {
       "Main menu does not have any items, something is terribly wrong!");
 
   // Create new menu bar for use with modal dialog
-  NSMenu* newMenuBar = [[NSMenu alloc] initWithTitle:@""];
+  NSMenu* newMenuBar = [[GeckoNSMenu alloc] initWithTitle:@""];
 
   // Swap in our app menu. Note that the event target is whatever window is up
   // when the app modal dialog goes up.

widget/cocoa/nsMenuBarX.h

@@ -37,34 +37,22 @@ class Element;
 - (id)initWithApplicationMenu:(nsMenuBarX*)aApplicationMenu;
 @end
 
-// Objective-C class used to allow us to intervene with keyboard event handling.
+// Objective-C class used for menu items to allow Gecko to override their
-// We allow mouse actions to work normally.
+// standard behavior in order to stop key equivalents from firing in certain
-@interface GeckoNSMenu : NSMenu {
+// instances.
-}
+@interface GeckoNSMenuItem : NSMenuItem {
-- (BOOL)performSuperKeyEquivalent:(NSEvent*)aEvent;
-@end
-
-// Objective-C class used as action target for menu items
-@interface NativeMenuItemTarget : NSObject {
-}
-- (IBAction)menuItemHit:(id)aSender;
-@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.
-@interface GeckoServicesNSMenuItem : NSMenuItem {
 }
 - (id)target;
 - (SEL)action;
 - (void)_doNothing:(id)aSender;
 @end
 
-// Objective-C class used as the Services menu so that Gecko can override the
+// Objective-C class used to allow us to intervene with keyboard event handling,
-// standard behavior of the Services menu in order to stop key equivalents
+// particularly to stop key equivalents from firing in certain instances. We
-// from firing in certain instances.
+// allow mouse actions to work normally.
-@interface GeckoServicesNSMenu : NSMenu {
+@interface GeckoNSMenu : NSMenu {
 }
+- (BOOL)performSuperKeyEquivalent:(NSEvent*)aEvent;
 - (void)addItem:(NSMenuItem*)aNewItem;
 - (NSMenuItem*)addItemWithTitle:(NSString*)aString
                          action:(SEL)aSelector
@@ -77,6 +65,12 @@ class Element;
 - (void)_overrideClassOfMenuItem:(NSMenuItem*)aMenuItem;
 @end
 
+// Objective-C class used as action target for menu items.
+@interface NativeMenuItemTarget : NSObject {
+}
+- (IBAction)menuItemHit:(id)aSender;
+@end
+
 // Once instantiated, this object lives until its DOM node or its parent window
 // is destroyed. Do not hold references to this, they can become invalid any
 // time the DOM node can be destroyed.

widget/cocoa/nsMenuBarX.mm

@@ -5,12 +5,13 @@
 
 #include <objc/objc-runtime.h>
 
+#include "nsChildView.h"
+#include "nsCocoaUtils.h"
+#include "nsCocoaWindow.h"
 #include "nsMenuBarX.h"
-#include "nsMenuX.h"
 #include "nsMenuItemX.h"
 #include "nsMenuUtilsX.h"
-#include "nsCocoaUtils.h"
+#include "nsMenuX.h"
-#include "nsChildView.h"
 
 #include "nsCOMPtr.h"
 #include "nsString.h"
@@ -37,6 +38,11 @@ NSMenu* sApplicationMenu = nil;
 BOOL sApplicationMenuIsFallback = NO;
 BOOL gSomeMenuBarPainted = NO;
 
+// Controls whether or not native menu items should invoke their commands. See
+// class comments for `GeckoNSMenuItem` and `GeckoNSMenu` below for an
+// explanation of why this switch is necessary.
+static BOOL gMenuItemsExecuteCommands = YES;
+
 // defined in nsCocoaWindow.mm.
 extern BOOL sTouchBarIsInitialized;
 
@@ -191,9 +197,9 @@ void nsMenuBarX::ConstructFallbackNativeMenus() {
   }
   sApplicationMenu.delegate = mApplicationMenuDelegate;
   NSMenuItem* quitMenuItem =
-      [[[NSMenuItem alloc] initWithTitle:labelStr
+      [[[GeckoNSMenuItem alloc] initWithTitle:labelStr
-                                  action:@selector(menuItemHit:)
+                                       action:@selector(menuItemHit:)
-                           keyEquivalent:keyStr] autorelease];
+                                keyEquivalent:keyStr] autorelease];
   quitMenuItem.target = nsMenuBarX::sNativeEventTarget;
   quitMenuItem.tag = eCommand_ID_Quit;
   [sApplicationMenu addItem:quitMenuItem];
@@ -681,9 +687,9 @@ NSMenuItem* nsMenuBarX::CreateNativeAppMenuItem(nsMenuX* aMenu,
   }
 
   // put together the actual NSMenuItem
-  NSMenuItem* newMenuItem = [[NSMenuItem alloc] initWithTitle:labelString
+  NSMenuItem* newMenuItem = [[GeckoNSMenuItem alloc] initWithTitle:labelString
-                                                       action:aAction
+                                                            action:aAction
-                                                keyEquivalent:keyEquiv];
+                                                     keyEquivalent:keyEquiv];
 
   newMenuItem.tag = aTag;
   newMenuItem.target = aTarget;
@@ -806,7 +812,7 @@ void nsMenuBarX::CreateApplicationMenu(nsMenuX* aMenu) {
       [sApplicationMenu addItem:itemBeingAdded];
 
       // set this menu item up as the Mac OS X Services menu
-      NSMenu* servicesMenu = [[GeckoServicesNSMenu alloc] initWithTitle:@""];
+      NSMenu* servicesMenu = [[GeckoNSMenu alloc] initWithTitle:@""];
       itemBeingAdded.submenu = servicesMenu;
       NSApp.servicesMenu = servicesMenu;
 
@@ -896,9 +902,9 @@ void nsMenuBarX::CreateApplicationMenu(nsMenuX* aMenu) {
       // the current application does not have a DOM node for "Quit". Add one
       // anyway, in English.
       NSMenuItem* defaultQuitItem =
-          [[[NSMenuItem alloc] initWithTitle:@"Quit"
+          [[[GeckoNSMenuItem alloc] initWithTitle:@"Quit"
-                                      action:@selector(menuItemHit:)
+                                           action:@selector(menuItemHit:)
-                               keyEquivalent:@"q"] autorelease];
+                                    keyEquivalent:@"q"] autorelease];
       defaultQuitItem.target = nsMenuBarX::sNativeEventTarget;
       defaultQuitItem.tag = eCommand_ID_Quit;
       [sApplicationMenu addItem:defaultQuitItem];
@@ -908,15 +914,38 @@ void nsMenuBarX::CreateApplicationMenu(nsMenuX* aMenu) {
   NS_OBJC_END_TRY_ABORT_BLOCK;
 }
 
+// Objective-C class used for menu items to allow Gecko to override their
+// standard behavior in order to stop key equivalents from firing in certain
+// instances. When gMenuItemsExecuteCommands is NO, we return a dummy target and
+// action instead of the actual target and action.
+@implementation GeckoNSMenuItem
+
+- (id)target {
+  id realTarget = super.target;
+  if (gMenuItemsExecuteCommands) {
+    return realTarget;
+  }
+  return realTarget ? self : nil;
+}
+
+- (SEL)action {
+  SEL realAction = super.action;
+  if (gMenuItemsExecuteCommands) {
+    return realAction;
+  }
+  return realAction ? @selector(_doNothing:) : nullptr;
+}
+
+- (void)_doNothing:(id)aSender {
+}
+
+@end
+
 //
 // 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.
 //
-
-// Controls whether or not native menu items should invoke their commands.
-static BOOL gMenuItemsExecuteCommands = YES;
-
 @implementation GeckoNSMenu
 
 // Keyboard commands should not cause menu items to invoke their
@@ -945,8 +974,14 @@ static BOOL gMenuItemsExecuteCommands = YES;
 
   NSResponder* firstResponder = keyWindow.firstResponder;
 
-  gMenuItemsExecuteCommands = NO;
+  if ([keyWindow isKindOfClass:[BaseWindow class]]) {
+    gMenuItemsExecuteCommands = NO;
+  }
+
+  NS_OBJC_BEGIN_TRY_IGNORE_BLOCK
   [super performKeyEquivalent:aEvent];
+  NS_OBJC_END_TRY_IGNORE_BLOCK
+
   gMenuItemsExecuteCommands = YES;  // return to default
 
   // Return YES if we invoked a command and there is now no key window or we
@@ -964,6 +999,46 @@ static BOOL gMenuItemsExecuteCommands = YES;
   return [super performKeyEquivalent:aEvent];
 }
 
+- (void)addItem:(NSMenuItem*)aNewItem {
+  [self _overrideClassOfMenuItem:aNewItem];
+  [super addItem:aNewItem];
+}
+
+- (NSMenuItem*)addItemWithTitle:(NSString*)aString
+                         action:(SEL)aSelector
+                  keyEquivalent:(NSString*)aKeyEquiv {
+  NSMenuItem* newItem = [super addItemWithTitle:aString
+                                         action:aSelector
+                                  keyEquivalent:aKeyEquiv];
+  [self _overrideClassOfMenuItem:newItem];
+  return newItem;
+}
+
+- (void)insertItem:(NSMenuItem*)aNewItem atIndex:(NSInteger)aIndex {
+  [self _overrideClassOfMenuItem:aNewItem];
+  [super insertItem:aNewItem atIndex:aIndex];
+}
+
+- (NSMenuItem*)insertItemWithTitle:(NSString*)aString
+                            action:(SEL)aSelector
+                     keyEquivalent:(NSString*)aKeyEquiv
+                           atIndex:(NSInteger)aIndex {
+  NSMenuItem* newItem = [super insertItemWithTitle:aString
+                                            action:aSelector
+                                     keyEquivalent:aKeyEquiv
+                                           atIndex:aIndex];
+  [self _overrideClassOfMenuItem:newItem];
+  return newItem;
+}
+
+- (void)_overrideClassOfMenuItem:(NSMenuItem*)aMenuItem {
+  if ([aMenuItem class] == [NSMenuItem class]) {
+    // See class comment for `GeckoNSMenuItem` above for an explanation of why
+    // we do this.
+    object_setClass(aMenuItem, [GeckoNSMenuItem class]);
+  }
+}
+
 @end
 
 //
@@ -974,9 +1049,9 @@ static BOOL gMenuItemsExecuteCommands = YES;
 
 // called when some menu item in this menu gets hit
 - (IBAction)menuItemHit:(id)aSender {
-  if (!gMenuItemsExecuteCommands) {
+  // We should never get here when we do not want menu items to execute their
-    return;
+  // commands.
-  }
+  MOZ_RELEASE_ASSERT(gMenuItemsExecuteCommands);
 
   if (![aSender isKindOfClass:[NSMenuItem class]]) {
     return;
@@ -1093,77 +1168,3 @@ static BOOL gMenuItemsExecuteCommands = YES;
 }
 
 @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 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 (gMenuItemsExecuteCommands) {
-    return realTarget;
-  }
-  return realTarget ? self : nil;
-}
-
-- (SEL)action {
-  SEL realAction = super.action;
-  if (gMenuItemsExecuteCommands) {
-    return realAction;
-  }
-  return realAction ? @selector(_doNothing:) : nullptr;
-}
-
-- (void)_doNothing:(id)aSender {
-}
-
-@end
-
-// Objective-C class used as the Services menu so that Gecko can override the
-// standard behavior of the Services menu in order to stop key equivalents
-// from firing in certain instances.
-
-@implementation GeckoServicesNSMenu
-
-- (void)addItem:(NSMenuItem*)aNewItem {
-  [self _overrideClassOfMenuItem:aNewItem];
-  [super addItem:aNewItem];
-}
-
-- (NSMenuItem*)addItemWithTitle:(NSString*)aString
-                         action:(SEL)aSelector
-                  keyEquivalent:(NSString*)aKeyEquiv {
-  NSMenuItem* newItem = [super addItemWithTitle:aString
-                                         action:aSelector
-                                  keyEquivalent:aKeyEquiv];
-  [self _overrideClassOfMenuItem:newItem];
-  return newItem;
-}
-
-- (void)insertItem:(NSMenuItem*)aNewItem atIndex:(NSInteger)aIndex {
-  [self _overrideClassOfMenuItem:aNewItem];
-  [super insertItem:aNewItem atIndex:aIndex];
-}
-
-- (NSMenuItem*)insertItemWithTitle:(NSString*)aString
-                            action:(SEL)aSelector
-                     keyEquivalent:(NSString*)aKeyEquiv
-                           atIndex:(NSInteger)aIndex {
-  NSMenuItem* newItem = [super insertItemWithTitle:aString
-                                            action:aSelector
-                                     keyEquivalent:aKeyEquiv
-                                           atIndex:aIndex];
-  [self _overrideClassOfMenuItem:newItem];
-  return newItem;
-}
-
-- (void)_overrideClassOfMenuItem:(NSMenuItem*)aMenuItem {
-  if ([aMenuItem class] == [NSMenuItem class]) {
-    object_setClass(aMenuItem, [GeckoServicesNSMenuItem class]);
-  }
-}
-
-@end

widget/cocoa/nsMenuItemX.mm

@@ -79,9 +79,9 @@ nsMenuItemX::nsMenuItemX(nsMenuX* aParent, const nsString& aLabel,
   } else {
     NSString* newCocoaLabelString =
         nsMenuUtilsX::GetTruncatedCocoaLabel(aLabel);
-    mNativeMenuItem = [[NSMenuItem alloc] initWithTitle:newCocoaLabelString
+    mNativeMenuItem = [[GeckoNSMenuItem alloc] initWithTitle:newCocoaLabelString
-                                                 action:nil
+                                                      action:nil
-                                          keyEquivalent:@""];
+                                               keyEquivalent:@""];
 
     mIsChecked = mContent->AsElement()->AttrValueIs(
         kNameSpaceID_None, nsGkAtoms::checked, nsGkAtoms::_true, eCaseMatters);

widget/cocoa/nsMenuUtilsX.mm

@@ -133,23 +133,24 @@ NSMenuItem* nsMenuUtilsX::GetStandardEditMenuItem() {
   // we return here isn't always released before it needs to be added to
   // another menu.  See bmo bug 468393.
   NSMenuItem* standardEditMenuItem =
-      [[[NSMenuItem alloc] initWithTitle:@"Edit" action:nil
+      [[[GeckoNSMenuItem alloc] initWithTitle:@"Edit"
-                           keyEquivalent:@""] autorelease];
+                                       action:nil
-  NSMenu* standardEditMenu = [[NSMenu alloc] initWithTitle:@"Edit"];
+                                keyEquivalent:@""] autorelease];
+  NSMenu* standardEditMenu = [[GeckoNSMenu alloc] initWithTitle:@"Edit"];
   standardEditMenuItem.submenu = standardEditMenu;
   [standardEditMenu release];
 
   // Add Undo
-  NSMenuItem* undoItem = [[NSMenuItem alloc] initWithTitle:@"Undo"
+  NSMenuItem* undoItem = [[GeckoNSMenuItem alloc] initWithTitle:@"Undo"
-                                                    action:@selector(undo:)
+                                                         action:@selector(undo:)
-                                             keyEquivalent:@"z"];
+                                                  keyEquivalent:@"z"];
   [standardEditMenu addItem:undoItem];
   [undoItem release];
 
   // Add Redo
-  NSMenuItem* redoItem = [[NSMenuItem alloc] initWithTitle:@"Redo"
+  NSMenuItem* redoItem = [[GeckoNSMenuItem alloc] initWithTitle:@"Redo"
-                                                    action:@selector(redo:)
+                                                         action:@selector(redo:)
-                                             keyEquivalent:@"Z"];
+                                                  keyEquivalent:@"Z"];
   [standardEditMenu addItem:redoItem];
   [redoItem release];
 
@@ -157,38 +158,40 @@ NSMenuItem* nsMenuUtilsX::GetStandardEditMenuItem() {
   [standardEditMenu addItem:[NSMenuItem separatorItem]];
 
   // Add Cut
-  NSMenuItem* cutItem = [[NSMenuItem alloc] initWithTitle:@"Cut"
+  NSMenuItem* cutItem = [[GeckoNSMenuItem alloc] initWithTitle:@"Cut"
-                                                   action:@selector(cut:)
+                                                        action:@selector(cut:)
-                                            keyEquivalent:@"x"];
+                                                 keyEquivalent:@"x"];
   [standardEditMenu addItem:cutItem];
   [cutItem release];
 
   // Add Copy
-  NSMenuItem* copyItem = [[NSMenuItem alloc] initWithTitle:@"Copy"
+  NSMenuItem* copyItem = [[GeckoNSMenuItem alloc] initWithTitle:@"Copy"
-                                                    action:@selector(copy:)
+                                                         action:@selector(copy:)
-                                             keyEquivalent:@"c"];
+                                                  keyEquivalent:@"c"];
   [standardEditMenu addItem:copyItem];
   [copyItem release];
 
   // Add Paste
-  NSMenuItem* pasteItem = [[NSMenuItem alloc] initWithTitle:@"Paste"
+  NSMenuItem* pasteItem =
-                                                     action:@selector(paste:)
+      [[GeckoNSMenuItem alloc] initWithTitle:@"Paste"
-                                              keyEquivalent:@"v"];
+                                      action:@selector(paste:)
+                               keyEquivalent:@"v"];
   [standardEditMenu addItem:pasteItem];
   [pasteItem release];
 
   // Add Delete
-  NSMenuItem* deleteItem = [[NSMenuItem alloc] initWithTitle:@"Delete"
+  NSMenuItem* deleteItem =
-                                                      action:@selector(delete:)
+      [[GeckoNSMenuItem alloc] initWithTitle:@"Delete"
-                                               keyEquivalent:@""];
+                                      action:@selector(delete:)
+                               keyEquivalent:@""];
   [standardEditMenu addItem:deleteItem];
   [deleteItem release];
 
   // Add Select All
   NSMenuItem* selectAllItem =
-      [[NSMenuItem alloc] initWithTitle:@"Select All"
+      [[GeckoNSMenuItem alloc] initWithTitle:@"Select All"
-                                 action:@selector(selectAll:)
+                                      action:@selector(selectAll:)
-                          keyEquivalent:@"a"];
+                               keyEquivalent:@"a"];
   [standardEditMenu addItem:selectAllItem];
   [selectAllItem release];
 

widget/cocoa/nsMenuX.mm

@@ -116,9 +116,9 @@ nsMenuX::nsMenuX(nsMenuParentX* aParent, nsMenuGroupOwnerX* aMenuGroupOwner,
   mVisible = !nsMenuUtilsX::NodeIsHiddenOrCollapsed(mContent);
 
   NSString* newCocoaLabelString = nsMenuUtilsX::GetTruncatedCocoaLabel(mLabel);
-  mNativeMenuItem = [[NSMenuItem alloc] initWithTitle:newCocoaLabelString
+  mNativeMenuItem = [[GeckoNSMenuItem alloc] initWithTitle:newCocoaLabelString
-                                               action:nil
+                                                    action:nil
-                                        keyEquivalent:@""];
+                                             keyEquivalent:@""];
   mNativeMenuItem.submenu = mNativeMenu;
 
   SetEnabled(!mContent->IsElement() ||
@@ -794,9 +794,9 @@ void nsMenuX::InsertPlaceholderIfNeeded() {
 
   if ([mNativeMenu numberOfItems] == 0) {
     MOZ_RELEASE_ASSERT(mVisibleItemsCount == 0);
-    NSMenuItem* item = [[NSMenuItem alloc] initWithTitle:@""
+    NSMenuItem* item = [[GeckoNSMenuItem alloc] initWithTitle:@""
-                                                  action:nil
+                                                       action:nil
-                                           keyEquivalent:@""];
+                                                keyEquivalent:@""];
     item.enabled = NO;
     item.view =
         [[[NSView alloc] initWithFrame:NSMakeRect(0, 0, 150, 1)] autorelease];