Bugzilla – Bug 1639
Panel navigation keys don't work in docked toolbars, if bound to some action.
Last modified: 2018-08-20 11:45:26 UTC
This problem is caused by the new key bindings for Left and Right arrow, which capture key presses that are intended to go to wxWidgets. If the keys left and right are not bound, then left and right arrows in docked toolbars behave correctly. Note that NumericText controls that we draw such as selection start do behave correctly, as we handle left and right key presses. The problem of left and right arrow being captured also affects sliders, drop downs and radio buttons of docked toolbars.
Further investigation: Disabling track cursor left/right unless TrackHasFocus does not solve the problem. Having found a bound command, even though disabled, Audacity does not forward the keypress on to wxWidgets. Besides this would be the 'wrong' solution, as any other command could have been bound to left or right. We actually want these keys to take priority over Audacity key bindings, if in a control which uses them. [Or alternatively, not to bind or provide these keys for binding]
***STEPS UPDATED*** The original bug is DEVEL-FIXMADE "Left and Right arrow keys no longer work in docked toolbars" By: https://github.com/audacity/audacity/commit/5088881a69d18e2219fa6151f3860d77b36a3614 Arguably that is only a workaround. To check this fix, use a clean config to ensure you are using the revised key bindings, and skip step 1 in the revised steps (indeed, you could check that Left and Right are not bound shortcuts) I've changed the title to: "Left and Right arrow keys don't work in docked toolbars, if bound to some action." and updated the steps, release notes and priority.
(In reply to James Crook from comment #0) > This problem is caused by the new key bindings for Left and Right arrow, > which capture key presses that are intended to go to wxWidgets. I don't fully understand. Could you point please to the commit that changed this in the first place. It happened after David reported bug 1630. (In reply to James Crook from comment #2) > https://github.com/audacity/audacity/commit/5088881a69d18e2219fa6151f3860d77b36a3614 > Arguably that is only a workaround. I think arguably the workaround is if anything worse than the original bug. The workaround will affect more users than the original bug and would IMO require a new bug to be opened. Surely we must be able to use quick, easy to hit, unmodified Left/Right to nudge the editing cursor 1 px or to the Snap To position (don't VI users as well as sighted want to do that)? Changing long established shortcuts is to be avoided if possible. I know sighted users want to jump the playback cursor quickly and easily. They can use single key "." and "," but this fix shuts off the change I am lobbying for to let unmodified Left/Right nudge the editing cursor during playback (currently impossible with the keyboard). With the fix as it is, on Windows there is a "Disallowed" message hitting Enter on a focused button in a docked toolbar, which message you have to OK before the action occurs. Try Tools, Edit or Transport Toolbar for example to see this. "Disallowed" also occurs using Up or Down arrow in a focused dropdown or focused slider in a docked toolbar. Left/Right works, but Up/Down previously worked (and is expected to) on Windows.
> This problem is caused by the new key bindings for Left and Right arrow, > which capture key presses that are intended to go to wxWidgets. I don't actually know which commit caused this problem, but I do know that it is the capture of these keys by our command handling that is the issue. If I unbind left and right, then left and right work correctly in the Selection Toolbar, including on the end/length radio buttons. It is possible that the Ext-Menus are providing an additional way to trigger the Left and Right shortcuts that did not exist before, when they did not appear in menus, but I need to investigate that possibility further. Our code seems to test for our bindings first, before giving wxWidgets a go. Please note that 'Enter' is not mentioned in the bug title or steps to reproduce. Possibly we should close this bug as 'Duplicate' and continue with bug 1637 which is more general. Essentially by opening this bug I was attempting to show that the theming colour change (from DA) was in the clear, now, in light and classic theme, though it was previously a problem. Note that DA I released last August has the end/length problem, because of the colouring changes, but does not have this left-right problem in the project rate field.
(In reply to James Crook from comment #4) >> This problem is caused by the new key bindings for Left and Right arrow, >> which capture key presses that are intended to go to wxWidgets. > I don't actually know which commit caused this problem, but I do know that it is > the capture of these keys by our command handling that is the issue. If I unbind > left and right, then left and right work correctly in the Selection Toolbar, including > on the end/length radio buttons. > > It is possible that the Ext-Menus are providing an additional way to trigger the > Left and Right shortcuts that did not exist before, when they did not appear in > menus, but I need to investigate that possibility further. > > Our code seems to test for our bindings first, before giving wxWidgets a go. OK. Thank you. > Please note that 'Enter' is not mentioned in the bug title or steps to reproduce. I know, but I did not create this bug report. I am looking at the (currently) P1 bug 1637. I was surprised by the split from 1637 to a new bug because I understood there was likely to be a common cause for all four steps listed in 1637. > Possibly we should close this bug as 'Duplicate' and continue with bug 1637 > which is more general. Essentially by opening this bug I was attempting to > show that the theming colour change (from DA) was in the clear, now, in > light and classic theme, though it was previously a problem. Note that DA > I released last August has the end/length problem, because of the colouring > changes, but does not have this left-right problem in the project rate field. I think it might be better to close this as Duplicate of 1637 until/unless we actually get left with one or two steps from 1637 as unfixed/hard to fix. If you want to proceed with 1637 from the starting point of mapping Left/Right to Ctrl modified Left/Right, I feel that needs a new bug (as discussed above). 1630 has now become just about that theming problem on those two radio buttons in Selection Toolbar. I assume 1630 will close in due course.
OK. On basis of Comment #5 marking this as a duplicate. I don't think we need a separate bug for reverting the commit in comment #2. We can do it as part of bug 1637. The comment #2 commit helps whilst we are in development, to a very small extent, in that bug 1630 confuses the issue less. *** This bug has been marked as a duplicate of bug 1637 ***
(In reply to James Crook from comment #4) > > This problem is caused by the new key bindings for Left and Right arrow, > > which capture key presses that are intended to go to wxWidgets. > > I don't actually know which commit caused this problem, it looks like it was 031f841. David. but I do know that > it is the capture of these keys by our command handling that is the issue. > If I unbind left and right, then left and right work correctly in the > Selection Toolbar, including on the end/length radio buttons. > > It is possible that the Ext-Menus are providing an additional way to trigger > the Left and Right shortcuts that did not exist before, when they did not > appear in menus, but I need to investigate that possibility further. > > Our code seems to test for our bindings first, before giving wxWidgets a go. > > Please note that 'Enter' is not mentioned in the bug title or steps to > reproduce. > > Possibly we should close this bug as 'Duplicate' and continue with bug 1637 > which is more general. Essentially by opening this bug I was attempting to > show that the theming colour change (from DA) was in the clear, now, in > light and classic theme, though it was previously a problem. Note that DA I > released last August has the end/length problem, because of the colouring > changes, but does not have this left-right problem in the project rate field.
> it looks like it was 031f841. Thanks. That's very helpful to progressing this.
Further information: Even before that commit, having an item in the menu (such as Zoom-In or Zoom-Out) bound to Left or Right is a problem. If those shortcuts are bound and visible in a menu, then they takes precedence over left and right controlling navigation in the Toolbar. Further, there is also an inconsistency between in-dock and not. We can use safe keyboard shortcuts like SPACE and R when in an in-dock toolbar. As soon as the toolbar is a floating toolbar, we can't. That 'inconsistency' is probably valuable to VI users who have toolbars docked, as it means they can do more via keyboard when a toolbar has focus.
(In reply to James Crook from comment #9) > Further information: > > Even before that commit, having an item in the menu (such as Zoom-In or > Zoom-Out) bound to Left or Right is a problem. If those shortcuts are bound > and visible in a menu, then they takes precedence over left and right > controlling navigation in the Toolbar. So a straightforward solution is not to have commands in the menus which have shortcuts which are standard keystrokes used for interacting with controls or navigating toolbars. (There is one case of this in Audacity 2.1.3, space for play/start. This means that buttons have to be pressed using enter rather than enter or spacebar, but this situation is fine.) > > Further, there is also an inconsistency between in-dock and not. We can use > safe keyboard shortcuts like SPACE and R when in an in-dock toolbar. As > soon as the toolbar is a floating toolbar, we can't. > > That 'inconsistency' is probably valuable to VI users who have toolbars > docked, as it means they can do more via keyboard when a toolbar has focus.
(In reply to James Crook from comment #9) > Further information: > > Even before that commit, having an item in the menu (such as Zoom-In or > Zoom-Out) bound to Left or Right is a problem. If those shortcuts are bound > and visible in a menu, then they takes precedence over left and right > controlling navigation in the Toolbar. my understanding is that in CommandManagerEventMonitor::FilterEvent(wxEvent& event) this is the code that can stop keystrokes going to the CommandManager: // Give the capture handler first dibs at the event. wxWindow *handler = project->GetKeyboardCaptureHandler(); if (handler && HandleCapture(handler, key)) { return Event_Processed; } So for example NumericTextControl sets a handler, so that it uses keystrokes that would otherwise execute commmands. So I guess you could get up the appropriate keyboard handler(s) in the appropriate place(s)... > > Further, there is also an inconsistency between in-dock and not. We can use > safe keyboard shortcuts like SPACE and R when in an in-dock toolbar. As > soon as the toolbar is a floating toolbar, we can't. > > That 'inconsistency' is probably valuable to VI users who have toolbars > docked, as it means they can do more via keyboard when a toolbar has focus.
Thanks David. I already looked into that approach. The problem is that to get wxWidgets to handle keys like 'Left' and 'Right' you have to pass them on to it. If the control is sitting in the TrackPanel window, then that means allowing the key press to propagate to there, and then, IF a menu item is set for it, the menu item will get the key press instead, stop it going further and coming back to your control. The menu accelerators get in the way. ResumePropagation() and Skip(true) do not fix that. So that seems to mean the docks would have to be turned into actual windows, that did not have the menus, and then additionally key presses we do want to process ourselves would have to be caught by the dock. A problem with that is that there are some system provided menu events that should propagate to the menus. It turns out that we don't actually want active accelerators in the menus for our own bindings. We already have our own command manager which normally catches those key presses first. We do want accelerators there as hints, but we don't want the menu's accelerators to eat our key press events when we tell wxWidgets to handle a keypress events. If they do, the focused window won't see the event. When I disable the menu accelerators, I can successfully pass Left and Right keypresses on through to wxWidgets, past the menu accelerator catching, and hence to the focus window, such as project rate. The code I currently have working removes the problematic accelerator key bindings from displaying in the menus. That stops the menu catching them, but has the downside that there is no hint of the binding for us. Mainly relevant for Left and Right arrow. A better way to disable our accelerators (in menus) but still display them may be possible.
OK. I think this commit largely addresses the issue in this bug: https://github.com/audacity/audacity/commit/57b200884ee52f718cff9dec6c7e80fadc00a99c The main downside of it is that if you do bind Left, Right, Up or Down, then they will not appear as menu accelerators (in the menus). They still work though. They still show in Keyboard preferences.
(In reply to James Crook from comment #12) > A better way to disable our accelerators (in menus) but still display them may be > possible. That is wanted, IMO. Some of the benefit of the Extended Menus / Commands page in the Manual is lost otherwise. It sounds like a P2 enh to me. What is a screen reader user to make of two instances of "Toggle Focused Track T" as read in Ext-Command's "Focus" submenu, without hearing that they have different shortcuts?
*** STEPS UPDATED *** just for completeness by adding "Same applies to Up/Down, Return and NUMPAD_ENTER."
*** TITLE CHANGED *** The changed steps to reproduce meant that title and steps no longer were in sync. The scope of this bug has been significantly extended. Accordingly summary title updated too, so it refers to all these keys, not just left and right arrows. This bug now has more substantial overlap with bug 1637 which mentioned 'enter' in its steps.
Comment #14 is an argument for not giving the user the option of seeing the Ext commands in the menu, i.e. treat extended commands as visible menu as an #ifdef EXPERIMENTAL feature, and disable it. They will still be available in the preferences panel to bind to.
(In reply to James Crook from comment #16) > This bug now has more substantial overlap with bug 1637 which mentioned > 'enter' in its steps. That's why it's a "duplicate". AFAIK, the keys now mentioned in the steps in this bug were always affected, and #c0 was too specific and would have required additional duplicates.
(In reply to James Crook from comment #17) > Comment #14 is an argument for not giving the user the option of seeing the Ext > commands in the menu, i.e. treat extended commands as visible menu as an > #ifdef EXPERIMENTAL feature, and disable it. They will still be available in the > preferences panel to bind to. I disagree. Not only would that throw out one of the (IMO) universally beneficial menu changes, it would not solve all the problem. For example, clear the pair of instances of Left and Right in default Keyboard Preferences, then bind Left to File > Open and Right to File > New. As expected (to us) the shortcuts can't be seen in the menus.
No longer reproduces on any platforms. (In reply to James Crook from comment #13) > https://github.com/audacity/audacity/commit/57b2008 > > The main downside of it is that if you do bind Left, Right, Up or Down, then > they will not appear as menu accelerators (in the menus). They still work > though. They still show in Keyboard preferences. New bug for this is at bug 1641.
