Skip to content

Commit 4042ca4

Browse files
committed
Decide a popup menu's failure from whether it was shown
TrackPopupMenu with TPM_RETURNCMD returns 0 both when the user dismissed the menu and when the call failed. The last error code cannot tell them apart: the function runs a whole modal loop whose internal calls leave one set on success too. WM_INITMENUPOPUP can, so record that the popup appeared and treat only a menu that never appeared as a failure. Replace the interactive check that came with the fix. It drove the menu with SendInput key strokes, which never reach a menu whose owner is not the foreground window -- Open() then never returns and the test hangs forever instead of failing. tests/menu_click_test.cpp opens a real menu, leaves the mouse to tools/gui/core_menu_backend_test.ps1 in the workspace, and checks that a top-level and a submenu click arrive inside Open() exactly once, and that dismissing fires none.
1 parent 0f84355 commit 4042ca4

4 files changed

Lines changed: 161 additions & 33 deletions

File tree

‎src/platform/windows/menu_windows.cpp‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -502,6 +502,8 @@ class Menu::Impl {
502502
#endif
503503
bool wraps_native_ = false;
504504
bool opening_ = false;
505+
// Set from WM_INITMENUPOPUP: the popup really appeared on screen.
506+
bool shown_ = false;
505507
#ifdef NATIVEAPI_ENABLE_WINUI3
506508
std::shared_ptr<WinUI3MenuSession> modern_session_;
507509
#endif
@@ -535,6 +537,7 @@ class Menu::Impl {
535537

536538
if (popup_menu == hmenu_) {
537539
// This is our menu being opened
540+
shown_ = true;
538541
// Emit menu opened event via callback
539542
if (opened_callback_) {
540543
opened_callback_(id_);
@@ -885,14 +888,15 @@ bool Menu::Open(const PositioningStrategy& strategy, Placement placement) {
885888
// callback outside an isolate". Emitting the click here, before Open()
886889
// returns, keeps every menu event inside the Open() call.
887890
pimpl_->opening_ = true;
888-
SetLastError(ERROR_SUCCESS);
891+
pimpl_->shown_ = false;
889892
const UINT cmd = static_cast<UINT>(TrackPopupMenu(
890893
pimpl_->hmenu_, uFlags | TPM_RETURNCMD, pt.x, pt.y, 0, host_window, nullptr));
891-
// With TPM_RETURNCMD, 0 means "dismissed" or "failed"; only the error code
892-
// tells them apart.
893-
const bool failed = cmd == 0 && GetLastError() != ERROR_SUCCESS;
894894
pimpl_->opening_ = false;
895-
if (failed) {
895+
// With TPM_RETURNCMD, 0 means "dismissed without a pick" or "failed".
896+
// WM_INITMENUPOPUP tells them apart: a menu that never appeared is a failure.
897+
// (The last error code cannot: TrackPopupMenu runs a whole modal loop, whose
898+
// internal calls leave one set on success too.)
899+
if (cmd == 0 && !pimpl_->shown_) {
896900
return false;
897901
}
898902

‎tests/CMakeLists.txt‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,15 @@ add_executable(shortcut_accelerator_test shortcut_accelerator_test.cpp)
2424
target_link_libraries(shortcut_accelerator_test PRIVATE nativeapi)
2525
add_test(NAME shortcut_accelerator_test COMMAND shortcut_accelerator_test)
2626

27+
# Interactive: with a round name it opens a real menu and waits for
28+
# tools/gui/core_menu_backend_test.ps1 to work it with the mouse. Without arguments
29+
# it is a no-op, so CTest can run it.
30+
if(WIN32)
31+
add_executable(menu_click_test menu_click_test.cpp)
32+
target_link_libraries(menu_click_test PRIVATE nativeapi)
33+
add_test(NAME menu_click_test COMMAND menu_click_test)
34+
endif()
35+
2736
if(WIN32)
2837
add_executable(menu_backend_test menu_backend_test.cpp)
2938
target_link_libraries(menu_backend_test PRIVATE nativeapi)

‎tests/menu_backend_test.cpp‎

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -45,34 +45,6 @@ int main(int argc, char**) {
4545
Menu wrapped(CreatePopupMenu());
4646
if (!Check(!wrapped.SetBackend(MenuBackend::WinUI3), "Wrapped HMENU accepted modern backend")) return 1;
4747
if (argc <= 1) return 0;
48-
{
49-
// Native backend: the click must be emitted before Open() returns. A
50-
// posted WM_COMMAND would arrive later from the message loop, where
51-
// bindings with call-scoped callbacks (Dart NativeCallable.isolateLocal)
52-
// cannot receive it.
53-
Menu native_menu;
54-
if (!Check(native_menu.SetBackend(MenuBackend::Native), "Native backend rejected")) return 1;
55-
auto pick = std::make_shared<MenuItem>("Pick me");
56-
native_menu.AddItem(pick);
57-
bool clicked = false;
58-
pick->AddListener<MenuItemClickedEvent>([&](const auto&) { clicked = true; });
59-
auto pick_timer = SetTimer(nullptr, 0, 700, [](HWND, UINT, UINT_PTR t, DWORD) {
60-
KillTimer(nullptr, t);
61-
INPUT keys[4] = {};
62-
for (auto& k : keys) k.type = INPUT_KEYBOARD;
63-
keys[0].ki.wVk = VK_DOWN;
64-
keys[1].ki.wVk = VK_DOWN;
65-
keys[1].ki.dwFlags = KEYEVENTF_KEYUP;
66-
keys[2].ki.wVk = VK_RETURN;
67-
keys[3].ki.wVk = VK_RETURN;
68-
keys[3].ki.dwFlags = KEYEVENTF_KEYUP;
69-
SendInput(4, keys, sizeof(INPUT));
70-
});
71-
const bool native_opened =
72-
pick_timer && native_menu.Open(PositioningStrategy::Absolute({300, 300}));
73-
if (!Check(native_opened, "Native Open failed") ||
74-
!Check(clicked, "Native click was not emitted before Open() returned")) return 1;
75-
}
7648
if (!supported) return 1;
7749
HWND preview = nullptr;
7850
if (argc > 2) {

‎tests/menu_click_test.cpp‎

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
// Menu item clicks must reach their listener before Menu::Open() returns.
2+
//
3+
// The native Windows backend used to let TrackPopupMenu post WM_COMMAND, which the
4+
// host's message loop dispatches after Open() has returned; bindings whose callbacks
5+
// are only valid inside the call (Dart's NativeCallable.isolateLocal) cannot receive
6+
// an event that late.
7+
//
8+
// Without arguments this is a headless no-op, so CTest can run it. With a round name
9+
// it opens a real menu and waits for a driver to work it with the mouse:
10+
// tools/gui/core_menu_backend_test.ps1 in the libnativeapi workspace.
11+
//
12+
// menu_click_test pick_top_level driver clicks "Alpha"
13+
// menu_click_test dismiss driver clicks the window, clear of the menu
14+
// menu_click_test pick_submenu driver opens "More" and clicks "Beta"
15+
//
16+
// The driver creates go.flag in the working directory once it has activated the
17+
// window, so that no activation click can race the menu open.
18+
#include <windows.h>
19+
#include <iostream>
20+
#include <memory>
21+
#include <string>
22+
#include "nativeapi.h"
23+
24+
using namespace nativeapi;
25+
26+
namespace {
27+
28+
int alpha_clicks = 0, beta_clicks = 0, alpha_inside = 0, beta_inside = 0;
29+
int opened = 0, closed = 0;
30+
bool inside_open = false;
31+
int failures = 0;
32+
33+
void Pump(int ms) {
34+
const ULONGLONG end = GetTickCount64() + ms;
35+
MSG msg;
36+
while (GetTickCount64() < end) {
37+
while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) {
38+
TranslateMessage(&msg);
39+
DispatchMessage(&msg);
40+
}
41+
Sleep(10);
42+
}
43+
}
44+
45+
bool Check(bool condition, const std::string& what) {
46+
if (!condition) {
47+
++failures;
48+
std::cout << "FAIL " << what << std::endl;
49+
} else {
50+
std::cout << "PASS " << what << std::endl;
51+
}
52+
return condition;
53+
}
54+
55+
} // namespace
56+
57+
int main(int argc, char** argv) {
58+
if (argc <= 1) return 0; // headless: nothing to drive the menu with
59+
const std::string round = argv[1];
60+
if (round != "pick_top_level" && round != "dismiss" && round != "pick_submenu") {
61+
std::cerr << "unknown round: " << round << '\n';
62+
return 2;
63+
}
64+
65+
Application::GetInstance();
66+
67+
// A window of our own: the menu behaves like it does in an app only when its owner
68+
// is the foreground window, and the driver needs something safe to click on.
69+
WNDCLASSW wc = {};
70+
wc.lpfnWndProc = DefWindowProcW;
71+
wc.hInstance = GetModuleHandleW(nullptr);
72+
wc.hCursor = LoadCursorW(nullptr, reinterpret_cast<LPCWSTR>(IDC_ARROW));
73+
wc.hbrBackground = reinterpret_cast<HBRUSH>(COLOR_WINDOW + 1);
74+
wc.lpszClassName = L"NativeApiMenuClickTest";
75+
RegisterClassW(&wc);
76+
HWND host = CreateWindowExW(0, wc.lpszClassName, L"nativeapi menu click test",
77+
WS_OVERLAPPEDWINDOW | WS_VISIBLE, 200, 200, 700, 520, nullptr,
78+
nullptr, wc.hInstance, nullptr);
79+
if (!host) {
80+
std::cerr << "could not create the test window\n";
81+
return 2;
82+
}
83+
std::cout << "READY" << std::endl;
84+
85+
Menu menu;
86+
if (!Check(menu.SetBackend(MenuBackend::Native), "native backend selected")) return 1;
87+
auto alpha = std::make_shared<MenuItem>("Alpha");
88+
menu.AddItem(alpha);
89+
auto submenu = std::make_shared<Menu>();
90+
auto beta = std::make_shared<MenuItem>("Beta");
91+
submenu->AddItem(beta);
92+
auto more = std::make_shared<MenuItem>("More", MenuItemType::Submenu);
93+
more->SetSubmenu(submenu);
94+
menu.AddItem(more);
95+
96+
alpha->AddListener<MenuItemClickedEvent>([&](const MenuItemClickedEvent&) {
97+
++alpha_clicks;
98+
if (inside_open) ++alpha_inside;
99+
});
100+
beta->AddListener<MenuItemClickedEvent>([&](const MenuItemClickedEvent&) {
101+
++beta_clicks;
102+
if (inside_open) ++beta_inside;
103+
});
104+
menu.AddListener<MenuOpenedEvent>([&](const MenuOpenedEvent&) { ++opened; });
105+
menu.AddListener<MenuClosedEvent>([&](const MenuClosedEvent&) { ++closed; });
106+
107+
// Wait for the driver's go flag; it activates the window first.
108+
const ULONGLONG deadline = GetTickCount64() + 60000;
109+
while (GetFileAttributesA("go.flag") == INVALID_FILE_ATTRIBUTES &&
110+
GetTickCount64() < deadline) {
111+
Pump(200);
112+
}
113+
Pump(300);
114+
const bool foreground = GetForegroundWindow() == host;
115+
std::cout << "GO foreground=" << foreground << std::endl;
116+
// A popup menu whose owner is not the foreground window ignores mouse input and
117+
// never closes, which would hang Open(): say so rather than time out silently.
118+
if (!Check(foreground, "test window is in the foreground")) return 1;
119+
120+
inside_open = true;
121+
const bool returned = menu.Open(PositioningStrategy::Absolute({300, 300}));
122+
inside_open = false;
123+
124+
Check(returned, "Open() reported success");
125+
Check(opened == 1 && closed == 1, "one opened and one closed event");
126+
if (round == "pick_top_level") {
127+
Check(alpha_inside == 1, "the click reached the listener before Open() returned");
128+
Check(beta_clicks == 0, "no other item fired");
129+
} else if (round == "pick_submenu") {
130+
Check(beta_inside == 1, "the submenu click reached the listener before Open() returned");
131+
Check(alpha_clicks == 0, "no other item fired");
132+
} else {
133+
Check(alpha_clicks == 0 && beta_clicks == 0, "dismissing fires no click");
134+
}
135+
136+
const int before = alpha_clicks + beta_clicks;
137+
Pump(1500); // a posted WM_COMMAND would be dispatched in here
138+
Check(alpha_clicks + beta_clicks == before, "no second click arrives after Open() returned");
139+
140+
DestroyWindow(host);
141+
std::cout << (failures ? "FAILED" : "OK") << std::endl;
142+
return failures ? 1 : 0;
143+
}

0 commit comments

Comments
 (0)