Repository: electron/electron
Pull request: fix: allow disabling all `NSMenuItems`

#### Description of Change

Closes https://github.com/electron/electron/issues/46505
Closes https://github.com/electron/electron/issues/43130

Reworks menu item validation to manual management and not the confusing combination of automatic and manual management that's been in use up to now. As it stood, menu items with default system-defined selectors didn't call `validateUserInterfaceItem` as this class was not where those were defined. 

This was previously tried in https://github.com/electron/electron/pull/46307, but with the missing understanding that disabling `autoenablesItems` meant `validateUserInterfaceItem` would no longer be called under any circumstances, and this we need to now shift this logic to `menuWillShow` and call it ourselves.

Confirmed not to regress https://github.com/electron/electron/issues/46501, https://github.com/electron/electron/issues/46593, or https://github.com/electron/electron/issues/46485

#### Checklist
<!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->

- [x] PR description included and stakeholders cc'd
- [x] `npm test` passes
- [x] [PR release notes](https://github.com/electron/clerk/blob/main/README.md) describe the change in a way relevant to app developers, and are [capitalized, punctuated, and past tense](https://github.com/electron/clerk/blob/main/README.md#examples).

#### Release Notes

Notes: Fixed an issue that menu items on macOS could not be disabled under all circumstances. 

Changed files:
filename: lib/browser/api/menu.ts
patch: @@ -25,11 +25,30 @@ Menu.prototype._isCommandIdChecked = function (id) {
 };
 
 Menu.prototype._isCommandIdEnabled = function (id) {
-  return this.commandsMap[id] ? this.commandsMap[id].enabled : false;
+  const item = this.commandsMap[id];
+  if (!item) return false;
+
+  const focusedWindow = BaseWindow.getFocusedWindow();
+
+  if (item.role === 'minimize' && focusedWindow) {
+    return focusedWindow.isMinimizable();
+  }
+
+  if (item.role === 'togglefullscreen' && focusedWindow) {
+    return focusedWindow.isFullScreenable();
+  }
+
+  if (item.role === 'close' && focusedWindow) {
+    return focusedWindow.isClosable();
+  }
+
+  return item.enabled;
 };
+
 Menu.prototype._shouldCommandIdWorkWhenHidden = function (id) {
   return this.commandsMap[id] ? !!this.commandsMap[id].acceleratorWorksWhenHidden : false;
 };
+
 Menu.prototype._isCommandIdVisible = function (id) {
   return this.commandsMap[id] ? this.commandsMap[id].visible : false;
 };

filename: shell/browser/ui/cocoa/electron_menu_controller.mm
patch: @@ -90,6 +90,7 @@ bool MenuHasVisibleItems(const electron::ElectronMenuModel* model) {
 // "(empty)" into the submenu. Matches Windows behavior.
 NSMenu* MakeEmptySubmenu() {
   NSMenu* submenu = [[NSMenu alloc] initWithTitle:@""];
+  submenu.autoenablesItems = NO;
   NSString* empty_menu_title =
       l10n_util::GetNSString(IDS_APP_MENU_EMPTY_SUBMENU);
 
@@ -231,6 +232,9 @@ - (void)cancel {
 // be invoked recursively.
 - (NSMenu*)menuFromModel:(electron::ElectronMenuModel*)model {
   NSMenu* menu = [[NSMenu alloc] initWithTitle:@""];
+  // We manually manage enabled/disabled/hidden state for every item,
+  // including Cocoa role-based selectors.
+  menu.autoenablesItems = NO;
 
   const int count = model->GetItemCount();
   for (int index = 0; index < count; index++) {
@@ -240,6 +244,7 @@ - (NSMenu*)menuFromModel:(electron::ElectronMenuModel*)model {
       [self addItemToMenu:menu atIndex:index fromModel:model];
   }
 
+  menu.delegate = self;
   return menu;
 }
 
@@ -294,9 +299,11 @@ - (NSMenu*)createShareMenuForItem:(const SharingItem&)item {
   if ([items count] == 0)
     return MakeEmptySubmenu();
   NSMenu* menu = [[NSMenu alloc] init];
+  menu.autoenablesItems = NO;
   NSArray* services = [NSSharingService sharingServicesForItems:items];
   for (NSSharingService* service in services)
     [menu addItem:[self menuItemForService:service withItems:items]];
+  [menu setDelegate:self];
   return menu;
 }
 
@@ -353,27 +360,22 @@ - (NSMenuItem*)makeMenuItemForIndex:(NSInteger)index
     std::u16string title = u"Services";
     NSString* sub_label = l10n_util::FixUpWindowsStyleLabel(title);
 
-    [item setTarget:nil];
-    [item setAction:nil];
+    item.target = nil;
+    item.action = nil;
     NSMenu* submenu = [[NSMenu alloc] initWithTitle:sub_label];
-    [item setSubmenu:submenu];
+    item.submenu = submenu;
     [NSApp setServicesMenu:submenu];
   } else if (role == u"sharemenu") {
     SharingItem sharing_item;
     model->GetSharingItemAt(index, &sharing_item);
-    [item setTarget:nil];
-    [item setAction:nil];
+    item.target = nil;
+    item.action = nil;
     [item setSubmenu:[self createShareMenuForItem:sharing_item]];
   } else if (type == electron::ElectronMenuModel::TYPE_SUBMENU &&
              model->IsVisibleAt(index)) {
-    // We need to specifically check that the submenu top-level item has been
-    // enabled as it's not validated by validateUserInterfaceItem
-    if (!model->IsEnabledAt(index))
-      [item setEnabled:NO];
-
     // Recursively build a submenu from the sub-model at this index.
-    [item setTarget:nil];
-    [item setAction:nil];
+    item.target = nil;
+    item.action = nil;
     electron::ElectronMenuModel* submenuModel =
         static_cast<electron::ElectronMenuModel*>(
             model->GetSubmenuModelAt(index));
@@ -388,8 +390,12 @@ - (NSMenuItem*)makeMenuItemForIndex:(NSInteger)index
       }
     }
 
-    [submenu setTitle:[item title]];
-    [item setSubmenu:submenu];
+    submenu.title = item.title;
+    item.submenu = submenu;
+    item.tag = index;
+    item.representedObject =
+        [WeakPtrToElectronMenuModelAsNSObject weakPtrForModel:model];
+    submenu.delegate = self;
 
     // Set submenu's role.
     if ((role == u"window" || role == u"windowmenu") && [submenu numberOfItems])
@@ -404,9 +410,9 @@ - (NSMenuItem*)makeMenuItemForIndex:(NSInteger)index
     // the model so hierarchical menus check the correct index in the correct
     // model. Setting the target to |self| allows this class to participate
     // in validation of the menu items.
-    [item setTag:index];
-    [item setRepresentedObject:[WeakPtrToElectronMenuModelAsNSObject
-                                   weakPtrForModel:model]];
+    item.tag = index;
+    item.representedObject =
+        [WeakPtrToElectronMenuModelAsNSObject weakPtrForModel:model];
     ui::Accelerator accelerator;
     if (model->GetAcceleratorAtWithParams(index, useDefaultAccelerator_,
                                           &accelerator)) {
@@ -434,20 +440,20 @@ - (NSMenuItem*)makeMenuItemForIndex:(NSInteger)index
         ui::MacKeyCodeForWindowsKeyCode(accelerator.key_code(), modifier_mask,
                                         nullptr, &character);
       }
-      [item setKeyEquivalent:[NSString stringWithFormat:@"%C", character]];
-      [item setKeyEquivalentModifierMask:modifier_mask];
+      item.keyEquivalent = [NSString stringWithFormat:@"%C", character];
+      item.keyEquivalentModifierMask = modifier_mask;
     }
 
     [(id)item
         setAllowsKeyEquivalentWhenHidden:(model->WorksWhenHiddenAt(index))];
 
     // Set menu item's role.
-    [item setTarget:self];
+    item.target = self;
     if (!role.empty()) {
       for (const Role& pair : kRolesMap) {
         if (role == base::ASCIIToUTF16(pair.role)) {
-          [item setTarget:nil];
-          [item setAction:pair.selector];
+          item.target = nil;
+          item.action = pair.selector;
           break;
         }
       }
@@ -457,6 +463,37 @@ - (NSMenuItem*)makeMenuItemForIndex:(NSInteger)index
   return item;
 }
 
+- (void)applyStateToMenuItem:(NSMenuItem*)item {
+  id represented = item.representedObject;
+  if (!represented)
+    return;
+
+  electron::ElectronMenuModel* model =
+      [WeakPtrToElectronMenuModelAsNSObject getFrom:represented];
+  if (!model)
+    return;
+
+  NSInteger index = item.tag;
+  int count = model->GetItemCount();
+  if (index < 0 || index >= count)
+    return;
+
+  item.hidden = !model->IsVisibleAt(index);
+  item.enabled = model->IsEnabledAt(index);
+  item.state = model->IsItemCheckedAt(index) ? NSControlStateValueOn
+                                             : NSControlStateValueOff;
+}
+
+// Recursively refreshes the menu tree starting from |menu|, applying the
+// model state to each menu item.
+- (void)refreshMenuTree:(NSMenu*)menu {
+  for (NSMenuItem* item in menu.itemArray) {
+    [self applyStateToMenuItem:item];
+    if (item.submenu)
+      [self refreshMenuTree:item.submenu];
+  }
+}
+
 // Adds an item or a hierarchical menu to the item at the |index|,
 // associated with the entry in the model identified by |modelIndex|.
 - (void)addItemToMenu:(NSMenu*)menu
@@ -466,32 +503,6 @@ - (void)addItemToMenu:(NSMenu*)menu
            atIndex:index];
 }
 
-// Called before the menu is to be displayed to update the state (enabled,
-// radio, etc) of each item in the menu.
-- (BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item {
-  SEL action = [item action];
-  if (action == @selector(performShare:))
-    return YES;
-  if (action != @selector(itemSelected:))
-    return NO;
-
-  NSInteger modelIndex = [item tag];
-  electron::ElectronMenuModel* model = [WeakPtrToElectronMenuModelAsNSObject
-      getFrom:[(id)item representedObject]];
-  DCHECK(model);
-  if (model) {
-    BOOL checked = model->IsItemCheckedAt(modelIndex);
-    DCHECK([(id)item isKindOfClass:[NSMenuItem class]]);
-
-    [(id)item
-        setState:(checked ? NSControlStateValueOn : NSControlStateValueOff)];
-    [(id)item setHidden:(!model->IsVisibleAt(modelIndex))];
-
-    return model->IsEnabledAt(modelIndex);
-  }
-  return NO;
-}
-
 // Called when the user chooses a particular menu item. |sender| is the menu
 // item chosen.
 - (void)itemSelected:(id)sender {
@@ -526,10 +537,11 @@ - (NSMenu*)menu {
     menu_ = menu;
   } else {
     menu_ = [[NSMenu alloc] initWithTitle:@""];
+    menu_.autoenablesItems = NO;
     if (model_)
       [self populateWithModel:model_.get()];
   }
-  [menu_ setDelegate:self];
+  menu_.delegate = self;
   return menu_;
 }
 
@@ -539,6 +551,7 @@ - (BOOL)isMenuOpen {
 
 - (void)menuWillOpen:(NSMenu*)menu {
   isMenuOpen_ = YES;
+  [self refreshMenuTree:menu];
   if (model_)
     model_->MenuWillShow();
 }