feat: support keyboard navigation of flyout buttons #7852

mikeharv · 2024-02-13T19:40:26Z

The basics

I validated my changes

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

Create new AST Node type for flyout buttons. A marker for a flyout button resembles one used for a stack.
Implement IASTNodeLocationSvg into the FlyoutButton class. This is necessary in order to be able to create a navigation cursor around a flyout button.
Add methods setContents() and getContents() 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.
Add new 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:

modify navigation.js to find top button or block based on contents
modify navigation_controller to 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

github-actions

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.

cpcallen

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.

cpcallen · 2024-03-13T13:59:20Z

core/flyout_base.ts

+  /**
+   * List of visible buttons and blocks.
+   */
+  protected contents_: (FlyoutButton | BlockSvg | undefined)[] = [];


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.

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.

cpcallen · 2024-03-13T14:26:23Z

core/flyout_button.ts

+   * 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.


Please reformat to keep line length <=80 characters. (I recommend <=70 to reduce churn from minor edits in future.)

cpcallen · 2024-03-13T18:02:14Z

core/keyboard_nav/ast_node.ts

+      case ASTNode.types.STACK: {
+        const block = this.location as Block;
+        if (block.workspace.isFlyout) {
+          return this.navigateFlyoutContents(true);
+        } else {
+          return this.navigateBetweenStacks(true);
+        }
+      }


See top-level comment re: subclassing.

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.

Moved the .isFlyout check into navigateBetweenStacks, as @maribethb suggested.

maribethb · 2024-03-20T21:47:35Z

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.

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!

maribethb

I don't think you need to make design changes, just address the smaller feedback. Thanks!

maribethb · 2024-03-20T21:53:11Z

core/keyboard_nav/ast_node.ts

+      case ASTNode.types.STACK: {
+        const block = this.location as Block;
+        if (block.workspace.isFlyout) {
+          return this.navigateFlyoutContents(true);
+        } else {
+          return this.navigateBetweenStacks(true);
+        }
+      }


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

mb reviewed, christopher ooo

* feat: support keyboard navigation of flyout buttons * fix: use FlyoutItem type for flyout contents, rework navigateBetweenStacks for flyouts

feat: support keyboard navigation of flyout buttons

56c0c02

mikeharv requested a review from a team as a code owner February 13, 2024 19:40

mikeharv requested a review from cpcallen February 13, 2024 19:40

github-actions bot added the PR: feature Adds a feature label Feb 13, 2024

github-actions bot assigned cpcallen Feb 13, 2024

github-actions bot reviewed Feb 13, 2024

View reviewed changes

github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels Feb 13, 2024

mikeharv mentioned this pull request Feb 14, 2024

feat: support keyboard navigation of flyout buttons google/blockly-samples#2200

Merged

1 task

cpcallen previously requested changes Mar 13, 2024

View reviewed changes

cpcallen requested a review from maribethb March 13, 2024 20:01

github-actions bot assigned maribethb Mar 13, 2024

cpcallen removed their assignment Mar 14, 2024

maribethb reviewed Mar 20, 2024

View reviewed changes

fix: use FlyoutItem type for flyout contents, rework navigateBetweenS…

a48fd25

…tacks for flyouts

mikeharv requested review from cpcallen and maribethb April 1, 2024 23:39

github-actions bot assigned cpcallen Apr 1, 2024

maribethb approved these changes Apr 2, 2024

View reviewed changes

maribethb merged commit 9e05d69 into google:develop Apr 2, 2024
8 checks passed

maribethb mentioned this pull request May 3, 2024

fix!: add getContents to IFlyout #8064

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support keyboard navigation of flyout buttons #7852

feat: support keyboard navigation of flyout buttons #7852

mikeharv commented Feb 13, 2024 •

edited

Loading

github-actions bot left a comment

cpcallen left a comment

cpcallen Mar 13, 2024

mikeharv Apr 1, 2024

cpcallen Mar 13, 2024

mikeharv Apr 1, 2024

cpcallen Mar 13, 2024

maribethb Mar 20, 2024

mikeharv Apr 1, 2024

maribethb commented Mar 20, 2024

maribethb left a comment

maribethb Mar 20, 2024

		* 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.

feat: support keyboard navigation of flyout buttons #7852

feat: support keyboard navigation of flyout buttons #7852

Conversation

mikeharv commented Feb 13, 2024 • edited Loading

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Documentation

Additional Information

github-actions bot left a comment

Choose a reason for hiding this comment

cpcallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maribethb commented Mar 20, 2024

maribethb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeharv commented Feb 13, 2024 •

edited

Loading