Skip to content

Commit 2b9d06a

Browse files
authored
fix: Use a unique focus ID for BlockSvg. (#9045)
## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #9043 Fixes RaspberryPiFoundation/blockly-samples#2512 ### Proposed Changes This replaces using BlockSvg's own ID for focus management since that's not guaranteed to be unique across all workspaces on the page. ### Reason for Changes Both RaspberryPiFoundation/blockly-samples#2512 covers the user-facing issue in more detail, but from a technical perspective it's possible for blocks to share IDs across workspaces. One easy demonstration of this is the flyout: the first block created from the flyout to the main workspace will share an ID. The workspace minimap plugin just makes the underlying problem more obvious. The reason this introduces a breakage is due to the inherent ordering that `FocusManager` uses when trying to find a matching tree for a given DOM element that has received focus. These trees are iterated in the order of their registration, so it's quite possible for some cases (like main workspace vs. flyout) to resolve such that the behavior looks correct to users, vs. others (such as the workspace minimap) not behaving as expected. Guaranteeing ID uniqueness across all workspaces fixes the problem entirely. ### Test Coverage This has been manually tested in core Blockly's simple test playground and in Blockly samples' workspace minimap plugin test environment (linked against this change). See the new behavior for the minimap plugin: [Screen recording 2025-05-13 4.31.31 PM.webm](https://github.com/user-attachments/assets/d2ec3621-6e86-4932-ae85-333b0e7015e1) Note that this is a regression to v11 behavior in that the blocks in the minimap now show as selected. This has been verified as working with the latest version of the keyboard navigation plugin (tip-of-tree). Keyboard-based block operations and movement seem to work as expected. For automated testing this is expected to largely be covered by future tests added as part of resolving #8915. ### Documentation No public documentation changes should be needed, though `IFocusableNode`'s documentation has been refined to be clearer on the uniqueness property for focusable element IDs. ### Additional Information There's a separate open design question here about whether `BlockSvg`'s descendants should use the new focus ID vs. the block ID. Here is what I consider to be the trade-off analysis in this decision: | | Pros | Cons | |------------------------|-------------------------------------------------|------------------------------------------------------------------------------| | Use `BlockSvg.id` | Can use fast `WorkspaceSvg.getBlockById`. | `WorkspaceSvg.lookUpFocusableNode` now uses 2 different IDs. | | Use `BlockSvg.focusId` | Consistency in IDs use for block-related focus. | Requires more expensive block look-up in `WorkspaceSvg.lookUpFocusableNode`. |
1 parent ae22165 commit 2b9d06a

File tree

3 files changed

+14
-7
lines changed

3 files changed

+14
-7
lines changed

core/block_svg.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ import type {IPathObject} from './renderers/common/i_path_object.js';
5555
import * as blocks from './serialization/blocks.js';
5656
import type {BlockStyle} from './theme.js';
5757
import * as Tooltip from './tooltip.js';
58+
import {idGenerator} from './utils.js';
5859
import {Coordinate} from './utils/coordinate.js';
5960
import * as dom from './utils/dom.js';
6061
import {Rect} from './utils/rect.js';
@@ -210,7 +211,9 @@ export class BlockSvg
210211

211212
// Expose this block's ID on its top-level SVG group.
212213
this.svgGroup.setAttribute('data-id', this.id);
213-
svgPath.id = this.id;
214+
215+
// The page-wide unique ID of this Block used for focusing.
216+
svgPath.id = idGenerator.getNextUniqueId();
214217

215218
this.doInit_();
216219
}

core/interfaces/i_focusable_node.ts

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,13 @@ export interface IFocusableNode {
1919
* - blocklyActiveFocus
2020
* - blocklyPassiveFocus
2121
*
22-
* The returned element must also have a valid ID specified, and unique to the
23-
* element relative to its nearest IFocusableTree parent. It must also have a
24-
* negative tabindex (since the focus manager itself will manage its tab index
25-
* and a tab index must be present in order for the element to be focusable in
26-
* the DOM).
22+
* The returned element must also have a valid ID specified, and unique across
23+
* the entire page. Failing to have a properly unique ID could result in
24+
* trying to focus one node (such as via a mouse click) leading to another
25+
* node with the same ID actually becoming focused by FocusManager. The
26+
* returned element must also have a negative tabindex (since the focus
27+
* manager itself will manage its tab index and a tab index must be present in
28+
* order for the element to be focusable in the DOM).
2729
*
2830
* The returned element must be visible if the node is ever focused via
2931
* FocusManager.focusNode() or FocusManager.focusTree(). It's allowed for an

core/workspace_svg.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2759,7 +2759,9 @@ export class WorkspaceSvg
27592759
}
27602760

27612761
// Search for a specific block.
2762-
const block = this.getBlockById(id);
2762+
const block = this.getAllBlocks(false).find(
2763+
(block) => block.getFocusableElement().id === id,
2764+
);
27632765
if (block) return block;
27642766

27652767
// Search for a workspace comment (semi-expensive).

0 commit comments

Comments
 (0)