feat: support keyboard navigation of flyout buttons #7852
Conversation
There was a problem hiding this comment.
Welcome! It looks like this is your first pull request in Blockly, so here are a couple of tips:
- You can find tips about contributing to Blockly and how to validate your changes on our developer site.
- All contributors must sign the Google Contributor License Agreement (CLA). If the google-cla bot leaves a comment on this PR, make sure you follow the instructions.
- We use conventional commits to make versioning the package easier. Make sure your commit message is in the proper format or learn how to fix it.
- If any of the other checks on this PR fail, you can click on them to learn why. It might be that your change caused a test failure, or that you need to double-check the style guide.
Thank you for opening this PR! A member of the Blockly team will review it soon.
There was a problem hiding this comment.
Hi Mike,
Thanks for this great contribution. We always appreciate it when people submit substantial fixes to Blockly, especially for important things like accessibility.
I'm marking this review as "request changes" due to a few small nit-picks below, but I know that several of my colleagues want to have a look at this PR before it goes in—which is good because this is an area of the Blockly codebase I am not so familiar with—and I wanted raise the possibility that there might be some scope for making the design a little cleaner. Specifically:
Although I'm not entirely clear about the purpose of the ASTNode class (alas it has no meaningful JSDoc or file overview), I was surprised to see you trying to kludge support for flyout buttons into this class: buttons don't seem to be nodes of an abstract syntax tree the way blocks are. And then when I started to see several switch statements dealing with button vs. non-button ASTNodes, I began to wonder if it might be better to modify the class structure.
My first thought was that perhaps ASTNode should have a subclass for buttons (let's call it ButtonNode but better suggestions welcome), but upon reflection I twonder if it would be better to have a (possibly abstract) NavigationNode class, with subclasses ASTNode and ButtonNode. (Or, if NavigationNode doesn't have much common code on it, it could be an interface implemented by both ASTNode and ButtonNode.
Perhaps my colleagues would be so kind as to weigh in on this design suggestion.
core/flyout_base.ts
Outdated
| /** | ||
| * List of visible buttons and blocks. | ||
| */ | ||
| protected contents_: (FlyoutButton | BlockSvg | undefined)[] = []; |
There was a problem hiding this comment.
Although there are still some legacy examples in the Blockly codebase that have either been retained for backwards compatibility or simply not fixed yet, the current Google TS styleguide forbids prefixing or suffixing an identifier with an underscore.
Also: since I'm not clear what the presence of undefined in this array might mean, it would probably be useful to explain that in the description.
There was a problem hiding this comment.
I realized I could just use the FlyoutItem type for the array contents here which makes more sense and saves us from having to specify a potential undefined type.
core/flyout_button.ts
Outdated
| * Required by IASTNodeLocationSvg, but not used. A marker cannot be set on a button. | ||
| * If the 'mark' shortcut is used on a button, its associated callback function is triggered. |
There was a problem hiding this comment.
Please reformat to keep line length <=80 characters. (I recommend <=70 to reduce churn from minor edits in future.)
core/keyboard_nav/ast_node.ts
Outdated
| case ASTNode.types.STACK: { | ||
| const block = this.location as Block; | ||
| if (block.workspace.isFlyout) { | ||
| return this.navigateFlyoutContents(true); | ||
| } else { | ||
| return this.navigateBetweenStacks(true); | ||
| } | ||
| } |
There was a problem hiding this comment.
See top-level comment re: subclassing.
There was a problem hiding this comment.
I don't think subclassing is a requirement right now, but I would suggest moving this check into the navigateBetweenStacks method rather than repeating this check in multiple places.
There was a problem hiding this comment.
Moved the .isFlyout check into navigateBetweenStacks, as @maribethb suggested.
This class already has support for different "types" of nodes: workspaces, blocks, fields, etc. None of those are subclasses of this one. If a block is a node on the tree of "things that can be on a workspace" then I think that also applies to buttons, where the tree container is a flyout workspace. The switch statements are not new in this PR, they are part of the original design of the class, and only additional branches are being made. There is probably room for improvement in this design, but I don't think this PR pushes it over the edge into requiring a new design. That would need a design doc and I think that would be unreasonable in holding up changes to improve the existing accessibility features. The reasons why improvement may be desired already existed before this PR. So overall, I'm fine with the design as-is and only have the minor comments Christopher made and perhaps a few others. Let me know if you have questions, Mike! |
core/keyboard_nav/ast_node.ts
Outdated
| case ASTNode.types.STACK: { | ||
| const block = this.location as Block; | ||
| if (block.workspace.isFlyout) { | ||
| return this.navigateFlyoutContents(true); | ||
| } else { | ||
| return this.navigateBetweenStacks(true); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think subclassing is a requirement right now, but I would suggest moving this check into the navigateBetweenStacks method rather than repeating this check in multiple places.
…tacks for flyouts
* feat: support keyboard navigation of flyout buttons * fix: use FlyoutItem type for flyout contents, rework navigateBetweenStacks for flyouts
The basics
The details
Resolves
Makes it possible to support keyboard navigation of flyout buttons, which is listed as a limitation in the documentation:
https://developers.google.com/blockly/guides/configure/web/keyboard-nav
Proposed Changes
IASTNodeLocationSvginto theFlyoutButtonclass. This is necessary in order to be able to create a navigation cursor around a flyout button.setContents()andgetContents()for flyouts. These are used to be able to return an ordered array of blocks and buttons, necessary for navigating the contents of a flyout.navigateFlyoutContents()function which supports traversing next/previous flyout items.Reason for Changes
These changes will make it possible for users to navigate between stacks and buttons in a flyout. Code.org specifically needs this functionality for variables as well its modal editor for functions and behaviors.
There should not be any breaking changes here and you still do not have to use the plugin at all. However, if you use these changes in core with an old version of the plugin, there is a potential for crashing. Without updating the plugin, using the "mark" shortcut on a button will crash. Additionally, the first block will always be selected when a flyout is focused, even if there is a button above it (e.g. Variables category).
I have drafted a PR (google/blockly-samples#2200) for @blockly/keyboard-navigation that includes those needed changes. Specifically, it will be necessary to:
navigation.jsto find top button or block based on contentsnavigation_controllerto trigger button callbacks when pressing "mark" (enter key)The changes described above are also working locally. The following screen recording demonstrates some simple flyout navigation including using various buttons. Note that the keyboard monitoring in the middle of the workspace is a separate application and just used here for illustration purposes.
https://github.com/google/blockly/assets/43474485/899bf8e3-5a76-4c5c-8e3a-3645e42de323
Test Coverage
No additional tests have been added so far.
Documentation
The following sections would need to be updated:
Additional Information