From e7af75e051858dccf5efc317852788a69013cb14 Mon Sep 17 00:00:00 2001
From: Ben Henning <bhenning@google.com>
Date: Mon, 12 May 2025 16:36:23 -0700
Subject: [PATCH] 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.
---
 core/utils/focusable_tree_traverser.ts | 11 +++++++----
 core/workspace_svg.ts                  | 17 ++++++++++++++---
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/core/utils/focusable_tree_traverser.ts b/core/utils/focusable_tree_traverser.ts
index 94603edd0..916437b6a 100644
--- a/core/utils/focusable_tree_traverser.ts
+++ b/core/utils/focusable_tree_traverser.ts
@@ -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.
diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts
index dd8cac242..3095f0924 100644
--- a/core/workspace_svg.ts
+++ b/core/workspace_svg.ts
@@ -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;
+        }
       }
     }