fix: Ensure BlockSvg is uniquely focusable on the page #9045
Conversation
This replaces using BlockSvg's own ID since that's not guaranteed to be unique across all workspaces on the page.
|
PTAL @maribethb though be aware that there's an open design question that I'd like input on, and additional testing to do once google/blockly-keyboard-experimentation#520 is merged. |
There was a problem hiding this comment.
I think I would use the first option in your design question. The reason being that it might be possible to have globally unique block IDs in the future and then we could just revert this change.
We actually tried to have globally unique block ids in a previous release, but code.org was relying on the current behavior in their tests so it became a blocking/breaking change for them. I think Mike said he was working on their tests such that that would no longer be a blocker. See #7432 for historic context
As an aside, I think the correct behavior for the minimap would be to select the block in the main workspace when a block in the minimap is selected and I wonder if that's now possible with navigation policies?
Ah, that's very helpful. In that case things stay as-is since the first option is the current status quo. And completely agree on the possibilities to simplify this in the future if truly global IDs can be reintroduced. As for the selection bit, do you want me to file a bug on that? It should be possible purely with focus manager actually, but it would need to be a change in the plugin (and I'm uncertain the extent of the changes without digging into the plugin code more, but it might not be too complicated). |
|
Confirming that this works well against the keyboard navigation plugin tip-of-tree. |
The basics
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
FocusManageruses 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
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:BlockSvg.idWorkspaceSvg.getBlockById.WorkspaceSvg.lookUpFocusableNodenow uses 2 different IDs.BlockSvg.focusIdWorkspaceSvg.lookUpFocusableNode.