fix: Short-circuit node lookups for missing IDs

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();