feat: Add keyboard shortcut to perform an action on the currently focused element#9673
feat: Add keyboard shortcut to perform an action on the currently focused element#9673
Conversation
mikeharv
left a comment
mikeharv
left a comment
There was a problem hiding this comment.
This looks great. Thanks for your patience with my questions as some of this code is still somewhat new to me.
| * Handles the user acting on this bubble via keyboard navigation by focusing | ||
| * the comment editor. | ||
| */ | ||
| performAction() { |
There was a problem hiding this comment.
Any reason we're adding this to these Bubble subclasses instead of the base class itself?
There was a problem hiding this comment.
Yes, see the other reply on the workspace bubble.
| return shortcut | ||
| .map((maybeNumeric) => | ||
| Number.isFinite(+maybeNumeric) | ||
| ? String.fromCharCode(+maybeNumeric) |
There was a problem hiding this comment.
Could we document the expected numeric range for these strings? Are they always assumed to be character codes for letters/numbers?
There was a problem hiding this comment.
Yes, they're always character codes. The isFinite thing isn't to avoid Infinity, just to check if a string is secretly a number.
| * the mutator workspace. | ||
| */ | ||
| performAction() { | ||
| getFocusManager().focusTree(this.getWorkspace()); |
There was a problem hiding this comment.
Is there a particular reason to add this to two Bubble subclasses as opposed to just the base class?
There was a problem hiding this comment.
Yes, they're doing different things; the mini workspace bubble focuses its workspace and the text input bubble focuses its comment editor. Bubble is an abstract class and there's no common behavior, so each subclass does its own thing.
| * @param action The action name, e.g. "cut". | ||
| * @returns The formatted shortcuts as individual keys. | ||
| */ | ||
| export function getLongActionShortcutsAsKeys(action: string): string[][] { |
There was a problem hiding this comment.
Do we expect to use this for screen reader output?
There was a problem hiding this comment.
Not exactly, at present it's not used for anything but I just brought back this whole file verbatim. It will be used in the dialog that displays all the available shortcuts, which isn't screenreader per se but would/can be read by a screenreader.
|
|
||
| /** | ||
| * Find the primary shortcut for this platform and return it as single string | ||
| * in a short user facing format. |
There was a problem hiding this comment.
For this and the function below it, could we document a simple example of each to make it obvious how the short/long formats differ?
| * @param workspace The workspace. | ||
| */ | ||
| export function showHelpHint(workspace: WorkspaceSvg) { | ||
| const shortcut = getShortActionShortcut('list_shortcuts'); |
There was a problem hiding this comment.
Should we handle the potential case where shortcut is an empty string? I think it's possible that the shortcut could have been unregistered which would lead to a help prompt with a nonsensical message.
There was a problem hiding this comment.
That is a possibility, I'm not sure how we might helpfully handle it though. End users wouldn't be able to do anything about it, I suppose we could just not show the toast? Or did you have something else in mind?
| Blockly.Msg.KEYBOARD_NAV_CUT_HINT = 'Cut. Press %1 to paste.'; No newline at end of file | ||
| Blockly.Msg.KEYBOARD_NAV_CUT_HINT = 'Cut. Press %1 to paste.'; | ||
| /** @type {string} */ | ||
| /// Message shown when a user presses Enter with a navigable block focused. |
There was a problem hiding this comment.
Will translators need any more context? For example, is it still the right arrow key on LTR workspaces? (Genuine question I don't know the answer to.)
There was a problem hiding this comment.
I think we're OK, ideally this would interpolate the actually-bound shortcut like some of the others but that's something we can address later I think.
| const toolbox = document.getElementById('toolbox-categories'); | ||
| this.workspace = Blockly.inject('blocklyDiv', {toolbox}); | ||
| const toolbox = document.getElementById('toolbox-test'); | ||
| this.workspace = Blockly.inject('blocklyDiv', {toolbox, renderer: 'zelos'}); |
There was a problem hiding this comment.
Intentional to change this whole test suite to use Zelos?
There was a problem hiding this comment.
Yes, in order to test the behavior of full-block fields, which don't exist in other renderers. All the other tests continue to work fine with it, so I just changed it globally for the file.
| }); | ||
|
|
||
| suite('Focus Toolbox (T)', function () { | ||
| setup(function () { |
There was a problem hiding this comment.
Did this have a purpose for this test suite at some point?
There was a problem hiding this comment.
Yes, this block is in the old categories toolbox these tests were using, but is not in the test toolbox we're now using, so we don't need to define it any more.
2 participants
The basics
The details
Resolves
Proposed Changes
This PR backports the Enter keyboard shortcut from the keyboard-experimentation repo that performs an action on the currently focused element. It adds an optional
performAction()method to IFocusableNode which may be adopted by classes to gain support for Enter acting upon them.