Repository: electron/electron
Pull request: fix: ensure `menu-did-close` is emitted for application menus

Backport of #49075

See that PR for details.


Notes: Fixed an issue where `menu-did-close` was not emitted properly for some application menus.

Changed files:
filename: shell/browser/api/electron_api_menu_mac.mm
patch: @@ -155,7 +155,8 @@
   if (rightmostMenuPoint > screenRight)
     position.x = position.x - [menu size].width;
 
-  [popup_controllers_[window_id] setCloseCallback:std::move(close_callback)];
+  [popup_controllers_[window_id]
+      setPopupCloseCallback:std::move(close_callback)];
 
   if (frame && frame->render_frame_host()) {
     auto* rfh = frame->render_frame_host()->GetOutermostMainFrameOrEmbedder();

filename: shell/browser/ui/cocoa/electron_menu_controller.h
patch: @@ -28,15 +28,15 @@ class ElectronMenuModel;
   NSMenu* __strong menu_;
   BOOL isMenuOpen_;
   BOOL useDefaultAccelerator_;
-  base::OnceClosure closeCallback;
+  base::OnceClosure popupCloseCallback;
 }
 
 // Builds a NSMenu from the pre-built model (must not be nil). Changes made
 // to the contents of the model after calling this will not be noticed.
 - (id)initWithModel:(electron::ElectronMenuModel*)model
     useDefaultAccelerator:(BOOL)use;
 
-- (void)setCloseCallback:(base::OnceClosure)callback;
+- (void)setPopupCloseCallback:(base::OnceClosure)callback;
 
 // Populate current NSMenu with |model|.
 - (void)populateWithModel:(electron::ElectronMenuModel*)model;

filename: shell/browser/ui/cocoa/electron_menu_controller.mm
patch: @@ -186,8 +186,8 @@ - (void)dealloc {
   model_ = nullptr;
 }
 
-- (void)setCloseCallback:(base::OnceClosure)callback {
-  closeCallback = std::move(callback);
+- (void)setPopupCloseCallback:(base::OnceClosure)callback {
+  popupCloseCallback = std::move(callback);
 }
 
 - (void)populateWithModel:(electron::ElectronMenuModel*)model {
@@ -221,9 +221,9 @@ - (void)cancel {
     isMenuOpen_ = NO;
     if (model_)
       model_->MenuWillClose();
-    if (!closeCallback.is_null()) {
-      content::GetUIThreadTaskRunner({})->PostTask(FROM_HERE,
-                                                   std::move(closeCallback));
+    if (!popupCloseCallback.is_null()) {
+      content::GetUIThreadTaskRunner({})->PostTask(
+          FROM_HERE, std::move(popupCloseCallback));
     }
   }
 }
@@ -567,18 +567,25 @@ - (void)menuDidClose:(NSMenu*)menu {
   if (!isMenuOpen_)
     return;
 
-  // We should only respond to the top-level menu's close event.
-  if (menu != menu_)
-    return;
+  bool has_close_cb = !popupCloseCallback.is_null();
+
+  // 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.
+  if (menu != menu_) {
+    if (has_close_cb || menu.supermenu != menu_)
+      return;
+  }
 
   isMenuOpen_ = NO;
   if (model_)
     model_->MenuWillClose();
   // Post async task so that itemSelected runs before the close callback
-  // deletes the controller from the map which deallocates it
-  if (!closeCallback.is_null()) {
+  // deletes the controller from the map which deallocates it.
+  if (has_close_cb) {
     content::GetUIThreadTaskRunner({})->PostTask(FROM_HERE,
-                                                 std::move(closeCallback));
+                                                 std::move(popupCloseCallback));
   }
 }