Skip to content

Windows: emit menu item clicks before Menu::Open() returns - #53

Merged
lijy91 merged 1 commit into
libnativeapi:mainfrom
tofutim:fix/windows-menu-click-inside-open
Sep 19, 2026
Merged

lijy91 merged 1 commit into
libnativeapi:mainfrom
tofutim:fix/windows-menu-click-inside-open

Conversation

@tofutim

@tofutim tofutim commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Fixes #52.

The problem

On the native Windows backend, Menu::Open() calls TrackPopupMenu without TPM_RETURNCMD. Windows
therefore posts WM_COMMAND for the chosen item, and Open() returns before that message is handled. The
click is emitted later, when the host window's message loop delivers it.

A binding whose callbacks are only valid while its caller is inside the call cannot receive an event that
late. In nativeapi-flutter 0.2.4 the Dart listener is a NativeCallable.isolateLocal, and the Dart VM
aborts the process when it is invoked with no isolate on the thread. I saw this on Flutter 3.41.2 and
Windows 11, in a release build with the merged platform and UI thread the example README asks for. Every
click on a menu item killed the app:

MenuItem: Item clicked, ID = 50331649
../../../flutter/third_party/dart/runtime/vm/runtime_entry.cc: 5034: error: Cannot invoke native callback outside an isolate.

The opened and closed events were always safe, because WM_INITMENUPOPUP and WM_UNINITMENUPOPUP arrive
while Open() is still on the stack. Only the click was late.

The change

Menu::Open() now passes TPM_RETURNCMD, looks up the returned command id among its items and their
submenus, and fires that item's click before it returns. The WinUI 3 backend already emits clicks inside
Open(), so the two backends now behave alike.

A return of zero under TPM_RETURNCMD means either that the user dismissed the menu or that the call
failed, so the code clears the error state beforehand and returns false only for a real failure.
TPM_NONOTIFY is deliberately absent, since it suppresses WM_INITMENUPOPUP and WM_UNINITMENUPOPUP
as well, and I lost the opened and closed events when I tried it.

tests/menu_backend_test.cpp gains a check in the interactive section. It opens a native-backend menu,
sends Down and Enter from a timer, and asserts that the listener ran before Open() returned.

Verification

The new check fails on main with "Native click was not emitted before Open() returned" and passes with
this change, built with Visual Studio 18 BuildTools and CMake on Windows 11.

I also vendored this patch into cnativeapi 0.2.4 and drove a small Flutter app with it. It ran six open
and pick rounds and six open and dismiss rounds, over a menu rebuilt on every open and a menu built once
and reused. Every round logged its opened event, its closed event and, where an item was picked, its
click, all before open() returned.

One thing this change does not address, in case it matters to you. Destroying a Menu or MenuItem
aborts the process, whether Dart's collector frees it or the program calls dispose() itself. In my app
a menu rebuilt on every open died on the second pick, four runs out of four, while a menu built once and
kept alive survived every run. It is unrelated to the click path above, and I am happy to open a separate
issue with the traces if that would help.

TrackPopupMenu without TPM_RETURNCMD posts WM_COMMAND, so the click was
emitted after Open() returned, from the host message loop. Bindings whose
callbacks are only valid inside a call (Dart NativeCallable.isolateLocal)
abort there: 'Cannot invoke native callback outside an isolate'.

Use TPM_RETURNCMD and emit the chosen item's click (searching submenus)
before returning, as the WinUI 3 backend already does. A genuine
TrackPopupMenu failure still returns false.

menu_backend_test: native-backend check that the click listener ran
before Open() returned.
@lijy91
lijy91 merged commit 0f84355 into libnativeapi:main Sep 19, 2026
@lijy91

lijy91 commented Sep 19, 2026

Copy link
Copy Markdown
Member

Merged, thank you — the diagnosis was exactly right.

I verified it on a real Windows 11 desktop before merging, driving the menu with the mouse (a small test app with its own foreground window, so the menu behaves as it does in a real app):

before (main) with this change
pick a top-level item click arrives after Open() returned, from the message loop inside Open(), exactly once
pick an item in a submenu same, late inside Open(), exactly once
dismiss without picking returns true, no click returns true, no click

No duplicate click in either case, and Open() reports success in all three rounds.

Two things I changed on top of your commit (4042ca4):

  1. Failure detection. SetLastError(ERROR_SUCCESS) + GetLastError() is not a reliable signal here: TrackPopupMenu runs a whole modal loop whose internal calls leave a last error set on success too, so a dismissal could be reported as a failure. WM_INITMENUPOPUP can tell the two apart, so the menu now records that the popup actually appeared and Open() treats only a menu that never appeared as a failure.

  2. The interactive check in tests/menu_backend_test.cpp. It drives the menu with SendInput key strokes, which never reach a popup menu whose owner window is not the foreground window — Open() then never returns and the test hangs forever instead of failing. I hit that on the first run. It is replaced by tests/menu_click_test.cpp, which opens a real menu, checks the events itself (click inside Open(), exactly once, none when dismissed), and leaves the mouse to a driver script in the workspace repo (tools/gui/core_menu_backend_test.ps1). It fails on pre-TPM_RETURNCMD core and passes with this change.

One behavioural note for anyone reading this later: with the native backend the order is now opened -> closed -> click, because WM_UNINITMENUPOPUP arrives while TrackPopupMenu is still running. That matches macOS. The WinUI 3 backend still emits the click before the closed event; the two backends agree on "inside Open()" but not on ordering.

The Rust and C# bindings are already bumped to the new core; the Flutter one follows shortly (another branch is in flight there).

And yes, please do open a separate issue for the Menu / MenuItem destruction crash with the traces — that is worth its own investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Windows] Menu item clicks are emitted after Menu::Open() returns, aborting Dart FFI bindings

2 participants