diff --git a/core/focus_manager.ts b/core/focus_manager.ts index 3d0a9347f..02e059107 100644 --- a/core/focus_manager.ts +++ b/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); diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index 57e849ce2..31bb2b636 100644 --- a/core/toolbox/toolbox.ts +++ b/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); diff --git a/core/utils/focusable_tree_traverser.ts b/core/utils/focusable_tree_traverser.ts index 916437b6a..aa4585b82 100644 --- a/core/utils/focusable_tree_traverser.ts +++ b/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); diff --git a/tests/mocha/focus_manager_test.js b/tests/mocha/focus_manager_test.js index 3a1fc98a7..26dcb8dbe 100644 --- a/tests/mocha/focus_manager_test.js +++ b/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); diff --git a/tests/mocha/focusable_tree_traverser_test.js b/tests/mocha/focusable_tree_traverser_test.js index 66cc598cc..0f88e1106 100644 --- a/tests/mocha/focusable_tree_traverser_test.js +++ b/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();