mirror of
https://github.com/google/blockly.git
synced 2026-01-08 09:30:06 +01:00
2b9d06ac99
## 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 https://github.com/google/blockly-samples/issues/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 https://github.com/google/blockly-samples/issues/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`. |