Skip to content

fix: ensure menu-did-close is emitted for application menus#49075

Merged
ckerr merged 1 commit intomainfrom
fix-close-callback-non-popup
Nov 26, 2025
Merged

fix: ensure menu-did-close is emitted for application menus#49075
ckerr merged 1 commit intomainfrom
fix-close-callback-non-popup

Conversation

@codebytere
Copy link
Copy Markdown
Member

Description of Change

Follow up to #49017

Correct logic for event emission: we should emit "menu-did-close" both if it's a popup and the top level menu is closed, or if it's an application menu, and the current menu's supermenu
is the top-level menu.

Checklist

Release Notes

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

@codebytere codebytere requested a review from jkleinsc November 25, 2025 14:37
@codebytere codebytere added semver/patch backwards-compatible bug fixes target/38-x-y PR should also be added to the "38-x-y" branch. target/39-x-y PR should also be added to the "39-x-y" branch. target/40-x-y PR should also be added to the "40-x-y" branch. labels Nov 25, 2025
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Nov 25, 2025
@codebytere codebytere changed the title fix: ensure menu-did-close is emitted for application menus fix: ensure menu-did-close is emitted for application menus Nov 25, 2025
@codebytere codebytere force-pushed the fix-close-callback-non-popup branch from 0864337 to c03501b Compare November 25, 2025 15:53
@codebytere codebytere force-pushed the fix-close-callback-non-popup branch from c03501b to 3a61a11 Compare November 25, 2025 17:21
@ckerr
Copy link
Copy Markdown
Member

ckerr commented Nov 25, 2025

FTBFS
2025-11-25T17:54:49.6225600Z ##[error]../../electron/shell/browser/ui/cocoa/electron_menu_controller.mm:190:3: error: use of undeclared identifier 'popupCloseCallback'; did you mean 'closeCallback'?
2025-11-25T17:54:49.6228830Z   190 |   popupCloseCallback = std::move(callback);
2025-11-25T17:54:49.6229230Z       |   ^~~~~~~~~~~~~~~~~~
2025-11-25T17:54:49.6229530Z       |   closeCallback
2025-11-25T17:54:49.6230250Z ../../electron/shell/browser/ui/cocoa/electron_menu_controller.h:31:21: note: 'closeCallback' declared here
2025-11-25T17:54:49.6232190Z    31 |   base::OnceClosure closeCallback;
2025-11-25T17:54:49.6232700Z       |                     ^
2025-11-25T17:54:49.6234930Z ##[error]../../electron/shell/browser/ui/cocoa/electron_menu_controller.mm:224:10: error: use of undeclared identifier 'popupCloseCallback'; did you mean 'closeCallback'?
2025-11-25T17:54:49.6235700Z   224 |     if (!popupCloseCallback.is_null()) {
2025-11-25T17:54:49.6236050Z       |          ^~~~~~~~~~~~~~~~~~
2025-11-25T17:54:49.6236410Z       |          closeCallback
2025-11-25T17:54:49.6237150Z ../../electron/shell/browser/ui/cocoa/electron_menu_controller.h:31:21: note: 'closeCallback' declared here
2025-11-25T17:54:49.6237840Z    31 |   base::OnceClosure closeCallback;
2025-11-25T17:54:49.6238210Z       |                     ^
2025-11-25T17:54:49.6239410Z ##[error]../../electron/shell/browser/ui/cocoa/electron_menu_controller.mm:226:32: error: use of undeclared identifier 'popupCloseCallback'; did you mean 'closeCallback'?
2025-11-25T17:54:49.6240210Z   226 |           FROM_HERE, std::move(popupCloseCallback));
2025-11-25T17:54:49.6240710Z       |                                ^~~~~~~~~~~~~~~~~~
2025-11-25T17:54:49.6241100Z       |                                closeCallback
2025-11-25T17:54:49.6241920Z ../../electron/shell/browser/ui/cocoa/electron_menu_controller.h:31:21: note: 'closeCallback' declared here
2025-11-25T17:54:49.6242440Z    31 |   base::OnceClosure closeCallback;
2025-11-25T17:54:49.6242680Z       |                     ^
2025-11-25T17:54:49.6244830Z ##[error]../../electron/shell/browser/ui/cocoa/electron_menu_controller.mm:570:24: error: use of undeclared identifier 'popupCloseCallback'; did you mean 'closeCallback'?
2025-11-25T17:54:49.6245590Z   570 |   bool has_close_cb = !popupCloseCallback.is_null();
2025-11-25T17:54:49.6245950Z       |                        ^~~~~~~~~~~~~~~~~~
2025-11-25T17:54:49.6246310Z       |                        closeCallback
2025-11-25T17:54:49.6247040Z ../../electron/shell/browser/ui/cocoa/electron_menu_controller.h:31:21: note: 'closeCallback' declared here
2025-11-25T17:54:49.6247510Z    31 |   base::OnceClosure closeCallback;
2025-11-25T17:54:49.6247900Z       |                     ^
2025-11-25T17:54:49.6249050Z ##[error]../../electron/shell/browser/ui/cocoa/electron_menu_controller.mm:588:60: error: use of undeclared identifier 'popupCloseCallback'; did you mean 'closeCallback'?
2025-11-25T17:54:49.6249870Z   588 |                                                  std::move(popupCloseCallback));
2025-11-25T17:54:49.6250340Z       |                                                            ^~~~~~~~~~~~~~~~~~
2025-11-25T17:54:49.6250760Z       |                                                            closeCallback
2025-11-25T17:54:49.6251510Z ../../electron/shell/browser/ui/cocoa/electron_menu_controller.h:31:21: note: 'closeCallback' declared here
2025-11-25T17:54:49.6252070Z    31 |   base::OnceClosure closeCallback;

@codebytere codebytere force-pushed the fix-close-callback-non-popup branch from 3a61a11 to 6ee8aea Compare November 26, 2025 08:25
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Nov 26, 2025
Copy link
Copy Markdown
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new commits; reapproving

@ckerr ckerr merged commit 555f507 into main Nov 26, 2025
58 checks passed
@ckerr ckerr deleted the fix-close-callback-non-popup branch November 26, 2025 15:44
@release-clerk
Copy link
Copy Markdown

release-clerk bot commented Nov 26, 2025

Release Notes Persisted

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

@trop
Copy link
Copy Markdown
Contributor

trop bot commented Nov 26, 2025

I have automatically backported this PR to "40-x-y", please check out #49092

@trop
Copy link
Copy Markdown
Contributor

trop bot commented Nov 26, 2025

I have automatically backported this PR to "39-x-y", please check out #49093

@trop trop bot removed the target/40-x-y PR should also be added to the "40-x-y" branch. label Nov 26, 2025
@trop
Copy link
Copy Markdown
Contributor

trop bot commented Nov 26, 2025

I have automatically backported this PR to "38-x-y", please check out #49094

@trop trop bot added merged/39-x-y PR was merged to the "39-x-y" branch. merged/40-x-y PR was merged to the "40-x-y" branch. merged/38-x-y PR was merged to the "38-x-y" branch. and removed target/39-x-y PR should also be added to the "39-x-y" branch. target/38-x-y PR should also be added to the "38-x-y" branch. in-flight/39-x-y in-flight/40-x-y in-flight/38-x-y labels Nov 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged/38-x-y PR was merged to the "38-x-y" branch. merged/39-x-y PR was merged to the "39-x-y" branch. merged/40-x-y PR was merged to the "40-x-y" branch. semver/patch backwards-compatible bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants