Commit Graph

8 Commits

Author SHA1 Message Date
Ben Henning
2b9d06ac99 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 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`. |
2025-05-14 10:46:22 -07:00
Ben Henning
4074cee31b feat!: Make everything ISelectable focusable (#9004)
* feat!: Make bubbles, comments, and icons focusable

* feat!: Make ISelectable and ICopyable focusable.

* feat: Consolidate selection calls.

Now everything is based on focus with selection only being used as a
proxy.

* feat: Invert responsibility for setSelected().

Now setSelected() is only for quasi-external use.

* feat: Push up shadow check to getters.

Needed new common-level helper.

* chore: Lint fixes.

* feat!: Allow IFocusableNode to disable focus.

* chore: post-merge lint fixes

* fix: Fix tests + text bubble focusing.

This fixed then regressed a circular dependency causing the node and
advanced compilation steps to fail. This investigation is ongoing.

* fix: Clean up & fix imports.

This ensures the node and advanced compilation test steps now pass.

* fix: Lint fixes + revert commented out logic.

* chore: Remove unnecessary cast.

Addresses reviewer comment.

* fix: Some issues and a bunch of clean-ups.

This addresses a bunch of review comments, and fixes selecting workspace
comments.

* chore: Lint fix.

* fix: Remove unnecessary shadow consideration.

* chore: Revert import.

* chore: Some doc updates & added a warn statement.
2025-05-09 08:16:14 -07:00
Ben Henning
4e8bb9850f Revert "chore: Remove unused isFocusable*() functions."
This reverts commit 404c20eeaf.
2025-04-21 21:09:26 +00:00
Ben Henning
c91fed3fdb chore: equality + doc cleanups 2025-04-21 21:00:27 +00:00
Ben Henning
404c20eeaf chore: Remove unused isFocusable*() functions.
These were needed in previous versions of plugin changes, but aren't
anymore.
2025-04-21 20:55:02 +00:00
Ben Henning
0772a29824 feat!: Introduce new focus tree/node functions.
This introduces new callback methods for IFocusableTree and
IFocusableNode for providing a basis of synchronizing domain state with
focus changes. It also introduces support for implementations of
IFocusableTree to better manage initial state cases, especially when a
tree is focused using tab navigation.

FocusManager has also been updated to ensure functional parity between
tab-navigating to a tree and using focusTree() on that tree. This means
that tab navigating to a tree will actually restore focus back to that
tree's previous focused node rather than the root (unless the root is
navigated to from within the tree itself). This is meant to provide
better consistency between tab and non-tab keyboard navigation.

Note that these changes originally came from #8875 and are required for
later PRs that will introduce IFocusableNode and IFocusableTree
implementations.
2025-04-21 20:42:28 +00:00
Ben Henning
d9beacddb4 feat: add FocusManager
This is the bulk of the work for introducing the central logical unit
for managing and sychronizing focus as a first-class Blockly concept
with that of DOM focus.

There's a lot to do yet, including:
- Ensuring clicks within Blockly's scope correctly sync back to focus
  changes.
- Adding support for, and testing, cases when focus is lost from all
  registered trees.
- Testing nested tree propagation.
- Testing the traverser utility class.
- Adding implementations for IFocusableTree and IFocusableNode
  throughout Blockly.
2025-03-21 00:33:51 +00:00
Ben Henning
3ae422a566 feat: Add interfaces for focus management.
Introduces the necessary base interfaces for representing different
focusable contexts within Blockly. The actual logic for utilizing and
implementing these interfaces will come in later PRs.
2025-02-19 23:03:21 +00:00