feat: Make toolbox and flyout focusable (roll forward) #8939
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…#8916) ## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes RaspberryPiFoundation#8913 Fixes RaspberryPiFoundation#8914 Fixes part of RaspberryPiFoundation#8771 ### Proposed Changes This updates `WorkspaceSvg` and `BlockSvg` to be focusable, that is, it makes the workspace a `IFocusableTree` and blocks `IFocusableNode`s. Some important details: - While this introduces focusable tree support for `Workspace` it doesn't include two other components that are obviously needed by the keyboard navigation plugin's playground: fields and connections. These will be introduced in subsequent PRs. - Blocks are set up to automatically synchronize their selection state with their focus state. This will eventually help to replace `LineCursor`'s responsibility for managing selection state itself. - The tabindex property for the workspace and its ARIA label have been moved down to the `.blocklyWorkspace` element itself rather than its wrapper. This helps address some tab stop issues that are already addressed in the plugin (via monkey patches), but also to ensure that the workspace's main SVG group interacts correctly with `FocusManager`. - `WorkspaceSvg` is being initially set up to default to its first top block when being focused for the first time. This is to match parity with the keyboard navigation plugin, however the latter also has functionality for defaulting to a position when no blocks are present. It's not clear how to actually support this under the new focus-based system (without adding an ephemeral element on which to focus), or if it's even necessary (since the workspace root can hold focus). ### Reason for Changes This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin). ### Test Coverage No new tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of RaspberryPiFoundation#8915. ### Documentation No documentation changes should be needed here. ### Additional Information This includes changes that have been pulled from RaspberryPiFoundation#8875.
## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes RaspberryPiFoundation#8918 Fixes RaspberryPiFoundation#8919 Fixes part of RaspberryPiFoundation#8771 ### Proposed Changes This updates several classes in order to make toolboxes and flyouts focusable: - `IFlyout` is now an `IFocusableTree` with corresponding implementations in `FlyoutBase`. - `IToolbox` is now an `IFocusableTree` with corresponding implementations in `Toolbox`. - `IToolboxItem` is now an `IFocusableNode` with corresponding implementations in `ToolboxItem`. - As the primary toolbox items, `ToolboxCategory` and `ToolboxSeparator` were updated to have -1 tab indexes and defined IDs to help `ToolboxItem` fulfill its contracted for `IFocusableNode.getFocusableElement`. Each of these two new focusable trees have specific noteworthy behaviors behaviors: - `Toolbox` will automatically indicate that its first item should be focused (if one is present), even overriding the ability to focus the toolbox's root (however there are some cases where that can still happen). - `Toolbox` will automatically synchronize its selection state with its item nodes being focused. - `FlyoutBase`, now being a focusable tree, has had a tab index of 0 added. Normally a tab index of -1 is all that's needed, but the keyboard navigation plugin specifically uses 0 for flyout so that the flyout is tabbable. This is a **new** tab stop being introduced. - `FlyoutBase` holds a workspace (for rendering blocks) and, since `WorkspaceSvg` is already set up to be a focusable tree, it's represented as a subtree to `FlyoutBase`. This does introduce some wonky behaviors: the flyout's root will have passive focus while its contents have active focus. This could be manually disabled with some CSS if it ends up being a confusing user experience. - Both `FlyoutBase` and `WorkspaceSvg` have built-in behaviors for detecting when a user tries navigating away from an open flyout to ensure that the flyout is closed when it's supposed to be. That is, the flyout is auto-hideable and a non-flyout, non-toolbox node has then been focused. This matches parity with the `T`/`Esc` flows supported in the keyboard navigation plugin playground. One other thing to note: `Toolbox` had a few tests to update that were trying to reinit a toolbox without first disposing of it (which was caught by one of `FocusManager`'s state guardrails). ### Reason for Changes This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin). ### Test Coverage No new tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of RaspberryPiFoundation#8915. ### Documentation No documentation changes should be needed here. ### Additional Information This includes changes that have been pulled from RaspberryPiFoundation#8875.
This was
linked to
issues
Apr 29, 2025
Closed
This was referenced Apr 29, 2025
The CSS work here is far more extensive than should go in this or any of the structural branches. Instead, proper rendering will be done via the plugin and eventually merged into Core once finalized.
…x-and-flyout-focusable-roll-forward
BenHenning
commented
Apr 30, 2025
It needed a unique ID and to be properly hooked up to node retrieval.
Addresses self-review comment.
BenHenning
commented
Apr 30, 2025
Contributor
Author
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Self-reviewed changes that don't have overlap with #8938.
Contributor
Author
|
PTAL @rachel-fenichel. FYI that this does include a few changes over #8920 in order to make @gonfunko feel free to take a pass, but I believe Rachel plans to review the entire chain of PRs up to #8941. |
rachel-fenichel
approved these changes
Apr 30, 2025
…x-and-flyout-focusable-roll-forward
BenHenning
changed the title
Contributor
Author
|
See #8938 (comment) for why this is being merged now. |
BenHenning
commented
Apr 30, 2025
Contributor
Author
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Self-reviewed. Confirming the changes are identical to BenHenning#4 so no extra changes have gotten in.
BenHenning
merged commit cac8f01
into
RaspberryPiFoundation:rc/v12.0.0
7 of 9 checks passed
This was referenced Apr 30, 2025
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: This is a roll forward of #8920 that was reverted in #8933. See Additional Information below.
The basics
The details
Resolves
Fixes #8918
Fixes #8919
Fixes part of #8943
Fixes part of #8771
Proposed Changes
This updates several classes in order to make toolboxes and flyouts focusable:
IFlyoutis now anIFocusableTreewith corresponding implementations inFlyoutBase.IToolboxis now anIFocusableTreewith corresponding implementations inToolbox.IToolboxItemis now anIFocusableNodewith corresponding implementations inToolboxItem.ToolboxCategoryandToolboxSeparatorwere updated to have -1 tab indexes and defined IDs to helpToolboxItemfulfill its contracted forIFocusableNode.getFocusableElement.FlyoutButtonis now anIFocusableNode(with corresponding ID generation, tab index setting, and ID matching for retrieval inWorkspaceSvg).Each of these two new focusable trees have specific noteworthy behaviors behaviors:
Toolboxwill automatically indicate that its first item should be focused (if one is present), even overriding the ability to focus the toolbox's root (however there are some cases where that can still happen).Toolboxwill automatically synchronize its selection state with its item nodes being focused.FlyoutBase, now being a focusable tree, has had a tab index of 0 added. Normally a tab index of -1 is all that's needed, but the keyboard navigation plugin specifically uses 0 for flyout so that the flyout is tabbable. This is a new tab stop being introduced.FlyoutBaseholds a workspace (for rendering blocks) and, sinceWorkspaceSvgis already set up to be a focusable tree, it's represented as a subtree toFlyoutBase. This does introduce some wonky behaviors: the flyout's root will have passive focus while its contents have active focus. This could be manually disabled with some CSS if it ends up being a confusing user experience.FlyoutBaseandWorkspaceSvghave built-in behaviors for detecting when a user tries navigating away from an open flyout to ensure that the flyout is closed when it's supposed to be. That is, the flyout is auto-hideable and a non-flyout, non-toolbox node has then been focused. This matches parity with theT/Escflows supported in the keyboard navigation plugin playground.One other thing to note:
Toolboxhad a few tests to update that were trying to reinit a toolbox without first disposing of it (which was caught by one ofFocusManager's state guardrails).This only addresses part of #8943: it adds support for
FlyoutButtonwhich covers both buttons and labels. However, a longer-term solution may be to changeFlyoutItemitself to force using anIFocusableNodeas its element.Reason for Changes
This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin).
Test Coverage
No new tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of #8915.
Documentation
No documentation changes should be needed here.
Additional Information
This includes changes that have been pulled from #8875.
This was originally merged in #8916 but was reverted in #8933 due to RaspberryPiFoundation/blockly-keyboard-experimentation#481. Note that this does contain a number of differences from the original PR (namely, changes in
WorkspaceSvgandFlyoutButtonin order to makeFlyoutButtons focusable). Otherwise, this has the same caveats as those noted in #8938 with regards to the experimental keyboard navigation plugin.