Opened 3 years ago

Last modified 10 days ago

#13683 accepted defect

MSW: Regression with wxEVT_CONTEXT_MENU generation

Reported by: daumling Owned by: vadz
Priority: normal Milestone: 3.1.0
Component: wxMSW Version: stable-latest
Keywords: regression Cc:
Blocked By: Blocking:
Patch: no

Description

Currently, only the frame window message handler handles a WM_MENUSELECT message. This makes it difficult for popup menus on other windows to display help text. IMHO, the generic window message handler should handle this event as well so any window can have a handler for the wxEVT_MENU_HIGHLIGHT handler.

A workaround is possible. Have the main frame display the context menu at the right position, but bind the wxEVT_COMMAND_MENU_SELECTED event to the owning window (the one that opened the popup menu) before so the owning window can handle the commands. In that case, the owning window must also handle message bubbling, though.

Attachments (3)

13683_test_minimal.diff download (2.4 KB) - added by catalin 20 months ago.
13683_eat_Highlight_events.diff download (777 bytes) - added by catalin 20 months ago.
13683_fix_multiple_ContextMenuEv.diff download (671 bytes) - added by catalin 20 months ago.

Download all attachments as: .zip

Change History (21)

comment:1 follow-up: Changed 3 years ago by vadz

  • Keywords simple added
  • Status changed from new to confirmed
  • Summary changed from MSW: WM_MENUSELECT should have a general event handler to handle popup menus to MSW: wxEVT_MENU_HIGHLIGHTshould be sent to the menu owner, not parent frame

If WM_MENUSELECT is already sent to the right window it should be enough to move the code from wxFrame down to wxWindow. If not we can still make this work as expected by checking for wxCurrentPopupMenu in wxFrame::HandleMenuSelect().

IOW this shouldn't be difficult to do and should actually make wxMSW work more consistently with the other ports as I think they do send this event to the popup menu owner already. If anybody can test and contribute a patch doing this, it would be welcome.

Changed 20 months ago by catalin

Changed 20 months ago by catalin

Changed 20 months ago by catalin

comment:2 in reply to: ↑ 1 ; follow-up: Changed 20 months ago by catalin

  • Priority changed from low to normal
  • Version set to 2.9-svn

Replying to vadz:

If WM_MENUSELECT is already sent to the right window it should be enough to move the code from wxFrame down to wxWindow.

AFAICS WM_MENUSELECT is sent to the TLW only. (And the same about the other wxEVT_MENU_* events.)
Would it be ok to move all the code in #if wxUSE_MENUS sections of src/msw/toplevel.cpp to src/msw/window.cpp ?

Other things noticed:
1) When a menu is closed, a useless WM_MENUSELECT / wxEVT_MENU_HIGHLIGHT event is sent.
2) A skipped wxContextMenuEvent is processed twice.
Attached are 2 small patches that attempt to fix these issues.

I've changed the Priority to Normal because of the issue number 2) above.

comment:3 in reply to: ↑ 2 Changed 20 months ago by vadz

  • Milestone set to 2.9.5

Replying to catalin:

Replying to vadz:

If WM_MENUSELECT is already sent to the right window it should be enough to move the code from wxFrame down to wxWindow.

AFAICS WM_MENUSELECT is sent to the TLW only. (And the same about the other wxEVT_MENU_* events.)
Would it be ok to move all the code in #if wxUSE_MENUS sections of src/msw/toplevel.cpp to src/msw/window.cpp ?

Sorry, I don't see how would this help? Shouldn't we rather check for wxCurrentPopupMenu in src/msw/toplevel.cpp code?

Other things noticed:
1) When a menu is closed, a useless WM_MENUSELECT / wxEVT_MENU_HIGHLIGHT event is sent.
2) A skipped wxContextMenuEvent is processed twice.
Attached are 2 small patches that attempt to fix these issues.

Thanks for these fixes, I'll apply them soon.

comment:4 Changed 20 months ago by catalin

Oh, sorry, WM_MENUSELECT, WM_INITMENUPOPUP and WM_UNINITMENUPOPUP are sent to the right window but are handled by wxTopLevelWindowMSW::MSWWindowProc() first and never make it to wxWindowMSW::MSWHandleMessage.
So moving the code to window.cpp should fix this.

comment:5 follow-up: Changed 20 months ago by vadz

Ah, I see. Unfortunately I'm not really sure what would moving this code do, i.e. if we're not going to break something else in the event propagation logic. Testing for the current popup menu in the TLW code still seems like a simpler change to me.

Also, I have another question about (1) from comment:2: are we sure this event is really useless, isn't it used by wxFrame to remove the menu help string from the status bar? Does this still work correctly after applying this?

comment:6 Changed 20 months ago by VZ

(In [73949]) Fix duplicate wxContextMenuEvent generation in wxMSW.

Prevent WM_CONTEXTMENU from being propagated upwards the window parent chain
by DefWindowProc(), we already do it ourselves and not marking the message as
processed could result in multiple calls to the same wxEVT_CONTEXT_MENU
handler if it skipped the event.

See #13683.

comment:7 in reply to: ↑ 5 Changed 20 months ago by catalin

Replying to vadz:

Testing for the current popup menu in the TLW code still seems like a simpler change to me.

I don't know how this check is supposed to work. There are such checks in wxTopLevelWindowMSW::HandleMenuPopup() for example. Based on that should the code search for the menu owner and call HandleWindowEvent() for it?

Also, I have another question about (1) from comment:2: are we sure this event is really useless, isn't it used by wxFrame to remove the menu help string from the status bar? Does this still work correctly after applying this?

From what I can see the short answer is yes.

On Win7 I see this:

  • when using a menu attached to a MenuBar, there will always be a last Highlight event for that menu, whether a menu option was chosen, or Esc was pressed, or the user clicked somewhere else after opening the menu [and highlighting an option]; a menu has no help string so the status bar message is reset;
  • when using a context menu the help string is never displayed (a bug?), and there is nothing to reset.

To be on the safe side maybe that extra event should still be sent?

comment:8 Changed 19 months ago by vadz

  • Milestone 2.9.5 deleted

[sorry for the delay]

To answer the first question, I think that checking for wxCurrentPopupMenu->GetHMenu() == hMenu in wxTopLevelWindowMSW::HandleMenuSelect() would indeed be the least intrusive way of fixing the problem.

For the second one, I agree that sending this extra event seems safer.

Thanks!

comment:9 Changed 18 months ago by plorkyeran

r73949 appears to have broken the default context menu for wxTextCtrl (and possibly other things). wxEVT_CONTEXT_MENU handlers still get called, but even if the event is skipped (or there is no handler bound at all) the default menu never gets displayed. Reverting that commit makes it work for me.

comment:10 Changed 18 months ago by vadz

Thanks for noticing this, will fix soon.

comment:11 Changed 18 months ago by VZ

(In [74329]) Better fix for duplicate wxContextMenuEvent generation under MSW.

Fix the bug with multiple wxContextMenuEvent being generated for a single
WM_CONTEXTMENU without breaking context menus for wxTextCtrl (and all the
other native controls). Do this by ensuring that WM_CONTEXTMENU is still
passed to DefWindowProc() if we don't process it instead of just being eaten
completely in any case.

Also add a unit test checking for this bug to ensure it stays fixed.

See #13683.

comment:12 Changed 2 weeks ago by Chuddah

changeset:74329 has broken context menu events for TreeCtrl on win32 (Tested on Windows 7).

The original code (before changeset:73949) received the WM_CONTEXTMENU and, knowing the win32 api may have decided it should be handled by an ancestor widget, forwarded the event to the originating child if that was required.

This event could then be propogated up the UI ancestors until it is handled. Removing this prevents event handlers, bound to any [recursive] child of the window instance, being called.

The result is. For treectrl's (possibly other widgets) the tree item context events still work although the context menu for the "empty space" of the control does not.

Damien

Last edited 2 weeks ago by Chuddah (previous) (diff)

comment:13 Changed 2 weeks ago by Chuddah

I have seen the OS propagating events when wxWidgets would prefer it not to:

The comments in changeset:73949 also indicate WM_HELP also gets propagated by DefWindowProc() also.

In ticket:15802 There was a similar issue to gtk propagating key events when wxWidgets preferred it not to. This was solved with (sorry) global statics.

Is there a way to isolate an ignore-os-event-propagation as this is appearing to be a common occurance?

Damien

comment:14 Changed 2 weeks ago by vadz

  • Keywords regression added; simple removed
  • Milestone set to 3.1.0
  • Owner set to vadz
  • Status changed from confirmed to accepted
  • Summary changed from MSW: wxEVT_MENU_HIGHLIGHTshould be sent to the menu owner, not parent frame to MSW: Regression with wxEVT_CONTEXT_MENU generation

Right now I don't see any good solution here :-(

The least worst seems to be reverting all the changes done here so far and living with duplicate events -- it's better than none at all. But far from ideal, of course...

Any ideas would be warmly welcome.

comment:15 Changed 10 days ago by Chuddah

I am trying to investigate this problem but I cannot reproduce the multiple context menu events.
Any suggestions of how to reproduce?

Damien,

comment:16 Changed 10 days ago by vadz

Do you mean the first patch attached to this ticket doesn't show the problem even if you undo all the commits which tried to fix this?

comment:17 Changed 10 days ago by Chuddah

I was trying to reproduce the bug using wxWidgets 2.9.
Is this bug new to 3.0?

The double-events bug doesnt occur neither if I revert the WM_CONTEXTMENU switch handler back to the wxWidgets2.9 implemenation:

src/msw/window.cpp

         case WM_CONTEXTMENU:
                // we don't convert from screen to client coordinates as
                // the event may be handled by a parent window
                wxPoint pt(GET_X_LPARAM(lParam), GET_Y_LPARAM(lParam));

                wxContextMenuEvent evtCtx(wxEVT_CONTEXT_MENU, GetId(), pt);

                // we could have got an event from our child, reflect it back
                // to it if this is the case
                wxWindowMSW *win = NULL;
                WXHWND hWnd = (WXHWND)wParam;
                if ( hWnd != m_hWnd )
                {
                    win = FindItemByHWND(hWnd);
                }

                if ( !win )
                    win = this;

                evtCtx.SetEventObject(win);
                processed = win->HandleWindowEvent(evtCtx);

Briefly, I have tried under 2.9 and 3.0 (without above patches) and 3.0 (with the patch in this message)...

I am seeing no *double-events* with any of these versions...
wx3.0 only works for context menus with the wx2.9 reversion patch (in this message)

comment:18 Changed 10 days ago by vadz

Strange, I don't remember the details but I think I did see this bug myself before applying the fix. Catalin, do you remember more by chance?

Note: See TracTickets for help on using tickets.