Menubar: clicking menu's label when it's open doesn't close

[vslavik]

From the department of nitpicky-but-user-noticeable differences from the native menubar, @rillig found another difference, originally reported at https://github.com/vslavik/poedit/issues/341. Quoting with slight rephrasing and minor additions:

To reproduce:

1. Click a menu in the menubar (e.g. File); this opens the menu under it
2. Click again on highlighted menu item in the menubar
3. The menu closes and immediately re-opens with a quick flicker, i.e. is again open. In all other applications (I tried Notepad, Thunderbird, eventvwr.exe) the menu is closed on the second click. The other applications differ in whether the menu item has mouseover highlighting applied (native does, e.g. MSVS2015 doesn't), but they all close the menu and drop the keyboard focus.

@rillig furthermore says:

1. Click on the File menu
2. Doubleclick on the File menu
3. Now the menu closes.

(In Notepad, this makes the menu open, because it is treated as two subsequent single clicks.)

I didn't look into trying to fix it yet, but will in a few weeks if you don't beat me to it (too busy writing new bugs for Poedit right now…).

[mity]

Should be fixed.

Thanks for reporting.

[vslavik]

I still can see the bug when testing with e2b670aed5d87a43647cc549adcff82a20a079fd — it behaves differently, but still wrongly:

In Notepad with native menu:

1. Click File twice slowly (i.e. with big enough pause to not count as double click): menu is shown and then hidden
2. Click File twice quickly (i.e. doubleclick): menu flickers and stays open

With the latest mCtrl it's the opposite:

1. Click File twice slowly, it flickers and stays open
2. Click File twice quickly, it's shown and hidden

(Windows 10)

[mity]

Originally I tested with mCtrl's example-menubar.exe and there (on my machine) it works the same as in Notepad.exe.

I now tested with the poedit.exe and I can reproduce what you see.

Ouch. Will need some investigation what's happening differently between the two.

[mity]

This is how it is supposed to work (and how it seems to work in native win32 app like example-menubar.exe):

1. When a dropdown menu is present (via TrackPopupMenuEx()), it has its own (modal/nested) message loop.

2. Its feature is that when user clicks (WM_MOUSEDOWN) outside of the popup menu, it is closed/canceled (i.e. TrackPopupMenuEx() returns) but the message itself causing that is not consumed. (And that's ok. It's same with window native menubar. Even when it is opened, and you click e.g. in another app, the menu is closed but the click itself is recognized in the other app.)

3. The commit e2b670a attempts to fix the issue by this: A. We maintain a flag "has dropdown". B. We set it just before calling TrackPopupMenuEx(). C. But instead of resetting just after it returns, we post a message to end of thread's message queue (and the flag shall be reset later in the handler of the message when it eventually comes). D. In code where we normally start the dropdown menu, we check the flag and if it is already set, we don't do anything.

The point C is crucial. It means then any messages enqueued before our helper message shall be ignored for purpose of opening a dropdown in the menubar.

However in Poedit there is some reversion of the order how the messages are processed. I verified that for the 2nd click, the message resetting the flag (TB_SETSTATE) comes to the control BEFORE the TBN_DROPDOWN which notifies us the user clicked on the menubar item.

Why this inversion happens in poedit is unclear to me. I just took some very brief look into sources of wxWidgets, and all I know so far, that the message processing (wxwidgets/src/msw/evtloop.cpp) seems to be quite complicated so I am currently quite suspicious in that direction.

[vslavik]

On one hand, you are right, this is due to wxWidgets' event loop.

On the other hand, nothing wx does is unreasonable and despite evtloop.cpp size, it boils down to almost the classical win32 get-translate-dispatch event loop, with one exception: it's using PeekMessage(&msg, 0, 0, 0, PM_NOREMOVE); too. This is not unreasonable in a larger application that may need to do some processing in idle time (as even MSDN mentions) or handle non-win32 async I/O; and this call really ought to be a no-op.

After spending most of today debugging the event loop, I was able to convince myself that it's not something weird that wx is doing and reduced it to this minimal change to the example that reproduces the behavior in Poedit/wx:

diff --git a/examples/example-menubar.c b/examples/example-menubar.c
index 8605af3..aba4200 100644
--- a/examples/example-menubar.c
+++ b/examples/example-menubar.c
@@ -217,11 +217,14 @@ _tWinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPTSTR lpCmdLine, int nC
             continue;
         if(mcIsMenubarMessage(hwndMenubarSmall, &msg))
             continue;
-        if(IsDialogMessage(hWnd, &msg))
-            continue;

         TranslateMessage(&msg);
         DispatchMessage(&msg);
+
+        PeekMessage(&msg, 0, 0, 0, PM_NOREMOVE);
     }

     mcMenubar_Terminate();

Note that it really is minimal: both calling PeekMessage and not calling IsDialogMessage is required. The latter is reasonable too: the latter is explicitly documented as for modal dialogs, with the possibility, not requirement, to use it elsewhere.

At this point, my only ideas are to either use WM_TIMER to reset the state after a small delay or pump and clear the event queue after closing the menu, but both fall squarely into the category of "hiding the underlying problem" that you rightly dislike.

[mity]

Big thanks for the minimized code showing the issue. That really helps a lot.

According to MSDN, PeekMessage(&msg, 0, 0, 0, PM_NOREMOVE) gets any posted message before any input messages. And the code is likely a proof that it also changes the order of the messages in the queue to follow the same condition, and this seems to be the source of message order inversion.

I was never aware that PeekMessage() can have such effect. Even worse, with different arguments (especially wMsgFilterMin, wMsgFilterMax) the reordering may likely be completely different, so we cannot make any guaranty about order of the messages we post into the queue. What a mess it is.

I'll try to think what I can do with that. If I cannot find anything better, I will likely use the timer.

[mity]

New attempt. Works for me both with poedit as well as the example-menubar.exe (both with and without the PeekMessage())

[vslavik]

Great, thanks a lot!