Repository: electron/electron
Pull request: fix: menu close event missing after opening a submenu

#### Description of Change

The issue was that `Menu` would not emit `menu-will-close` event if its submenu has been open first.

~~There are no tests - if anyone has a reliable way to interact with `NSMenu` I'm open to suggestions!~~
Added a unit-like test that directly triggers the `menuWillOpen`/`menuDidClose` sequence that reproduced the issue.

#### Checklist

- [x] PR description included
- [x] I have built and tested this PR
- [x] `npm test` passes
- [x] tests are [changed or added](https://github.com/electron/electron/blob/main/docs/development/testing.md)
- [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 menus to correctly emit `menu-will-close` event when closed after any submenu has been open.

Changed files:
filename: shell/browser/api/electron_api_menu.cc
patch: @@ -311,6 +311,8 @@ void Menu::FillObjectTemplate(v8::Isolate* isolate,
       .SetMethod("_getAcceleratorTextAt", &Menu::GetAcceleratorTextAtForTesting)
 #if BUILDFLAG(IS_MAC)
       .SetMethod("_getUserAcceleratorAt", &Menu::GetUserAcceleratorAt)
+      .SetMethod("_simulateSubmenuCloseSequenceForTesting",
+                 &Menu::SimulateSubmenuCloseSequenceForTesting)
 #endif
       .Build();
 }

filename: shell/browser/api/electron_api_menu.h
patch: @@ -84,6 +84,7 @@ class Menu : public gin::Wrappable<Menu>,
       int command_id,
       ElectronMenuModel::SharingItem* item) const override;
   v8::Local<v8::Value> GetUserAcceleratorAt(int command_id) const;
+  virtual void SimulateSubmenuCloseSequenceForTesting();
 #endif
   void ExecuteCommand(int command_id, int event_flags) override;
   void OnMenuWillShow(ui::SimpleMenuModel* source) override;

filename: shell/browser/api/electron_api_menu_mac.mm
patch: @@ -7,6 +7,7 @@
 #include <string>
 #include <utility>
 
+#include "base/functional/bind.h"
 #include "base/mac/scoped_sending_event.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/task/current_thread.h"
@@ -246,6 +247,20 @@
   return text;
 }
 
+void Menu::SimulateSubmenuCloseSequenceForTesting() {
+  ElectronMenuController* controller =
+      [[ElectronMenuController alloc] initWithModel:model()
+                              useDefaultAccelerator:NO];
+  NSMenu* menu = [controller menu];
+  NSMenu* submenu = menu.itemArray[0].submenu;
+
+  [controller setPopupCloseCallback:base::BindOnce([] {})];
+  [controller menuWillOpen:menu];
+  [controller menuWillOpen:submenu];
+  [controller menuDidClose:submenu];
+  [controller menuDidClose:menu];
+}
+
 void MenuMac::ClosePopupOnUI(int32_t window_id) {
   auto controller = popup_controllers_.find(window_id);
   if (controller != popup_controllers_.end()) {

filename: shell/browser/ui/cocoa/electron_menu_controller.mm
patch: @@ -583,18 +583,25 @@ - (void)menuDidClose:(NSMenu*)menu {
   if (!isMenuOpen_)
     return;
 
-  isMenuOpen_ = NO;
-
   // There are two scenarios where we should emit menu-did-close:
   // 1. It's a popup and the top level menu is closed.
   // 2. It's an application menu, and the current menu's supermenu
   //    is the top-level menu.
   bool has_close_cb = !popupCloseCallback.is_null();
+  bool should_emit_close = true;
   if (menu != menu_) {
-    if (has_close_cb || menu.supermenu != menu_)
-      return;
+    should_emit_close = !has_close_cb && menu.supermenu == menu_;
   }
 
+  [self refreshMenuTree:menu];
+
+  // Submenu's close event arrives before the top-level menu closes.
+  // Don't change isMenuOpen_ until the top-level one receives the close event.
+  if (!should_emit_close)
+    return;
+
+  isMenuOpen_ = NO;
+
   if (model_)
     model_->MenuWillClose();
   // Post async task so that itemSelected runs before the close callback

filename: spec/api-menu-spec.ts
patch: @@ -946,6 +946,27 @@ describe('Menu module', function () {
     }
   });
 
+  ifit(process.platform === 'darwin')(
+    'emits menu close event even if submenu closes first',
+    async () => {
+      const menu = Menu.buildFromTemplate([{
+        label: 'parent',
+        submenu: [{
+          label: 'child'
+        }]
+      }]);
+
+      const menuWillClose = once(menu, 'menu-will-close');
+      (menu as any)._simulateSubmenuCloseSequenceForTesting();
+
+      await Promise.race([
+        menuWillClose,
+        setTimeout(1000).then(() => {
+          throw new Error('menu-will-close was not emitted');
+        })
+      ]);
+    });
+
   describe('Menu.setApplicationMenu', () => {
     it('sets a menu', () => {
       const menu = Menu.buildFromTemplate([