feat: add keyboard navigation controller #8924
Conversation
|
I 8000 will add tests but wanted a gut check on how we feel about the code in gesture before I write tests for it. |
|
Yeah, I think this approach makes sense. Adding a page-wide Blockly object is justified because we otherwise could hit the inconsistent state of mouse mode in one workspace and keyboard in another, with mixed visualizations as a result. |
There was a problem hiding this comment.
Thanks @maribethb! The general approach makes sense, though I do have some specific questions for documentation and how we should be initializing the controller (I wonder if this initialization change from the current plugin behavior is what's causing google/blockly-keyboard-experimentation#555 to fail its tests, but I didn't actually look to verify).
Separately, do you have plans to add tests either in google/blockly-keyboard-experimentation#555 or separately for verifying the specific cases of "do action and verify keyboard nav is on/off"? That seems very important to include as a way of early detecting issues with the controller integration points, and to ensure that each integration point is properly tested (since I don't think we can rely on the main flows to catch every exact point where keyboard nav should be enabled/disabled--it seems very likely for edge cases to crop up with a system like this where we have to make sure we hook it up in all the correct places at all the correct times).
| @@ -743,6 +746,8 @@ export class Gesture { | |||
| e.preventDefault(); | |||
| e.stopPropagation(); | |||
|
|
|||
| keyboardNavigationController.setIsActive(false); | |||
There was a problem hiding this comment.
Out of curiosity, what happens if someone tries using the menu key? Does that simulate a right click or require a key event handler? It doesn't currently work in Blockly, and I'm unsure of the expectations for that key for accessibility (or if those expectations even matter since we have so much custom key binding, anyway).
There was a problem hiding this comment.
When you say menu key, do you mean the keyboard shortcut to open the context menu? That just opens the menu, it doesn't go through the gesture system, so it isn't affected by this.
There was a problem hiding this comment.
I think even your question to try and disambiguate my question is still ambiguous. :) "Menu key" is a bit confusing.
I actually mean this key: https://en.wikipedia.org/wiki/Menu_key. From testing it seems like it does nothing, so this is probably a no-op, but it occurred to me because it could theoretically be implemented as a right click on systems where right clicks are intended for opening the context menu.
| */ | ||
| export class KeyboardNavigationController { | ||
| /** Whether the user is actively using keyboard navigation. */ | ||
| private isActive = false; |
There was a problem hiding this comment.
There's an important difference here with the version implemented in google/blockly-keyboard-experimentation#511: this doesn't default on when focus is received.
This brings up a broader question: how do we decide a proper default? For non-plugin users it completely makes sense to default to off since keyboard nav will be confusing and most users are likely to use mouse. However, as we move more of keyboard nav into core there's an eventual question of properly initializing this state based on the user. I'm not sure arrow keys and the like are the correct approach since we're teaching users to tab-navigate before using arrow keys.
Tab navigation right now just means focusing, and focusing happens on click. We could perhaps listen for tabs and forward them to the browser (and use the presence of pressing tab to mean 'enable keyboard nav'), but this gets tricky with the likes of #9049.
There was a problem hiding this comment.
I don't think tabbing should activate keyboard navigation mode. I would put it in the same category as delete or ctrl+c.
If a user is using keyboard navigation, they won't see the visualizations until they start using the arrow keys or other shortcuts. In practice, that means they are missing one visualization: the workspace focus ring when any part of the workspace (including blocks on it) has active focus. Any other interaction that would show a visualization would entail first using one of the actions that turns on keyboard mode. And even in this case, they do still get the block focus ring.
This approach also lets the application set their own heuristics. They could enable it if the user has enabled some preference, or if the user was using keyboard navigation last time they loaded the workspace for example. I think it's reasonable for blockly to default to off and let the application to decide a different default if they want.
There was a problem hiding this comment.
I think that makes sense, though I do wonder if we need to document it somewhere more clearly so that application authors know exactly what to expect with how, and when, keyboard nav mode properly enables.
The two flows I'm unsure of are:
- A low vision user relying on a combination of tab/keyboard nav and visuals for interaction (with no mouse movement). This might be an extremely specific case though, I'm not sure.
- A user with motor impairments that has to use limited keyboard actions, but is not visually impaired and is still largely relying on visual indicators.
Both can certainly be made better with application side customizing, as you mentioned.
For low vision users not relying on visuals then the on/off highlights shouldn't matter, and users using a combination of keyboard and mouse I suspect will use mouse to select a block before arrow keys.
There was a problem hiding this comment.
I know that microbit really wants to avoid having a user-setting for this, but I think for the reasons you mentioned and others, it probably makes sense to have one. But that will be the application's responsibility to figure out :)
|
google/blockly-keyboard-experimentation#555 fails to build because I'm working on tests now but I wanted to make sure the approach was generally approved before finishing them. |
|
I'm adding more serious webdriver tests in the kb experiment since there's no scenario in which it's ever set to true in core. |
|
Tests are in google/blockly-keyboard-experimentation#555 so this is ready for review again now. |
The basics
The details
Resolves
Fixes #8852
Proposed Changes
KeyboardNavigationControllersingleton that has methods for setting keyboard navigation as active or not.See matching PR in kbe google/blockly-keyboard-experimentation#555
Additional Information
The idea behind this is explained in #8852
When a user takes a "mouse action" like dragging blocks or the workspace with the mouse or right-clicking a block, then we decide they're not using keyboard navigation and remove the css class.
When a user takes a "keyboard action" like pressing m for move mode, using the arrow keys to navigate, pressing w to get a workspace cursor, etc. we add the css class, which can be used to show additional styling such as passive focus in certain circumstances. So far these keyboard actions are only present in the keyboard-experiment plugin, so core will never set the value to true, so there's no way for this to affect projects that aren't using the keyboard plugin.
There are some ambiguous actions like using the keyboard to copy/paste that are frequently done by both mouse and keyboard users. Those actions do not update the mode one way or the other.
In this PR I also chose not to change the mode for simple block clicks. The reason is that Ben pointed out that for mixed-mode users, clicking might be simpler with the mouse (or a tap on touch devices) while other actions use keyboard shortcuts. I think the current behavior is good. It starts as false, so a mouse-only user won't ever see the extra styling for keyboard users. Keyboard users will see the extra styling when they take any of several common actions with the keyboard. Mixed mode users will largely always see the extra styling unless they use the mouse to drag blocks. This is an improvement over the current input mode tracking in the plugin which does not handle the ambiguous cases noted above.