From 720e8dab2b2786887e2d0dbabb8a7e64d4a68d64 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 3 Apr 2025 22:55:35 +0000 Subject: [PATCH] chore: part 2 of addressing reviewer comments. --- core/focus_manager.ts | 35 ++++++-- core/interfaces/i_focusable_tree.ts | 32 ++----- core/utils/focusable_tree_traverser.ts | 39 ++++++--- tests/mocha/focus_manager_test.js | 89 ++++++-------------- tests/mocha/focusable_tree_traverser_test.js | 8 -- 5 files changed, 89 insertions(+), 114 deletions(-) diff --git a/core/focus_manager.ts b/core/focus_manager.ts index f0cc32b74..228f659a8 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -6,6 +6,7 @@ import type {IFocusableNode} from './interfaces/i_focusable_node.js'; import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; +import {FocusableTreeTraverser} from './utils/focusable_tree_traverser.js'; import * as dom from './utils/dom.js'; /** @@ -30,7 +31,29 @@ export type ReturnEphemeralFocus = () => void; * focusNode(). */ export class FocusManager { + /** + * The CSS class assigned to IFocusableNode elements that presently have + * active DOM and Blockly focus. + * + * This should never be used directly. Instead, rely on FocusManager to ensure + * nodes have active focus (either automatically through DOM focus or manually + * through the various focus* methods provided by this class). + * + * It's recommended to not query using this class name, either. Instead, use + * FocusableTreeTraverser or IFocusableTree's methods to find a specific node. + */ static readonly ACTIVE_FOCUS_NODE_CSS_CLASS_NAME = 'blocklyActiveFocus'; + + /** + * The CSS class assigned to IFocusableNode elements that presently have + * passive focus (that is, they were the most recent node in their relative + * tree to have active focus--see ACTIVE_FOCUS_NODE_CSS_CLASS_NAME--and will + * receive active focus again if their surrounding tree is requested to become + * focused, i.e. using focusTree below). + * + * See ACTIVE_FOCUS_NODE_CSS_CLASS_NAME for caveats and limitations around + * using this constant directly (generally it never should need to be used). + */ static readonly PASSIVE_FOCUS_NODE_CSS_CLASS_NAME = 'blocklyPassiveFocus'; focusedNode: IFocusableNode | null = null; @@ -57,7 +80,8 @@ export class FocusManager { // updated. Per the contract of findFocusableNodeFor only one tree // should claim the element. for (const tree of this.registeredTrees) { - newNode = tree.findFocusableNodeFor(activeElement); + newNode = FocusableTreeTraverser.findFocusableNodeFor( + activeElement, tree); if (newNode) break; } } @@ -113,7 +137,7 @@ export class FocusManager { const treeIndex = this.registeredTrees.findIndex((tree) => tree === tree); this.registeredTrees.splice(treeIndex, 1); - const focusedNode = tree.getFocusedNode(); + const focusedNode = FocusableTreeTraverser.findFocusedNode(tree); const root = tree.getRootFocusableNode(); if (focusedNode) this.removeHighlight(focusedNode); if (this.focusedNode === focusedNode || this.focusedNode === root) { @@ -169,9 +193,8 @@ export class FocusManager { if (!this.isRegistered(focusableTree)) { throw Error(`Attempted to focus unregistered tree: ${focusableTree}.`); } - this.focusNode( - focusableTree.getFocusedNode() ?? focusableTree.getRootFocusableNode(), - ); + const currNode = FocusableTreeTraverser.findFocusedNode(focusableTree); + this.focusNode(currNode ?? focusableTree.getRootFocusableNode()); } /** @@ -193,7 +216,7 @@ export class FocusManager { this.setNodeToPassive(prevNode); } // If there's a focused node in the new node's tree, ensure it's reset. - const prevNodeNextTree = nextTree.getFocusedNode(); + const prevNodeNextTree = FocusableTreeTraverser.findFocusedNode(nextTree); const nextTreeRoot = nextTree.getRootFocusableNode(); if (prevNodeNextTree) { this.removeHighlight(prevNodeNextTree); diff --git a/core/interfaces/i_focusable_tree.ts b/core/interfaces/i_focusable_tree.ts index 9cedba732..bc0c38849 100644 --- a/core/interfaces/i_focusable_tree.ts +++ b/core/interfaces/i_focusable_tree.ts @@ -20,17 +20,14 @@ import type {IFocusableNode} from './i_focusable_node.js'; * page at any given time). The idea of passive focus is to provide context to * users on where their focus will be restored upon navigating back to a * previously focused tree. + * + * Note that if the tree's current focused node (passive or active) is needed, + * FocusableTreeTraverser.findFocusedNode can be used. + * + * Note that if specific nodes are needed to be retrieved for this tree, either + * use lookUpFocusableNode or FocusableTreeTraverser.findFocusableNodeFor. */ export interface IFocusableTree { - /** - * Returns the current node with focus in this tree, or null if none (or if - * the root has focus). - * - * Note that this will never return a node from a nested sub-tree as that tree - * should specifically be called in order to retrieve its focused node. - */ - getFocusedNode(): IFocusableNode | null; - /** * Returns the top-level focusable node of the tree. * @@ -61,21 +58,4 @@ export interface IFocusableTree { * @param id The ID of the node's focusable HTMLElement or SVGElement. */ lookUpFocusableNode(id: string): IFocusableNode | null; - - /** - * Returns the IFocusableNode corresponding to the select element, or null if - * the element does not have such a node. - * - * The provided element must have a non-null ID that conforms to the contract - * mentioned in IFocusableNode. - * - * This function may match against the root node of the tree. It will also map - * against the nearest node to the provided element if the element does not - * have an exact matching corresponding node. This function filters out - * matches against nested trees, so long as they are represented in the return - * value of getNestedTrees. - */ - findFocusableNodeFor( - element: HTMLElement | SVGElement, - ): IFocusableNode | null; } diff --git a/core/utils/focusable_tree_traverser.ts b/core/utils/focusable_tree_traverser.ts index 8061e981b..6ea95b0b0 100644 --- a/core/utils/focusable_tree_traverser.ts +++ b/core/utils/focusable_tree_traverser.ts @@ -4,7 +4,6 @@ * SPDX-License-Identifier: Apache-2.0 */ -import {FocusManager} from '../focus_manager.js'; import type {IFocusableNode} from '../interfaces/i_focusable_node.js'; import type {IFocusableTree} from '../interfaces/i_focusable_tree.js'; import * as dom from '../utils/dom.js'; @@ -14,24 +13,31 @@ import * as dom from '../utils/dom.js'; * tree traversals. */ export class FocusableTreeTraverser { - static readonly ACTIVE_FOCUS_NODE_CSS_SELECTOR = `.${FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME}`; - static readonly PASSIVE_FOCUS_NODE_CSS_SELECTOR = `.${FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME}`; + private static readonly ACTIVE_CLASS_NAME = 'blocklyActiveFocus'; + private static readonly PASSIVE_CSS_CLASS_NAME = 'blocklyPassiveFocus'; + private static readonly ACTIVE_FOCUS_NODE_CSS_SELECTOR = ( + `.${FocusableTreeTraverser.ACTIVE_CLASS_NAME}`); + private static readonly PASSIVE_FOCUS_NODE_CSS_SELECTOR = ( + `.${FocusableTreeTraverser.PASSIVE_CSS_CLASS_NAME}`); /** - * Returns the current IFocusableNode that either has the CSS class - * 'blocklyActiveFocus' or 'blocklyPassiveFocus', only considering HTML and - * SVG elements. + * Returns the current IFocusableNode that is styled (and thus represented) as + * having either passive or active focus, only considering HTML and SVG + * elements. * * This can match against the tree's root. * + * Note that this will never return a node from a nested sub-tree as that tree + * should specifically be used to retrieve its focused node. + * * @param tree The IFocusableTree in which to search for a focused node. * @returns The IFocusableNode currently with focus, or null if none. */ static findFocusedNode(tree: IFocusableTree): IFocusableNode | null { const root = tree.getRootFocusableNode().getFocusableElement(); if ( - dom.hasClass(root, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME) || - dom.hasClass(root, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME) + dom.hasClass(root, FocusableTreeTraverser.ACTIVE_CLASS_NAME) || + dom.hasClass(root, FocusableTreeTraverser.PASSIVE_CSS_CLASS_NAME) ) { // The root has focus. return tree.getRootFocusableNode(); @@ -39,7 +45,8 @@ export class FocusableTreeTraverser { const activeEl = root.querySelector(this.ACTIVE_FOCUS_NODE_CSS_SELECTOR); if (activeEl instanceof HTMLElement || activeEl instanceof SVGElement) { - const active = tree.findFocusableNodeFor(activeEl); + const active = FocusableTreeTraverser.findFocusableNodeFor( + activeEl, tree); if (active) return active; } @@ -47,7 +54,8 @@ export class FocusableTreeTraverser { // subtrees). const passiveEl = root.querySelector(this.PASSIVE_FOCUS_NODE_CSS_SELECTOR); if (passiveEl instanceof HTMLElement || passiveEl instanceof SVGElement) { - const passive = tree.findFocusableNodeFor(passiveEl); + const passive = FocusableTreeTraverser.findFocusableNodeFor( + passiveEl, tree); if (passive) return passive; } @@ -59,10 +67,17 @@ export class FocusableTreeTraverser { * element iff it's the root element or a descendent of the root element of * the specified IFocusableTree. * + * If the element exists within the specified tree's DOM structure but does + * not directly correspond to a node, the nearest parent node (or the tree's + * root) will be returned to represent the provided element. + * * If the tree contains another nested IFocusableTree, the nested tree may be * 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. + * * @param element The HTML or SVG element being sought. * @param tree The tree under which the provided element may be a descendant. * @returns The matching IFocusableNode, or null if there is no match. @@ -74,7 +89,9 @@ export class FocusableTreeTraverser { // First, match against subtrees. const subTreeMatches = tree .getNestedTrees() - .map((tree) => tree.findFocusableNodeFor(element)); + .map((tree) => { + return FocusableTreeTraverser.findFocusableNodeFor(element, tree); + }); if (subTreeMatches.findIndex((match) => !!match) !== -1) { // At least one subtree has a match for the element so it cannot be part // of the outer tree. diff --git a/tests/mocha/focus_manager_test.js b/tests/mocha/focus_manager_test.js index fa8f12083..f61e9d371 100644 --- a/tests/mocha/focus_manager_test.js +++ b/tests/mocha/focus_manager_test.js @@ -8,7 +8,6 @@ import { FocusManager, getFocusManager, } from '../../build/src/core/focus_manager.js'; -import {FocusableTreeTraverser} from '../../build/src/core/utils/focusable_tree_traverser.js'; import {assert} from '../../node_modules/chai/chai.js'; import { sharedTestSetup, @@ -43,10 +42,6 @@ class FocusableTreeImpl { return node; } - getFocusedNode() { - return FocusableTreeTraverser.findFocusedNode(this); - } - getRootFocusableNode() { return this.rootNode; } @@ -58,13 +53,14 @@ class FocusableTreeImpl { lookUpFocusableNode(id) { return this.idToNodeMap[id]; } - - findFocusableNodeFor(element) { - return FocusableTreeTraverser.findFocusableNodeFor(element, this); - } } suite('FocusManager', function () { + const ACTIVE_FOCUS_NODE_CSS_SELECTOR = ( + `.${FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME}`); + const PASSIVE_FOCUS_NODE_CSS_SELECTOR = ( + `.${FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME}`); + setup(function () { sharedTestSetup.call(this); @@ -160,35 +156,15 @@ suite('FocusManager', function () { const eventListener = this.globalDocumentEventListener; document.removeEventListener(eventType, eventListener); - const removeFocusIndicators = function (element) { - element.classList.remove(FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME); - }; - // Ensure all node CSS styles are reset so that state isn't leaked between tests. - removeFocusIndicators(document.getElementById('testFocusableTree1')); - removeFocusIndicators(document.getElementById('testFocusableTree1.node1')); - removeFocusIndicators( - document.getElementById('testFocusableTree1.node1.child1'), - ); - removeFocusIndicators(document.getElementById('testFocusableTree1.node2')); - removeFocusIndicators(document.getElementById('testFocusableTree2')); - removeFocusIndicators(document.getElementById('testFocusableTree2.node1')); - removeFocusIndicators(document.getElementById('testFocusableNestedTree4')); - removeFocusIndicators( - document.getElementById('testFocusableNestedTree4.node1'), - ); - removeFocusIndicators(document.getElementById('testFocusableGroup1')); - removeFocusIndicators(document.getElementById('testFocusableGroup1.node1')); - removeFocusIndicators( - document.getElementById('testFocusableGroup1.node1.child1'), - ); - removeFocusIndicators(document.getElementById('testFocusableGroup1.node2')); - removeFocusIndicators(document.getElementById('testFocusableGroup2')); - removeFocusIndicators(document.getElementById('testFocusableGroup2.node1')); - removeFocusIndicators(document.getElementById('testFocusableNestedGroup4')); - removeFocusIndicators( - document.getElementById('testFocusableNestedGroup4.node1'), - ); + const activeElems = document.querySelectorAll(ACTIVE_FOCUS_NODE_CSS_SELECTOR); + const passiveElems = document.querySelectorAll(PASSIVE_FOCUS_NODE_CSS_SELECTOR); + for (const elem of activeElems) { + elem.classList.remove(FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME); + } + for (const elem of passiveElems) { + elem.classList.remove(FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME); + } // Reset the current active element. document.body.focus(); @@ -4353,12 +4329,10 @@ suite('FocusManager', function () { // Taking focus without an existing node having focus should change no focus indicators. const activeElems = Array.from( - document.querySelectorAll( - FocusableTreeTraverser.ACTIVE_FOCUS_NODE_CSS_SELECTOR), + document.querySelectorAll(ACTIVE_FOCUS_NODE_CSS_SELECTOR), ); const passiveElems = Array.from( - document.querySelectorAll( - FocusableTreeTraverser.PASSIVE_FOCUS_NODE_CSS_SELECTOR), + document.querySelectorAll(PASSIVE_FOCUS_NODE_CSS_SELECTOR), ); assert.isEmpty(activeElems); assert.isEmpty(passiveElems); @@ -4376,12 +4350,10 @@ suite('FocusManager', function () { // Taking focus without an existing node having focus should change no focus indicators. const activeElems = Array.from( - document.querySelectorAll( - FocusableTreeTraverser.ACTIVE_FOCUS_NODE_CSS_SELECTOR), + document.querySelectorAll(ACTIVE_FOCUS_NODE_CSS_SELECTOR), ); const passiveElems = Array.from( - document.querySelectorAll( - FocusableTreeTraverser.PASSIVE_FOCUS_NODE_CSS_SELECTOR), + document.querySelectorAll(PASSIVE_FOCUS_NODE_CSS_SELECTOR), ); assert.isEmpty(activeElems); assert.strictEqual(passiveElems.length, 1); @@ -4450,8 +4422,7 @@ suite('FocusManager', function () { this.testFocusableGroup2, ); const activeElems = Array.from( - document.querySelectorAll( - FocusableTreeTraverser.ACTIVE_FOCUS_NODE_CSS_SELECTOR), + document.querySelectorAll(ACTIVE_FOCUS_NODE_CSS_SELECTOR), ); assert.isEmpty(activeElems); }); @@ -4472,8 +4443,7 @@ suite('FocusManager', function () { this.testFocusableGroup2Node1, ); const activeElems = Array.from( - document.querySelectorAll( - FocusableTreeTraverser.ACTIVE_FOCUS_NODE_CSS_SELECTOR), + document.querySelectorAll(ACTIVE_FOCUS_NODE_CSS_SELECTOR), ); assert.isEmpty(activeElems); }); @@ -4497,8 +4467,7 @@ suite('FocusManager', function () { this.testFocusableGroup2Node1, ); const activeElems = Array.from( - document.querySelectorAll( - FocusableTreeTraverser.ACTIVE_FOCUS_NODE_CSS_SELECTOR), + document.querySelectorAll(ACTIVE_FOCUS_NODE_CSS_SELECTOR), ); assert.isEmpty(activeElems); }); @@ -4516,12 +4485,10 @@ suite('FocusManager', function () { // Finishing ephemeral focus without a previously focused node should not change indicators. const activeElems = Array.from( - document.querySelectorAll( - FocusableTreeTraverser.ACTIVE_FOCUS_NODE_CSS_SELECTOR), + document.querySelectorAll(ACTIVE_FOCUS_NODE_CSS_SELECTOR), ); const passiveElems = Array.from( - document.querySelectorAll( - FocusableTreeTraverser.PASSIVE_FOCUS_NODE_CSS_SELECTOR), + document.querySelectorAll(PASSIVE_FOCUS_NODE_CSS_SELECTOR), ); assert.isEmpty(activeElems); assert.isEmpty(passiveElems); @@ -4577,8 +4544,7 @@ suite('FocusManager', function () { // The original focused node should be restored. const nodeElem = this.testFocusableTree2Node1.getFocusableElement(); const activeElems = Array.from( - document.querySelectorAll( - FocusableTreeTraverser.ACTIVE_FOCUS_NODE_CSS_SELECTOR), + document.querySelectorAll(ACTIVE_FOCUS_NODE_CSS_SELECTOR), ); assert.strictEqual( this.focusManager.getFocusedNode(), @@ -4608,8 +4574,7 @@ suite('FocusManager', function () { .getRootFocusableNode() .getFocusableElement(); const activeElems = Array.from( - document.querySelectorAll( - FocusableTreeTraverser.ACTIVE_FOCUS_NODE_CSS_SELECTOR), + document.querySelectorAll(ACTIVE_FOCUS_NODE_CSS_SELECTOR), ); assert.strictEqual( this.focusManager.getFocusedTree(), @@ -4637,8 +4602,7 @@ suite('FocusManager', function () { // end of the ephemeral flow. const nodeElem = this.testFocusableGroup2Node1.getFocusableElement(); const activeElems = Array.from( - document.querySelectorAll( - FocusableTreeTraverser.ACTIVE_FOCUS_NODE_CSS_SELECTOR), + document.querySelectorAll(ACTIVE_FOCUS_NODE_CSS_SELECTOR), ); assert.strictEqual( this.focusManager.getFocusedNode(), @@ -4666,8 +4630,7 @@ suite('FocusManager', function () { // end of the ephemeral flow. const nodeElem = this.testFocusableGroup2Node1.getFocusableElement(); const activeElems = Array.from( - document.querySelectorAll( - FocusableTreeTraverser.ACTIVE_FOCUS_NODE_CSS_SELECTOR), + document.querySelectorAll(ACTIVE_FOCUS_NODE_CSS_SELECTOR), ); assert.strictEqual( this.focusManager.getFocusedNode(), diff --git a/tests/mocha/focusable_tree_traverser_test.js b/tests/mocha/focusable_tree_traverser_test.js index 00b2c5390..59eae38c1 100644 --- a/tests/mocha/focusable_tree_traverser_test.js +++ b/tests/mocha/focusable_tree_traverser_test.js @@ -40,10 +40,6 @@ class FocusableTreeImpl { return node; } - getFocusedNode() { - throw Error('Unused in test suite.'); - } - getRootFocusableNode() { return this.rootNode; } @@ -55,10 +51,6 @@ class FocusableTreeImpl { lookUpFocusableNode(id) { return this.idToNodeMap[id]; } - - findFocusableNodeFor(element) { - return FocusableTreeTraverser.findFocusableNodeFor(element, this); - } } suite('FocusableTreeTraverser', function () {