Commit Graph

7 Commits

Author SHA1 Message Date
Ben Henning
c426c6d820 fix: Short-circuit node lookups for missing IDs (#9174)
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Fixes #9155

### Proposed Changes

In cases when an ID is missing for an element passed to `FocusableTreeTraverser.findFocusableNodeFor()`, always return `null`.

Additionally, the new short-circuit logic exposed that `Toolbox` actually wasn't being set up correctly (that is, its root element was not being configured with a valid ID). This has been fixed.

### Reason for Changes

These are cases when a valid node should never be matched (and it's technically possible to incorrectly match if an `IFocusableNode` is set up incorrectly and is providing a focusable element with an unset ID). This avoids the extra computation time of potentially calling deep into `WorkspaceSvg` and exploring all possible nodes for an ID that should never match.

Note that there is a weird quirk with `null` IDs actually being the string `"null"`. This is a side effect of how `setAttribute` and attributes in general work with HTML elements. There's nothing really that can be done here, so it's now considered invalid to also have an ID of string `"null"` just to ensure the `null` case is properly short-circuited.

Finally, the issue with toolbox being configured incorrectly was discovered with the introducing of a new hard failure in `FocusManager.registerTree()` when a tree with an invalid root element is registered. From testing there are no other such trees that need to be updated.

A new warning was also added if `focusNode()` is used on a node with an element that has an invalid ID. This isn't a hard failure to follow the convention of other invalid `focusNode()` situations. It's much more fragile for `focusNode()` to throw than `registerTree()` since the former generally happens much earlier in a page lifecycle, and is less prone to dynamic behaviors.

### Test Coverage

New tests were added to validate the various empty ID cases for `FocusableTreeTraverser.findFocusableNodeFor()`, and to validate the new error check for `FocusManager.registerTree()`.

### Documentation

No new documentation should be needed.

### Additional Information

Nothing to add.
2025-07-01 14:07:39 -07:00
Ben Henning
e7af75e051 fix: Improve robustness of IFocusableNode uses (#9031)
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Fixes https://github.com/google/blockly-keyboard-experimentation/issues/515

### Proposed Changes

This adds `canBeFocused()` checks to all the places that could currently cause problems if a node were to return `false`.

### Reason for Changes

This can't introduce a problem in current Core and, in fact, most of these classes can never return `false` even through subclasses. However, this adds better robustness and fixes the underlying issue by ensuring that `getFocusableElement()` isn't called for a node that has indicated it cannot be focused.

### Test Coverage

I've manually tested this through the keyboard navigation plugin. However, there are clearly additional tests that would be nice to add both for the traverser and for `WorkspaceSvg`, both likely as part of resolving #8915.

### Documentation

No new documentation changes should be needed here.

### Additional Information

This is fixing theoretical issues in Core, but a real issue tracked by the keyboard navigation plugin repository.
2025-05-12 23:36:23 +00:00
Ben Henning
c5404af82e chore: lint fixes. 2025-04-03 23:04:06 +00:00
Ben Henning
720e8dab2b chore: part 2 of addressing reviewer comments. 2025-04-03 22:55:35 +00:00
Ben Henning
902b26b1a1 chore: part 1 of addressing reviewer comments. 2025-04-03 22:25:50 +00:00
Ben Henning
516e3af936 feat: finish core impl + tests
This adds new tests for the FocusableTreeTraverser and fixes a number of
issues with the original implementation (one of which required two new
API methods to be added to IFocusableTree). More tests have also been
added for FocusManager, and defocusing tracked nodes/trees has been
fully implemented in FocusManager.
2025-03-27 21:57:30 +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