Skip to content

Commit e7af75e

Browse files
authored
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 RaspberryPiFoundation/blockly-keyboard-experimentation#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.
1 parent a1be83b commit e7af75e

File tree

2 files changed

+21
-7
lines changed

2 files changed

+21
-7
lines changed

core/utils/focusable_tree_traverser.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,15 @@ export class FocusableTreeTraverser {
3232
* @returns The IFocusableNode currently with focus, or null if none.
3333
*/
3434
static findFocusedNode(tree: IFocusableTree): IFocusableNode | null {
35-
const root = tree.getRootFocusableNode().getFocusableElement();
35+
const rootNode = tree.getRootFocusableNode();
36+
if (!rootNode.canBeFocused()) return null;
37+
const root = rootNode.getFocusableElement();
3638
if (
3739
dom.hasClass(root, FocusableTreeTraverser.ACTIVE_CLASS_NAME) ||
3840
dom.hasClass(root, FocusableTreeTraverser.PASSIVE_CSS_CLASS_NAME)
3941
) {
4042
// The root has focus.
41-
return tree.getRootFocusableNode();
43+
return rootNode;
4244
}
4345

4446
const activeEl = root.querySelector(this.ACTIVE_FOCUS_NODE_CSS_SELECTOR);
@@ -99,8 +101,9 @@ export class FocusableTreeTraverser {
99101
}
100102

101103
// Second, check against the tree's root.
102-
if (element === tree.getRootFocusableNode().getFocusableElement()) {
103-
return tree.getRootFocusableNode();
104+
const rootNode = tree.getRootFocusableNode();
105+
if (rootNode.canBeFocused() && element === rootNode.getFocusableElement()) {
106+
return rootNode;
104107
}
105108

106109
// Third, check if the element has a node.

core/workspace_svg.ts

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2746,7 +2746,9 @@ export class WorkspaceSvg
27462746
const block = this.getBlockById(blockId);
27472747
if (block) {
27482748
for (const field of block.getFields()) {
2749-
if (field.getFocusableElement().id === id) return field;
2749+
if (field.canBeFocused() && field.getFocusableElement().id === id) {
2750+
return field;
2751+
}
27502752
}
27512753
}
27522754
return null;
@@ -2769,6 +2771,7 @@ export class WorkspaceSvg
27692771
for (const comment of this.getTopComments()) {
27702772
if (
27712773
comment instanceof RenderedWorkspaceComment &&
2774+
comment.canBeFocused() &&
27722775
comment.getFocusableElement().id === id
27732776
) {
27742777
return comment;
@@ -2780,10 +2783,18 @@ export class WorkspaceSvg
27802783
.map((block) => block.getIcons())
27812784
.flat();
27822785
for (const icon of icons) {
2783-
if (icon.getFocusableElement().id === id) return icon;
2786+
if (icon.canBeFocused() && icon.getFocusableElement().id === id) {
2787+
return icon;
2788+
}
27842789
if (hasBubble(icon)) {
27852790
const bubble = icon.getBubble();
2786-
if (bubble && bubble.getFocusableElement().id === id) return bubble;
2791+
if (
2792+
bubble &&
2793+
bubble.canBeFocused() &&
2794+
bubble.getFocusableElement().id === id
2795+
) {
2796+
return bubble;
2797+
}
27872798
}
27882799
}
27892800

0 commit comments

Comments
 (0)