feat: Make WorkspaceSvg and BlockSvg focusable #8916

BenHenning · 2025-04-21T23:46:13Z

The basics

I validated my changes

The details

Resolves

Fixes #8913
Fixes #8914
Fixes part of #8771

Proposed Changes

This updates WorkspaceSvg and BlockSvg to be focusable, that is, it makes the workspace a IFocusableTree and blocks IFocusableNodes.

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 #8915.

Documentation

No documentation changes should be needed here.

Additional Information

This includes changes that have been pulled from #8875.

This adds basic support for WorkspaceSvg being focusable via its blocks, though fields and connections are not yet supported.

BenHenning

Self-reviewed changes that aren't part of #8909.

BenHenning · 2025-04-21T23:55:09Z

PTAL @gonfunko. FYI this includes changes from #8909 which can be ignored (there are no additional changes in those files in this PR).

…-workspace-focusable

rachel-fenichel · 2025-04-23T16:15:02Z

core/workspace_svg.ts

-    this.svgGroup_ = dom.createSvgElement(Svg.G, {'class': 'blocklyWorkspace'});
+    this.svgGroup_ = dom.createSvgElement(Svg.G, {
+      'class': 'blocklyWorkspace',
+      // Only the main workspace should be tabbable.


Say "top level" rather than "main", because Blockly.getMainWorkspace() already exists and refers to the currently active workspace.

From context, I'm assuming that the distinction is between top level and flyout or mutator workspaces.

That's correct--fixed!

rachel-fenichel · 2025-04-23T19:21:55Z

core/block_svg.ts

+
+  /** See IFocusableNode.onNodeFocus. */
+  onNodeFocus(): void {
+    if (!this.isShadow()) {


What happens if it is a shadow block?

This was actually copying https://github.com/google/blockly/blob/ac7fea11cffc4c445cedae77675290861ceec2f6/core/keyboard_nav/line_cursor.ts#L652-L657 but through a lot of code transforming. In retrospect, this is probably wrong.

To be fair, I'm not 100% certain how shadow blocks will behave and that will need testing. The field will receive focus, and the shadow block might be focusable if clicked on but will never otherwise be navigated to per the cursor's behavior (I think). So this should be safe to remove and just call setSelected() directly.

I'm also not sure. I think call setSelected directly here and do some manual verification, then file a ticket to follow up with comprehensive tests for that case.

So far I'm not seeing any issues, so I don't think there's a particular concern to address.

I've made a note in #8915 regarding this specific case.

…-workspace-focusable

Addresses review comment.

BenHenning

Self-reviewed latest changes & followed up to review comments.

Note that #8909 has been merged so this PR is only the changes related to workspace & blocks. Subsequently, I realized I forgot to include the tab index change to make blocks focusable, so that's been fixed in the latest changes.

BenHenning · 2025-04-23T21:48:54Z

core/block_svg.ts

+
+  /** See IFocusableNode.onNodeFocus. */
+  onNodeFocus(): void {
+    if (!this.isShadow()) {


This was actually copying https://github.com/google/blockly/blob/ac7fea11cffc4c445cedae77675290861ceec2f6/core/keyboard_nav/line_cursor.ts#L652-L657 but through a lot of code transforming. In retrospect, this is probably wrong.

To be fair, I'm not 100% certain how shadow blocks will behave and that will need testing. The field will receive focus, and the shadow block might be focusable if clicked on but will never otherwise be navigated to per the cursor's behavior (I think). So this should be safe to remove and just call setSelected() directly.

BenHenning · 2025-04-23T22:25:48Z

core/workspace_svg.ts

-    this.svgGroup_ = dom.createSvgElement(Svg.G, {'class': 'blocklyWorkspace'});
+    this.svgGroup_ = dom.createSvgElement(Svg.G, {
+      'class': 'blocklyWorkspace',
+      // Only the main workspace should be tabbable.


That's correct--fixed!

BenHenning · 2025-04-23T22:27:42Z

PTAL @gonfunko if you would like to double check the latest changes.

PTAL @rachel-fenichel per your comments.

BenHenning

Self-reviewed latest brances change.

BenHenning · 2025-04-24T21:47:55Z

Thanks all for the reviews. :) Going ahead and merging (with squash this time...).

…undation#8916)" This reverts commit d7680cf.

* Revert "feat: Make toolbox and flyout focusable (#8920)" This reverts commit 5bc8380. * Revert "feat: Make WorkspaceSvg and BlockSvg focusable (#8916)" This reverts commit d7680cf.

…#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.

_Note: This is a roll forward of #8916 that was reverted in #8933. See Additional Information below._ ## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #8913 Fixes #8914 Fixes part of #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). - `css.ts` was updated to remove `blocklyActiveFocus` and `blocklyPassiveFocus` since these have unintended highlighting consequences that aren't actually desirable yet. Instead, the exact styling for active/passive focus will be iterated in the keyboard navigation plugin project and moved to Core once finalized. ### 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. This actually contains no differences from the original PR except for `css.ts` which are documented above. It does employ a new merge strategy: all of the necessary PRs to move both Core and the plugin over to using `FocusManager` will be staged and merged in quick succession as ensuring the plugin works for each constituent change (vs. the final one) is quite complex. Thus, this PR *does* break the plugin, and won't be merged until its subsequent PRs are approved and also ready for merging. Edit: See #8938 (comment) for why this actually is being merged a bit sooner than originally planned. Keeping the original reasoning above for context.

_Note: This is a roll forward of #8920 that was reverted in #8933. See Additional Information below._ ## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## 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: - `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`. - `FlyoutButton` is now an `IFocusableNode` (with corresponding ID generation, tab index setting, and ID matching for retrieval in `WorkspaceSvg`). 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). This only addresses part of #8943: it adds support for `FlyoutButton` which covers both buttons and labels. However, a longer-term solution may be to change `FlyoutItem` itself to force using an `IFocusableNode` as 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 `WorkspaceSvg` and `FlyoutButton` in order to make `FlyoutButton`s focusable). Otherwise, this has the same caveats as those noted in #8938 with regards to the experimental keyboard navigation plugin.

BenHenning added 2 commits April 21, 2025 23:36

feat: Make WorkspaceSvg focusable.

41c8215

This adds basic support for WorkspaceSvg being focusable via its blocks, though fields and connections are not yet supported.

chore: remove accidental 'test.only'.

14b486e

BenHenning commented Apr 21, 2025

View reviewed changes

BenHenning marked this pull request as ready for review April 21, 2025 23:53

BenHenning requested a review from a team as a code owner April 21, 2025 23:53

BenHenning requested a review from gonfunko April 21, 2025 23:53

BenHenning added the PR: feature Adds a feature label Apr 21, 2025

BenHenning assigned gonfunko Apr 21, 2025

BenHenning changed the title ~~feat: Make WorkspaceSvg and BlockSvg focusable~~ feat: Make WorkspaceSvg and BlockSvg focusable [Blocked on: #8909] Apr 21, 2025

Merge branch 'add-focus-manager-callbacks-and-improvements' into make…

5ef2d7e

…-workspace-focusable

This was referenced Apr 22, 2025

feat: Make toolbox and flyout focusable #8920

Merged

[Do not merge] Test prototype focus manager incorporation #8875

Closed

gonfunko approved these changes Apr 22, 2025

View reviewed changes

BenHenning mentioned this pull request Apr 22, 2025

feat: Make fields focusable #8923

Merged

1 task

BenHenning changed the title ~~feat: Make WorkspaceSvg and BlockSvg focusable [Blocked on: #8909]~~ feat: Make WorkspaceSvg and BlockSvg focusable [Blocked on PR: #8909] Apr 22, 2025

rachel-fenichel reviewed Apr 23, 2025

View reviewed changes

Merge branch 'add-focus-manager-callbacks-and-improvements' into make…

4479b82

…-workspace-focusable

BenHenning changed the title ~~feat: Make WorkspaceSvg and BlockSvg focusable [Blocked on PR: #8909]~~ feat: Make WorkspaceSvg and BlockSvg focusable Apr 23, 2025

BenHenning added 2 commits April 23, 2025 21:43

fix: Ensure Block paths are focusable.

2637736

chore: Fix line comment.

49192ba

Addresses review comment.

BenHenning commented Apr 23, 2025

View reviewed changes

BenHenning assigned rachel-fenichel Apr 23, 2025

fix: remove unnecessary shadow check.

a346a92

BenHenning mentioned this pull request Apr 24, 2025

Add tests for accessibility use cases #8915

Open

1 task

BenHenning commented Apr 24, 2025

View reviewed changes

rachel-fenichel approved these changes Apr 24, 2025

View reviewed changes

BenHenning merged commit d7680cf into RaspberryPiFoundation:rc/v12.0.0 Apr 24, 2025
7 checks passed

BenHenning deleted the make-workspace-focusable branch April 24, 2025 21:49

BenHenning mentioned this pull request Apr 24, 2025

Blocks. #8914

Closed

RoboErikG mentioned this pull request Apr 25, 2025

Revert "feat: Make toolbox and flyout focusable" #8931

Closed

RoboErikG added a commit to RoboErikG/blockly that referenced this pull request Apr 25, 2025

Revert "feat: Make WorkspaceSvg and BlockSvg focusable (RaspberryPiFo…

15ced80

…undation#8916)" This reverts commit d7680cf.

This was referenced Apr 25, 2025

Fix: Revert focus prs #8932

Closed

Fix: Revert focus prs #8933

Merged

BenHenning mentioned this pull request Apr 29, 2025

Workspace. #8913

Closed

This was referenced Apr 29, 2025

feat: Make WorkspaceSvg and BlockSvg focusable (roll forward) #8938

Merged

feat: Make toolbox and flyout focusable (roll forward) #8939

Merged

feat: Make WorkspaceSvg and BlockSvg focusable #8916

feat: Make WorkspaceSvg and BlockSvg focusable #8916

Uh oh!

Conversation

BenHenning commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Documentation

Additional Information

Uh oh!

BenHenning left a comment

Choose a reason for hiding this comment

Uh oh!

BenHenning commented Apr 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenHenning left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenHenning commented Apr 23, 2025

Uh oh!

BenHenning left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenHenning commented Apr 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BenHenning commented Apr 21, 2025 •

edited

Loading

BenHenning left a comment •

edited

Loading