Block IDs can repeat across multiple uses #9043
Assignees
Labels
Comments
|
Note that it would be difficult to explain all the possible failure scenarios that can occur due to this state, so the goal is mainly to fix it, verify that it's fixed, and check the current known failing case (google/blockly-samples#2512) for behavior differences. |
This was referenced May 13, 2025
BenHenning
added a commit
that referenced
this issue
May 14, 2025
## 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 google/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 google/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`. |
|
Fixed in #9045. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Check for duplicates
Description
Block IDs are not guaranteed to be re-generated when inserting a block from the flyout, and can even be duplicated in other contexts (such as with the minimap plugin--see google/blockly-samples#2512).
Note that this issue here is not that IDs are reused, it's that the focus system can have multiple nodes with identical IDs. This can result in unpredictable or outright incorrect behavior (see google/blockly-keyboard-experimentation#521 for one example of what can happen when IDs are duplicated).
The fix here isn't to change core behavior but, rather, to change what the focus system relies upon.
Reproduction steps
Stack trace
Screenshots
No response
Browsers
No response
The text was updated successfully, but these errors were encountered: