From c426c6d820495d2b34dec7ac6a2fe62cfb02506c Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 1 Jul 2025 14:07:39 -0700 Subject: [PATCH] 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. --- core/focus_manager.ts | 20 +++++- core/toolbox/toolbox.ts | 2 + core/utils/focusable_tree_traverser.ts | 8 ++- tests/mocha/focus_manager_test.js | 48 +++++++++++++ tests/mocha/focusable_tree_traverser_test.js | 74 ++++++++++++++++++++ 5 files changed, 148 insertions(+), 4 deletions(-) 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();