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.

Author: BenHenning

core/focus_manager.ts

@@ -174,8 +174,15 @@ export class FocusManager {
     this.registeredTrees.push(
       new TreeRegistration(tree, rootShouldBeAutoTabbable),
     );
+    const rootElement = tree.getRootFocusableNode().getFocusableElement();
+    if (!rootElement.id || rootElement.id === 'null') {
+      throw Error(
+        `Attempting to register a tree with a root element that has an ` +
+          `invalid ID: ${tree}.`,
+      );
+    }
     if (rootShouldBeAutoTabbable) {
-      tree.getRootFocusableNode().getFocusableElement().tabIndex = 0;
+      rootElement.tabIndex = 0;
     }
   }
 
@@ -344,13 +351,22 @@ export class FocusManager {
       throw Error(`Attempted to focus unregistered node: ${focusableNode}.`);
     }
 
+    const focusableNodeElement = focusableNode.getFocusableElement();
+    if (!focusableNodeElement.id || focusableNodeElement.id === 'null') {
+      // Warn that the ID is invalid, but continue execution since an invalid ID
+      // will result in an unmatched (null) node. Since a request to focus
+      // something was initiated, the code below will attempt to find the next
+      // best thing to focus, instead.
+      console.warn('Trying to focus a node that has an invalid ID.');
+    }
+
     // Safety check for ensuring focusNode() doesn't get called for a node that
     // isn't actually hooked up to its parent tree correctly. This usually
     // happens when calls to focusNode() interleave with asynchronous clean-up
     // operations (which can happen due to ephemeral focus and in other cases).
     // Fall back to a reasonable default since there's no valid node to focus.
     const matchedNode = FocusableTreeTraverser.findFocusableNodeFor(
-      focusableNode.getFocusableElement(),
+      focusableNodeElement,
       nextTree,
     );
     const prevNodeNextTree = FocusableTreeTraverser.findFocusedNode(nextTree);

core/toolbox/toolbox.ts

@@ -43,6 +43,7 @@ import type {KeyboardShortcut} from '../shortcut_registry.js';
 import * as Touch from '../touch.js';
 import * as aria from '../utils/aria.js';
 import * as dom from '../utils/dom.js';
+import * as idGenerator from '../utils/idgenerator.js';
 import {Rect} from '../utils/rect.js';
 import * as toolbox from '../utils/toolbox.js';
 import type {WorkspaceSvg} from '../workspace_svg.js';
@@ -185,6 +186,7 @@ export class Toolbox
     const svg = workspace.getParentSvg();
 
     const container = this.createContainer_();
+    container.id = idGenerator.getNextUniqueId();
 
     this.contentsDiv_ = this.createContentsContainer_();
     aria.setRole(this.contentsDiv_, aria.Role.TREE);

core/utils/focusable_tree_traverser.ts

@@ -79,8 +79,8 @@ export class FocusableTreeTraverser {
    * traversed but its nodes will never be returned here per the contract of
    * IFocusableTree.lookUpFocusableNode.
    *
-   * The provided element must have a non-null ID that conforms to the contract
-   * mentioned in IFocusableNode.
+   * The provided element must have a non-null, non-empty ID that conforms to
+   * the contract mentioned in IFocusableNode.
    *
    * @param element The HTML or SVG element being sought.
    * @param tree The tree under which the provided element may be a descendant.
@@ -90,6 +90,10 @@ export class FocusableTreeTraverser {
     element: HTMLElement | SVGElement,
     tree: IFocusableTree,
   ): IFocusableNode | null {
+    // Note that the null check is due to Element.setAttribute() converting null
+    // to a string.
+    if (!element.id || element.id === 'null') return null;
+
     // First, match against subtrees.
     const subTreeMatches = tree.getNestedTrees().map((tree) => {
       return FocusableTreeTraverser.findFocusableNodeFor(element, tree);

tests/mocha/focus_manager_test.js

@@ -249,6 +249,54 @@ suite('FocusManager', function () {
       // The second register should not fail since the tree was previously unregistered.
     });
 
+    test('for tree with missing ID throws error', function () {
+      const rootNode = this.testFocusableTree1.getRootFocusableNode();
+      const rootElem = rootNode.getFocusableElement();
+      const oldId = rootElem.id;
+      rootElem.removeAttribute('id');
+
+      const errorMsgRegex =
+        /Attempting to register a tree with a root element that has an invalid ID.+?/;
+      assert.throws(
+        () => this.focusManager.registerTree(this.testFocusableTree1),
+        errorMsgRegex,
+      );
+      // Restore the ID for other tests.
+      rootElem.id = oldId;
+    });
+
+    test('for tree with null ID throws error', function () {
+      const rootNode = this.testFocusableTree1.getRootFocusableNode();
+      const rootElem = rootNode.getFocusableElement();
+      const oldId = rootElem.id;
+      rootElem.setAttribute('id', null);
+
+      const errorMsgRegex =
+        /Attempting to register a tree with a root element that has an invalid ID.+?/;
+      assert.throws(
+        () => this.focusManager.registerTree(this.testFocusableTree1),
+        errorMsgRegex,
+      );
+      // Restore the ID for other tests.
+      rootElem.id = oldId;
+    });
+
+    test('for tree with empty throws error', function () {
+      const rootNode = this.testFocusableTree1.getRootFocusableNode();
+      const rootElem = rootNode.getFocusableElement();
+      const oldId = rootElem.id;
+      rootElem.setAttribute('id', '');
+
+      const errorMsgRegex =
+        /Attempting to register a tree with a root element that has an invalid ID.+?/;
+      assert.throws(
+        () => this.focusManager.registerTree(this.testFocusableTree1),
+        errorMsgRegex,
+      );
+      // Restore the ID for other tests.
+      rootElem.id = oldId;
+    });
+
     test('for unmanaged tree does not overwrite tab index', function () {
       this.focusManager.registerTree(this.testFocusableTree1, false);
 

tests/mocha/focusable_tree_traverser_test.js

@@ -348,6 +348,80 @@ suite('FocusableTreeTraverser', function () {
   });
 
   suite('findFocusableNodeFor()', function () {
+    test('for element without ID returns null', function () {
+      const tree = this.testFocusableTree1;
+      const rootNode = tree.getRootFocusableNode();
+      const rootElem = rootNode.getFocusableElement();
+      const oldId = rootElem.id;
+      // Normally it's not valid to miss an ID, but it can realistically happen.
+      rootElem.removeAttribute('id');
+
+      const finding = FocusableTreeTraverser.findFocusableNodeFor(
+        rootElem,
+        tree,
+      );
+      // Restore the ID for other tests.
+      rootElem.setAttribute('id', oldId);
+
+      assert.isNull(finding);
+    });
+
+    test('for element with null ID returns null', function () {
+      const tree = this.testFocusableTree1;
+      const rootNode = tree.getRootFocusableNode();
+      const rootElem = rootNode.getFocusableElement();
+      const oldId = rootElem.id;
+      // Normally it's not valid to miss an ID, but it can realistically happen.
+      rootElem.setAttribute('id', null);
+
+      const finding = FocusableTreeTraverser.findFocusableNodeFor(
+        rootElem,
+        tree,
+      );
+      // Restore the ID for other tests.
+      rootElem.setAttribute('id', oldId);
+
+      assert.isNull(finding);
+    });
+
+    test('for element with null ID string returns null', function () {
+      const tree = this.testFocusableTree1;
+      const rootNode = tree.getRootFocusableNode();
+      const rootElem = rootNode.getFocusableElement();
+      const oldId = rootElem.id;
+      // This is a quirky version of the null variety above that's actually
+      // functionallity equivalent (since 'null' is converted to a string).
+      rootElem.setAttribute('id', 'null');
+
+      const finding = FocusableTreeTraverser.findFocusableNodeFor(
+        rootElem,
+        tree,
+      );
+      // Restore the ID for other tests.
+      rootElem.setAttribute('id', oldId);
+
+      assert.isNull(finding);
+    });
+
+    test('for element with empty ID returns null', function () {
+      const tree = this.testFocusableTree1;
+      const rootNode = tree.getRootFocusableNode();
+      const rootElem = rootNode.getFocusableElement();
+      const oldId = rootElem.id;
+      // An empty ID is invalid since it will potentially conflict with other
+      // elements, and element IDs must be unique for focus management.
+      rootElem.setAttribute('id', '');
+
+      const finding = FocusableTreeTraverser.findFocusableNodeFor(
+        rootElem,
+        tree,
+      );
+      // Restore the ID for other tests.
+      rootElem.setAttribute('id', oldId);
+
+      assert.isNull(finding);
+    });
+
     test('for root element returns root', function () {
       const tree = this.testFocusableTree1;
       const rootNode = tree.getRootFocusableNode();