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.
This commit is contained in:
Ben Henning
2025-05-12 16:36:23 -07:00
committed by GitHub
parent a1be83bad8
commit e7af75e051
2 changed files with 21 additions and 7 deletions

View File

@@ -32,13 +32,15 @@ export class FocusableTreeTraverser {
* @returns The IFocusableNode currently with focus, or null if none.
*/
static findFocusedNode(tree: IFocusableTree): IFocusableNode | null {
const root = tree.getRootFocusableNode().getFocusableElement();
const rootNode = tree.getRootFocusableNode();
if (!rootNode.canBeFocused()) return null;
const root = rootNode.getFocusableElement();
if (
dom.hasClass(root, FocusableTreeTraverser.ACTIVE_CLASS_NAME) ||
dom.hasClass(root, FocusableTreeTraverser.PASSIVE_CSS_CLASS_NAME)
) {
// The root has focus.
return tree.getRootFocusableNode();
return rootNode;
}
const activeEl = root.querySelector(this.ACTIVE_FOCUS_NODE_CSS_SELECTOR);
@@ -99,8 +101,9 @@ export class FocusableTreeTraverser {
}
// Second, check against the tree's root.
if (element === tree.getRootFocusableNode().getFocusableElement()) {
return tree.getRootFocusableNode();
const rootNode = tree.getRootFocusableNode();
if (rootNode.canBeFocused() && element === rootNode.getFocusableElement()) {
return rootNode;
}
// Third, check if the element has a node.

View File

@@ -2746,7 +2746,9 @@ export class WorkspaceSvg
const block = this.getBlockById(blockId);
if (block) {
for (const field of block.getFields()) {
if (field.getFocusableElement().id === id) return field;
if (field.canBeFocused() && field.getFocusableElement().id === id) {
return field;
}
}
}
return null;
@@ -2769,6 +2771,7 @@ export class WorkspaceSvg
for (const comment of this.getTopComments()) {
if (
comment instanceof RenderedWorkspaceComment &&
comment.canBeFocused() &&
comment.getFocusableElement().id === id
) {
return comment;
@@ -2780,10 +2783,18 @@ export class WorkspaceSvg
.map((block) => block.getIcons())
.flat();
for (const icon of icons) {
if (icon.getFocusableElement().id === id) return icon;
if (icon.canBeFocused() && icon.getFocusableElement().id === id) {
return icon;
}
if (hasBubble(icon)) {
const bubble = icon.getBubble();
if (bubble && bubble.getFocusableElement().id === id) return bubble;
if (
bubble &&
bubble.canBeFocused() &&
bubble.getFocusableElement().id === id
) {
return bubble;
}
}
}