fix(gui): raise the context menu on Shift+F10 for VoiceOver users - #877
Conversation
wx's Cocoa backend only ever fires wxEVT_DATAVIEW_ITEM_CONTEXT_MENU in response to a physical right-click. There is no keyboard-triggered path and no AXShowMenu implementation for any control on this port (wxWidgets/wxWidgets#13010, open since 2011, not specific to wxDataViewCtrl). VoiceOver's own context-menu gesture (VO+Shift+M) performs AXShowMenu, so it never reaches wx at all -- reported by a blind user for every wxDataViewCtrl-backed list in aMule (amule-org#180). Add a Shift+F10 handler to CMuleDataViewCtrl::OnKeyDown (the base class every list -- Downloads, Search, Shared Files, Friends, Clients, Servers -- derives from) that raises the same context-menu event ourselves, reusing every list's existing OnItemRightClicked/OnRightClick handler unchanged. Ctrl+Return was tried first, since Shift+F10 sits on the function row some users avoid, but NSOutlineView's own keyDown: treats bare Return as "activate the row" before this handler ever sees it, Control held or not -- confirmed by it opening a shared file in its associated app instead of showing a menu, so it can never fire here on this port. Verified against a live build: sent Shift+F10 via a real system key event (not just an accessibility action) to a selected row in Shared Files, screenshotted the result, and confirmed the correct list-specific context menu opened with all its items. Ctrl+Return's rejection is likewise from a live repro, not a guess. Local clang-tidy CI replica (Tier-1 whole-tree + Tier-2 changed-lines) reports nothing on this file; the Tier-1 hits that do exist are pre-existing, in unittests/ files this change never touches.
got3nks
left a comment
got3nks
left a comment
There was a problem hiding this comment.
Confirmed the macOS diagnosis: wxEVT_DATAVIEW_ITEM_CONTEXT_MENU has no keyboard path on the Cocoa port, and GetCurrentItem() / GetItemRect() are both implemented there, so the approach works.
1. The gap isn't macOS-only — Windows and Linux are equally affected.
wxEVT_DATAVIEW_ITEM_CONTEXT_MENU is mouse-only on every backend: generic/datavgen.cpp:5077 raises it under if (event.RightUp()) (that is the backend MSW uses), and gtk/dataview.cpp:4754 inside the button-press handler. Since all six lists bind only that event, keyboard users have no route to these context menus on any platform.
The difference is that Windows and GTK already deliver a portable event that we ignore. Measured with a minimal wxDataViewCtrl probe on both:
Windows (wx 3.3.2, MSW): Shift+F10 -> wxEVT_CONTEXT_MENU pos=(-1,-1)
Ubuntu (wx 3.2.9, GTK3): Shift+F10 -> wxEVT_CONTEXT_MENU pos=(-1,-1)
both: right-click -> wxEVT_DATAVIEW_ITEM_CONTEXT_MENU pos=(64,72)
pos=(-1,-1) is wx's keyboard-origin marker. On MSW it comes from WM_CONTEXTMENU (msw/window.cpp:3645), which the OS also sends for the Applications key.
Fix: bind wxEVT_CONTEXT_MENU in CMuleDataViewCtrl and raise wxEVT_DATAVIEW_ITEM_CONTEXT_MENU at the current item from there. That covers Windows and Linux with less code than the macOS path needs, and the #ifdef __WXOSX__ Shift+F10 handler then becomes the shim for the one port where nothing fires — feeding the same handler, so all three platforms end up on the same key.
2. The menu opens at the mouse pointer, not at the focused row.
All six handlers call PopupMenu(menu) with no position, which uses the cursor — as the comment at MuleNotebook.cpp:186 already notes. That is correct for a right-click, and wrong for a keyboard user, whose pointer may be sitting on another window or another display.
Fix: wxDataViewEvent carries a position, so set it when synthesising:
const wxDataViewItem item = GetCurrentItem();
wxDataViewEvent menuEvent(wxEVT_DATAVIEW_ITEM_CONTEXT_MENU, this, item);
const wxRect rect = item.IsOk() ? GetItemRect(item) : wxRect(GetClientSize());
menuEvent.SetPosition(rect.GetLeft(), rect.GetBottom());and pass event.GetPosition() through to PopupMenu in the handlers, as ChatWnd.cpp:114 and TransferWnd.cpp:393 already do. ServerWnd.cpp:497 has the same keyboard-origin fallback for its info list.
3. Minor: the 14-line comment is longer than the code it explains and largely restates the PR description. The Ctrl+Return rejection is the part worth keeping in-tree, since it stops the next person retrying it; the rest reads better condensed.
…w position got3nks' review on amule-org#877 found the Shift+F10 fix was scoped too narrowly to macOS. wxEVT_DATAVIEW_ITEM_CONTEXT_MENU is mouse-only on every backend, not just Cocoa: MSW and GTK already deliver a portable wxEVT_CONTEXT_MENU for Shift+F10/the Applications key (measured directly: pos=(-1,-1), wx's own keyboard-origin marker), which none of the six lists were listening for. CMuleDataViewCtrl now binds EVT_CONTEXT_MENU and, for the keyboard-origin case only (evt.GetPosition() == wxDefaultPosition; a real right-click's own wxEVT_DATAVIEW_ITEM_CONTEXT_MENU already fires and must not be duplicated), synthesises the item context-menu event itself. The macOS Shift+F10 handler in OnKeyDown now shares that same synthesis helper (RaiseItemContextMenu), since Cocoa has no equivalent portable event at all. Also fixes the menu popping up at the mouse cursor instead of the focused row for a keyboard-triggered open (wrong when the pointer is on another window/display): RaiseItemContextMenu positions the synthesized event at the current item's rect, and all six list handlers now pass their event's position through to PopupMenu() instead of leaving it at the default (which uses the current cursor position), matching the pattern already used in ChatWnd.cpp/TransferWnd.cpp/ServerWnd.cpp. Verified: `cmake --build . --target amule` clean before and after clang-format; a live GTK/MSW probe isn't available locally, so the portable event's existence and keyboard-origin marker are taken from got3nks' measurements quoted in the PR review, not independently re-verified here.
|
Thanks for the thorough review -- all three addressed:
Verified: I don't have a Windows/Linux box to re-measure the |
…ed away Two things from the second review round, both in the keyboard path. GetItemRect() answers an empty rect for a row that is not currently on screen -- the same limitation MoveByPage() documents a few lines above, which is why it uses GetCountPerPage() instead. RaiseItemContextMenu() took that rect at face value, so a cursor scrolled out of view put the menu in the control's top-left corner rather than at the row it acts on. Reachable by clicking a row and scrolling away before pressing the key. The row is brought into view first, which also shows the user what the menu is about to apply to, and does nothing when it is already visible. And the comment on the Cocoa branch said that port has no wxEVT_CONTEXT_MENU handling at all. It does: wxDataViewCtrl:: OnContextMenu() in osx/dataview_osx.cpp is exactly how a right-click becomes a dataview event there. What macOS lacks is any keystroke that raises that event. The distinction matters, because it makes the Skip() in OnContextMenuKey load-bearing on that port rather than tidy -- our handler runs first, being the derived class, so swallowing a mouse-origin event would leave right-click with no menu at all. Both comments now say so.
|
Re-reviewed the update — all three points are addressed, and I verified the parts that could bite rather than reading the diff alone, since you flagged you couldn't re-measure Windows and Linux yourself. The
No double-menu risk either: neither I've pushed a follow-up commit (445d586) with two small things rather than sending you round again:
Building on all three platforms now and testing before merge — macOS is already clean with the follow-up; Linux and Windows are rebuilding onto it. @nickinesio, I'll post a build once that's done, if you'd like to confirm Shift+F10 under VoiceOver on the real thing. |
2 participants
Summary
Second half of #180: with VoiceOver running, none of aMule's
wxDataViewCtrl-backed lists (Downloads, Search Results, Shared Files, Friends, Clients, Servers) could open their context menu via the keyboard. VO+Shift+M (VoiceOver's context-menu gesture) performsAXShowMenu, but wx's Cocoa backend only ever raiseswxEVT_DATAVIEW_ITEM_CONTEXT_MENUin response to a physical right-click -- there is no keyboard-triggered path and noAXShowMenuimplementation for any control on this port at all, not specific towxDataViewCtrl(tracked upstream since 2011: wxWidgets/wxWidgets#13010). So the gesture never reaches wx, let alone aMule.As discussed on #180, this is fixable on aMule's side without waiting on wx: nothing stops us from handling a keystroke ourselves and raising the same context-menu event
PopupMenu()-based handlers already listen for.Change
One handler in
CMuleDataViewCtrl::OnKeyDown-- the base class every affected list derives from, so this fixes all six in one place -- that raiseswxEVT_DATAVIEW_ITEM_CONTEXT_MENUon Shift+F10, targeting the current item. Every list's existingOnItemRightClicked/OnRightClickhandler picks it up unchanged: they already tolerate the event's item not matching the current selection (falling back to whatever's already selected), which is exactly what a keyboard-triggered menu needs.Ctrl+Return was tried first (the alternative suggested on #180, for users who avoid the function row) and rejected:
NSOutlineView's ownkeyDown:treats bare Return as "activate the row" before this handler ever sees it, Control held or not. Confirmed live -- it opened a shared file in its associated app instead of showing a menu. Shift+F10 has no such native meaning for an outline view and fires correctly.Testing
Verified against a live build with a real system key event (System Events
key code 109 using shift down, not just an accessibility action) sent to a selected row in Shared Files: the correct context menu opened with every expected item (screenshot taken during development). Ctrl+Return's rejection is from an equally live repro, not a guess.Local clang-tidy CI replica (Tier-1 whole-tree + Tier-2 changed-lines vs.
upstream/master): zero hits on this file; the Tier-1 hits that exist elsewhere are pre-existing, inunittests/files this change never touches.Fixes the context-menu half of #180. The other half (custom-renderer cells reading as
<wxCustomRendererObject: ...>) is a separate, already-filed upstream wx issue/PR (wxWidgets/wxWidgets#26808, wxWidgets/wxWidgets#26809).Test plan