From 0772a298245d76e4291977d352c7ef00745ef725 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Mon, 21 Apr 2025 20:37:26 +0000 Subject: [PATCH 1/7] feat!: Introduce new focus tree/node functions. This introduces new callback methods for IFocusableTree and IFocusableNode for providing a basis of synchronizing domain state with focus changes. It also introduces support for implementations of IFocusableTree to better manage initial state cases, especially when a tree is focused using tab navigation. FocusManager has also been updated to ensure functional parity between tab-navigating to a tree and using focusTree() on that tree. This means that tab navigating to a tree will actually restore focus back to that tree's previous focused node rather than the root (unless the root is navigated to from within the tree itself). This is meant to provide better consistency between tab and non-tab keyboard navigation. Note that these changes originally came from #8875 and are required for later PRs that will introduce IFocusableNode and IFocusableTree implementations. --- core/focus_manager.ts | 99 +++++++++++++++++--- core/interfaces/i_focusable_node.ts | 44 ++++++++- core/interfaces/i_focusable_tree.ts | 79 ++++++++++++++++ tests/mocha/focus_manager_test.js | 42 ++++++--- tests/mocha/focusable_tree_traverser_test.js | 12 +++ 5 files changed, 247 insertions(+), 29 deletions(-) diff --git a/core/focus_manager.ts b/core/focus_manager.ts index c1fc295b991..26cc1a0c511 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -60,6 +60,7 @@ export class FocusManager { registeredTrees: Array = []; private currentlyHoldsEphemeralFocus: boolean = false; + private lockFocusStateChanges: boolean = false; constructor( addGlobalEventListener: (type: string, listener: EventListener) => void, @@ -89,7 +90,16 @@ export class FocusManager { } if (newNode) { - this.focusNode(newNode); + const newTree = newNode.getFocusableTree(); + const oldTree = this.focusedNode?.getFocusableTree(); + if (newNode === newTree.getRootFocusableNode() && newTree !== oldTree) { + // If the root of the tree is the one taking focus (such as due to + // being tabbed), try to focus the whole tree explicitly to ensure the + // correct node re-receives focus. + this.focusTree(newTree); + } else { + this.focusNode(newNode); + } } else { this.defocusCurrentFocusedNode(); } @@ -108,6 +118,7 @@ export class FocusManager { * certain whether the tree has been registered. */ registerTree(tree: IFocusableTree): void { + this.ensureManagerIsUnlocked(); if (this.isRegistered(tree)) { throw Error(`Attempted to re-register already registered tree: ${tree}.`); } @@ -133,6 +144,7 @@ export class FocusManager { * this manager. */ unregisterTree(tree: IFocusableTree): void { + this.ensureManagerIsUnlocked(); if (!this.isRegistered(tree)) { throw Error(`Attempted to unregister not registered tree: ${tree}.`); } @@ -192,11 +204,14 @@ export class FocusManager { * focus. */ focusTree(focusableTree: IFocusableTree): void { + this.ensureManagerIsUnlocked(); if (!this.isRegistered(focusableTree)) { throw Error(`Attempted to focus unregistered tree: ${focusableTree}.`); } const currNode = FocusableTreeTraverser.findFocusedNode(focusableTree); - this.focusNode(currNode ?? focusableTree.getRootFocusableNode()); + const nodeToRestore = focusableTree.getRestoredFocusableNode(currNode); + const rootFallback = focusableTree.getRootFocusableNode(); + this.focusNode(nodeToRestore ?? currNode ?? rootFallback); } /** @@ -205,18 +220,37 @@ export class FocusManager { * Any previously focused node will be updated to be passively highlighted (if * it's in a different focusable tree) or blurred (if it's in the same one). * - * @param focusableNode The node that should receive active - * focus. + * @param focusableNode The node that should receive active focus. */ focusNode(focusableNode: IFocusableNode): void { + this.ensureManagerIsUnlocked(); + if (this.focusedNode == focusableNode) return; // State is unchanged. + const nextTree = focusableNode.getFocusableTree(); if (!this.isRegistered(nextTree)) { throw Error(`Attempted to focus unregistered node: ${focusableNode}.`); } + + // Safety check for ensuring focusNode() doesn't get called for a node that + // isn't actually hooked up to its parent tree correctly (since this can + // cause weird inconsistencies). + const matchedNode = FocusableTreeTraverser.findFocusableNodeFor( + focusableNode.getFocusableElement(), + nextTree, + ); + if (matchedNode !== focusableNode) { + throw Error( + `Attempting to focus node which isn't recognized by its parent tree: ` + + `${focusableNode}.`, + ); + } + const prevNode = this.focusedNode; - if (prevNode && prevNode.getFocusableTree() !== nextTree) { - this.setNodeToPassive(prevNode); + const prevTree = prevNode?.getFocusableTree(); + if (prevNode && prevTree !== nextTree) { + this.passivelyFocusNode(prevNode, nextTree); } + // If there's a focused node in the new node's tree, ensure it's reset. const prevNodeNextTree = FocusableTreeTraverser.findFocusedNode(nextTree); const nextTreeRoot = nextTree.getRootFocusableNode(); @@ -229,9 +263,10 @@ export class FocusManager { if (nextTreeRoot !== focusableNode) { this.removeHighlight(nextTreeRoot); } + if (!this.currentlyHoldsEphemeralFocus) { // Only change the actively focused node if ephemeral state isn't held. - this.setNodeToActive(focusableNode); + this.activelyFocusNode(focusableNode, prevTree ?? null); } this.focusedNode = focusableNode; } @@ -257,6 +292,7 @@ export class FocusManager { takeEphemeralFocus( focusableElement: HTMLElement | SVGElement, ): ReturnEphemeralFocus { + this.ensureManagerIsUnlocked(); if (this.currentlyHoldsEphemeralFocus) { throw Error( `Attempted to take ephemeral focus when it's already held, ` + @@ -266,7 +302,7 @@ export class FocusManager { this.currentlyHoldsEphemeralFocus = true; if (this.focusedNode) { - this.setNodeToPassive(this.focusedNode); + this.passivelyFocusNode(this.focusedNode, null); } focusableElement.focus(); @@ -282,29 +318,66 @@ export class FocusManager { this.currentlyHoldsEphemeralFocus = false; if (this.focusedNode) { - this.setNodeToActive(this.focusedNode); + this.activelyFocusNode(this.focusedNode, null); } }; } + private ensureManagerIsUnlocked(): void { + if (this.lockFocusStateChanges) { + throw Error( + 'FocusManager state changes cannot happen in a tree/node focus/blur ' + + 'callback.', + ); + } + } + private defocusCurrentFocusedNode(): void { // The current node will likely be defocused while ephemeral focus is held, // but internal manager state shouldn't change since the node should be // restored upon exiting ephemeral focus mode. if (this.focusedNode && !this.currentlyHoldsEphemeralFocus) { - this.setNodeToPassive(this.focusedNode); + this.passivelyFocusNode(this.focusedNode, null); this.focusedNode = null; } } - private setNodeToActive(node: IFocusableNode): void { + private activelyFocusNode( + node: IFocusableNode, + prevTree: IFocusableTree | null, + ): void { + // Note that order matters here. Focus callbacks are allowed to change + // element visibility which can influence focusability, including for a + // node's focusable element (which *is* allowed to be invisible until the + // node needs to be focused). + this.lockFocusStateChanges = true; + node.getFocusableTree().onTreeFocus(node, prevTree); + node.onNodeFocus(); + this.lockFocusStateChanges = false; + + this.setNodeToVisualActiveFocus(node); + node.getFocusableElement().focus(); + } + + private passivelyFocusNode( + node: IFocusableNode, + nextTree: IFocusableTree | null, + ): void { + this.lockFocusStateChanges = true; + node.getFocusableTree().onTreeBlur(nextTree); + node.onNodeBlur(); + this.lockFocusStateChanges = false; + + this.setNodeToVisualPassiveFocus(node); + } + + private setNodeToVisualActiveFocus(node: IFocusableNode): void { const element = node.getFocusableElement(); dom.addClass(element, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME); dom.removeClass(element, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME); - element.focus(); } - private setNodeToPassive(node: IFocusableNode): void { + private setNodeToVisualPassiveFocus(node: IFocusableNode): void { const element = node.getFocusableElement(); dom.removeClass(element, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME); dom.addClass(element, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME); diff --git a/core/interfaces/i_focusable_node.ts b/core/interfaces/i_focusable_node.ts index 14100d44c7f..44bdf8be0db 100644 --- a/core/interfaces/i_focusable_node.ts +++ b/core/interfaces/i_focusable_node.ts @@ -25,8 +25,14 @@ export interface IFocusableNode { * and a tab index must be present in order for the element to be focusable in * the DOM). * - * It's expected the return element will not change for the lifetime of the - * node. + * The returned element must be visible if the node is ever focused via + * FocusManager.focusNode() or FocusManager.focusTree(). It's allowed for an + * element to be hidden until onNodeFocus() is called, or become hidden with a + * call to onNodeBlur(). + * + * It's expected the actual returned element will not change for the lifetime + * of the node (that is, its properties can change but a new element should + * never be returned.) */ getFocusableElement(): HTMLElement | SVGElement; @@ -36,4 +42,38 @@ export interface IFocusableNode { * belongs. */ getFocusableTree(): IFocusableTree; + + /** + * Called when this node receives active focus. + * + * Note that it's fine for implementations to change visibility modifiers, but + * they should avoid the following: + * - Creating or removing DOM elements (including via the renderer or drawer). + * - Affecting focus via DOM focus() calls or the FocusManager. + */ + onNodeFocus(): void; + + /** + * Called when this node loses active focus. It may still have passive focus. + * + * This has the same implementation restrictions as onNodeFocus(). + */ + onNodeBlur(): void; +} + +/** + * Determines whether the provided object fulfills the contract of + * IFocusableNode. + * + * @param object The object to test. + * @returns Whether the provided object can be used as an IFocusableNode. + */ +export function isFocusableNode(object: any | null): object is IFocusableNode { + return ( + object && + 'getFocusableElement' in object && + 'getFocusableTree' in object && + 'onNodeFocus' in object && + 'onNodeBlur' in object + ); } diff --git a/core/interfaces/i_focusable_tree.ts b/core/interfaces/i_focusable_tree.ts index bc0c38849c8..364f4cad630 100644 --- a/core/interfaces/i_focusable_tree.ts +++ b/core/interfaces/i_focusable_tree.ts @@ -37,6 +37,34 @@ export interface IFocusableTree { */ getRootFocusableNode(): IFocusableNode; + /** + * Returns the IFocusableNode of this tree that should receive active focus + * when the tree itself has focused returned to it. + * + * There are some very important notes to consider about a tree's focus + * lifecycle when implementing a version of this method that doesn't return + * null: + * 1. A null previousNode does not guarantee first-time focus state as nodes + * can be deleted. + * 2. This method is only used when the tree itself is focused, either through + * tab navigation or via FocusManager.focusTree(). In many cases, the + * previously focused node will be directly focused instead which will + * bypass this method. + * 3. The default behavior (i.e. returning null here) involves either + * restoring the previous node (previousNode) or focusing the tree's root. + * + * This method is largely intended to provide tree implementations with the + * means of specifying a better default node than their root. + * + * @param previousNode The node that previously held passive focus for this + * tree, or null if the tree hasn't yet been focused. + * @returns The IFocusableNode that should now receive focus, or null if + * default behavior should be used, instead. + */ + getRestoredFocusableNode( + previousNode: IFocusableNode | null, + ): IFocusableNode | null; + /** * Returns all directly nested trees under this tree. * @@ -58,4 +86,55 @@ export interface IFocusableTree { * @param id The ID of the node's focusable HTMLElement or SVGElement. */ lookUpFocusableNode(id: string): IFocusableNode | null; + + /** + * Called when a node of this tree has received active focus. + * + * Note that a null previousTree does not necessarily indicate that this is + * the first time Blockly is receiving focus. In fact, few assumptions can be + * made about previous focus state as a previous null tree simply indicates + * that Blockly did not hold active focus prior to this tree becoming focused + * (which can happen due to focus exiting the Blockly injection div, or for + * other cases like ephemeral focus). + * + * See IFocusableNode.onNodeFocus() as implementations have the same + * restrictions as with that method. + * + * @param node The node receiving active focus. + * @param previousTree The previous tree that held active focus, or null if + * none. + */ + onTreeFocus(node: IFocusableNode, previousTree: IFocusableTree | null): void; + + /** + * Called when the previously actively focused node of this tree is now + * passively focused and there is no other active node of this tree taking its + * place. + * + * This has the same implementation restrictions and considerations as + * onTreeFocus(). + * + * @param nextTree The next tree receiving active focus, or null if none (such + * as in the case that Blockly is entirely losing DOM focus). + */ + onTreeBlur(nextTree: IFocusableTree | null): void; +} + +/** + * Determines whether the provided object fulfills the contract of + * IFocusableTree. + * + * @param object The object to test. + * @returns Whether the provided object can be used as an IFocusableTree. + */ +export function isFocusableTree(object: any | null): object is IFocusableTree { + return ( + object && + 'getRootFocusableNode' in object && + 'getRestoredFocusableNode' in object && + 'getNestedTrees' in object && + 'lookUpFocusableNode' in object && + 'onTreeFocus' in object && + 'onTreeBlur' in object + ); } diff --git a/tests/mocha/focus_manager_test.js b/tests/mocha/focus_manager_test.js index 4a3f6b3ad1f..70ef210c587 100644 --- a/tests/mocha/focus_manager_test.js +++ b/tests/mocha/focus_manager_test.js @@ -27,6 +27,10 @@ class FocusableNodeImpl { getFocusableTree() { return this.tree; } + + onNodeFocus() {} + + onNodeBlur() {} } class FocusableTreeImpl { @@ -46,6 +50,10 @@ class FocusableTreeImpl { return this.rootNode; } + getRestoredFocusableNode() { + return null; + } + getNestedTrees() { return this.nestedTrees; } @@ -53,6 +61,10 @@ class FocusableTreeImpl { lookUpFocusableNode(id) { return this.idToNodeMap[id]; } + + onTreeFocus() {} + + onTreeBlur() {} } suite('FocusManager', function () { @@ -2067,7 +2079,7 @@ suite('FocusManager', function () { ); }); - test('registered tree focus()ed other tree node passively focused tree root now has active property', function () { + test('registered tree focus()ed other tree node passively focused tree node now has active property', function () { this.focusManager.registerTree(this.testFocusableTree1); this.focusManager.registerTree(this.testFocusableTree2); document.getElementById('testFocusableTree1.node1').focus(); @@ -2075,26 +2087,27 @@ suite('FocusManager', function () { document.getElementById('testFocusableTree1').focus(); - // This differs from the behavior of focusTree() since directly focusing a tree's root will - // coerce it to now have focus. + // Directly refocusing a tree's root should have functional parity with focusTree(). That + // means the tree's previous node should now have active focus again and its root should + // have no focus indication. const rootElem = this.testFocusableTree1 .getRootFocusableNode() .getFocusableElement(); const nodeElem = this.testFocusableTree1Node1.getFocusableElement(); assert.includesClass( - rootElem.classList, + nodeElem.classList, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME, ); assert.notIncludesClass( - rootElem.classList, + nodeElem.classList, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME, ); assert.notIncludesClass( - nodeElem.classList, + rootElem.classList, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME, ); assert.notIncludesClass( - nodeElem.classList, + rootElem.classList, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME, ); }); @@ -3879,7 +3892,7 @@ suite('FocusManager', function () { ); }); - test('registered tree focus()ed other tree node passively focused tree root now has active property', function () { + test('registered tree focus()ed other tree node passively focused tree node now has active property', function () { this.focusManager.registerTree(this.testFocusableGroup1); this.focusManager.registerTree(this.testFocusableGroup2); document.getElementById('testFocusableGroup1.node1').focus(); @@ -3887,26 +3900,27 @@ suite('FocusManager', function () { document.getElementById('testFocusableGroup1').focus(); - // This differs from the behavior of focusTree() since directly focusing a tree's root will - // coerce it to now have focus. + // Directly refocusing a tree's root should have functional parity with focusTree(). That + // means the tree's previous node should now have active focus again and its root should + // have no focus indication. const rootElem = this.testFocusableGroup1 .getRootFocusableNode() .getFocusableElement(); const nodeElem = this.testFocusableGroup1Node1.getFocusableElement(); assert.includesClass( - rootElem.classList, + nodeElem.classList, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME, ); assert.notIncludesClass( - rootElem.classList, + nodeElem.classList, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME, ); assert.notIncludesClass( - nodeElem.classList, + rootElem.classList, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME, ); assert.notIncludesClass( - nodeElem.classList, + rootElem.classList, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME, ); }); diff --git a/tests/mocha/focusable_tree_traverser_test.js b/tests/mocha/focusable_tree_traverser_test.js index b6674573ecd..d2467b6e95c 100644 --- a/tests/mocha/focusable_tree_traverser_test.js +++ b/tests/mocha/focusable_tree_traverser_test.js @@ -25,6 +25,10 @@ class FocusableNodeImpl { getFocusableTree() { return this.tree; } + + onNodeFocus() {} + + onNodeBlur() {} } class FocusableTreeImpl { @@ -44,6 +48,10 @@ class FocusableTreeImpl { return this.rootNode; } + getRestoredFocusableNode() { + return null; + } + getNestedTrees() { return this.nestedTrees; } @@ -51,6 +59,10 @@ class FocusableTreeImpl { lookUpFocusableNode(id) { return this.idToNodeMap[id]; } + + onTreeFocus() {} + + onTreeBlur() {} } suite('FocusableTreeTraverser', function () { From 404c20eeaf5b0927ae65112256f5a040054bbcc6 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Mon, 21 Apr 2025 20:55:02 +0000 Subject: [PATCH 2/7] chore: Remove unused isFocusable*() functions. These were needed in previous versions of plugin changes, but aren't anymore. --- core/interfaces/i_focusable_node.ts | 17 ----------------- core/interfaces/i_focusable_tree.ts | 19 ------------------- 2 files changed, 36 deletions(-) diff --git a/core/interfaces/i_focusable_node.ts b/core/interfaces/i_focusable_node.ts index 44bdf8be0db..0e81cd8dcb9 100644 --- a/core/interfaces/i_focusable_node.ts +++ b/core/interfaces/i_focusable_node.ts @@ -60,20 +60,3 @@ export interface IFocusableNode { */ onNodeBlur(): void; } - -/** - * Determines whether the provided object fulfills the contract of - * IFocusableNode. - * - * @param object The object to test. - * @returns Whether the provided object can be used as an IFocusableNode. - */ -export function isFocusableNode(object: any | null): object is IFocusableNode { - return ( - object && - 'getFocusableElement' in object && - 'getFocusableTree' in object && - 'onNodeFocus' in object && - 'onNodeBlur' in object - ); -} diff --git a/core/interfaces/i_focusable_tree.ts b/core/interfaces/i_focusable_tree.ts index 364f4cad630..9d1e68559c7 100644 --- a/core/interfaces/i_focusable_tree.ts +++ b/core/interfaces/i_focusable_tree.ts @@ -119,22 +119,3 @@ export interface IFocusableTree { */ onTreeBlur(nextTree: IFocusableTree | null): void; } - -/** - * Determines whether the provided object fulfills the contract of - * IFocusableTree. - * - * @param object The object to test. - * @returns Whether the provided object can be used as an IFocusableTree. - */ -export function isFocusableTree(object: any | null): object is IFocusableTree { - return ( - object && - 'getRootFocusableNode' in object && - 'getRestoredFocusableNode' in object && - 'getNestedTrees' in object && - 'lookUpFocusableNode' in object && - 'onTreeFocus' in object && - 'onTreeBlur' in object - ); -} From c91fed3fdb775250b5c30d07c934be65e5db5fae Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Mon, 21 Apr 2025 21:00:27 +0000 Subject: [PATCH 3/7] chore: equality + doc cleanups --- core/focus_manager.ts | 2 +- core/interfaces/i_focusable_node.ts | 2 +- core/interfaces/i_focusable_tree.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/focus_manager.ts b/core/focus_manager.ts index 26cc1a0c511..230bdf030e1 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -224,7 +224,7 @@ export class FocusManager { */ focusNode(focusableNode: IFocusableNode): void { this.ensureManagerIsUnlocked(); - if (this.focusedNode == focusableNode) return; // State is unchanged. + if (this.focusedNode === focusableNode) return; // State is unchanged. const nextTree = focusableNode.getFocusableTree(); if (!this.isRegistered(nextTree)) { diff --git a/core/interfaces/i_focusable_node.ts b/core/interfaces/i_focusable_node.ts index 0e81cd8dcb9..06d43acea1f 100644 --- a/core/interfaces/i_focusable_node.ts +++ b/core/interfaces/i_focusable_node.ts @@ -32,7 +32,7 @@ export interface IFocusableNode { * * It's expected the actual returned element will not change for the lifetime * of the node (that is, its properties can change but a new element should - * never be returned.) + * never be returned). */ getFocusableElement(): HTMLElement | SVGElement; diff --git a/core/interfaces/i_focusable_tree.ts b/core/interfaces/i_focusable_tree.ts index 9d1e68559c7..699328ef8d6 100644 --- a/core/interfaces/i_focusable_tree.ts +++ b/core/interfaces/i_focusable_tree.ts @@ -39,7 +39,7 @@ export interface IFocusableTree { /** * Returns the IFocusableNode of this tree that should receive active focus - * when the tree itself has focused returned to it. + * when the tree itself has focus returned to it. * * There are some very important notes to consider about a tree's focus * lifecycle when implementing a version of this method that doesn't return From 4e8bb9850f35c0fbdceddd1ac3526487156534e3 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Mon, 21 Apr 2025 21:09:26 +0000 Subject: [PATCH 4/7] Revert "chore: Remove unused isFocusable*() functions." This reverts commit 404c20eeaf5b0927ae65112256f5a040054bbcc6. --- core/interfaces/i_focusable_node.ts | 17 +++++++++++++++++ core/interfaces/i_focusable_tree.ts | 19 +++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/core/interfaces/i_focusable_node.ts b/core/interfaces/i_focusable_node.ts index 06d43acea1f..53a432d30f4 100644 --- a/core/interfaces/i_focusable_node.ts +++ b/core/interfaces/i_focusable_node.ts @@ -60,3 +60,20 @@ export interface IFocusableNode { */ onNodeBlur(): void; } + +/** + * Determines whether the provided object fulfills the contract of + * IFocusableNode. + * + * @param object The object to test. + * @returns Whether the provided object can be used as an IFocusableNode. + */ +export function isFocusableNode(object: any | null): object is IFocusableNode { + return ( + object && + 'getFocusableElement' in object && + 'getFocusableTree' in object && + 'onNodeFocus' in object && + 'onNodeBlur' in object + ); +} diff --git a/core/interfaces/i_focusable_tree.ts b/core/interfaces/i_focusable_tree.ts index 699328ef8d6..69afa24ffdf 100644 --- a/core/interfaces/i_focusable_tree.ts +++ b/core/interfaces/i_focusable_tree.ts @@ -119,3 +119,22 @@ export interface IFocusableTree { */ onTreeBlur(nextTree: IFocusableTree | null): void; } + +/** + * Determines whether the provided object fulfills the contract of + * IFocusableTree. + * + * @param object The object to test. + * @returns Whether the provided object can be used as an IFocusableTree. + */ +export function isFocusableTree(object: any | null): object is IFocusableTree { + return ( + object && + 'getRootFocusableNode' in object && + 'getRestoredFocusableNode' in object && + 'getNestedTrees' in object && + 'lookUpFocusableNode' in object && + 'onTreeFocus' in object && + 'onTreeBlur' in object + ); +} From 7c0c8536e6b51af9088bc928ac26e9ae73209c29 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 22 Apr 2025 00:49:52 +0000 Subject: [PATCH 5/7] fix: Fix broken FocusManager tree unregistration. --- core/focus_manager.ts | 2 +- tests/mocha/focus_manager_test.js | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/core/focus_manager.ts b/core/focus_manager.ts index 230bdf030e1..c6d40fa0d0f 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -148,7 +148,7 @@ export class FocusManager { if (!this.isRegistered(tree)) { throw Error(`Attempted to unregister not registered tree: ${tree}.`); } - const treeIndex = this.registeredTrees.findIndex((tree) => tree === tree); + const treeIndex = this.registeredTrees.findIndex((reg) => reg === tree); this.registeredTrees.splice(treeIndex, 1); const focusedNode = FocusableTreeTraverser.findFocusedNode(tree); diff --git a/tests/mocha/focus_manager_test.js b/tests/mocha/focus_manager_test.js index 70ef210c587..69ecfe722a5 100644 --- a/tests/mocha/focus_manager_test.js +++ b/tests/mocha/focus_manager_test.js @@ -305,6 +305,18 @@ suite('FocusManager', function () { assert.isTrue(isRegistered); }); + + test('for unregistered tree with other registered tree returns false', function () { + this.focusManager.registerTree(this.testFocusableTree2); + this.focusManager.registerTree(this.testFocusableTree1); + this.focusManager.unregisterTree(this.testFocusableTree1); + + const isRegistered = this.focusManager.isRegistered( + this.testFocusableTree1, + ); + + assert.isFalse(isRegistered); + }); }); suite('getFocusedTree()', function () { From 2564239d23437a133b33209c03bcad7e57848365 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 23 Apr 2025 21:22:37 +0000 Subject: [PATCH 6/7] chore: Add private method documentation. Addresses a review comment. --- core/focus_manager.ts | 53 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/core/focus_manager.ts b/core/focus_manager.ts index c6d40fa0d0f..83755067c4c 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -323,6 +323,13 @@ export class FocusManager { }; } + /** + * Ensures that the manager is currently allowing operations that change its + * internal focus state (such as via focusNode()). + * + * If the manager is currently not allowing state changes, an exception is + * thrown. + */ private ensureManagerIsUnlocked(): void { if (this.lockFocusStateChanges) { throw Error( @@ -332,6 +339,10 @@ export class FocusManager { } } + /** + * Defocuses the current actively focused node tracked by the manager, if + * there is one iff the manager isn't in an ephemeral focus state. + */ private defocusCurrentFocusedNode(): void { // The current node will likely be defocused while ephemeral focus is held, // but internal manager state shouldn't change since the node should be @@ -342,6 +353,18 @@ export class FocusManager { } } + /** + * Marks the specified node as actively focused, also calling related lifecycle + * callback methods for both the node and its parent tree. This ensures that + * the node is properly styled to indicate its active focus. + * + * This does not change the manager's currently tracked node, nor does it + * change any other nodes. + * + * @param node The node to be actively focused. + * @param prevTree The tree of the previously actively focused node, or null + * if there wasn't a previously actively focused node. + */ private activelyFocusNode( node: IFocusableNode, prevTree: IFocusableTree | null, @@ -359,6 +382,18 @@ export class FocusManager { node.getFocusableElement().focus(); } + /** + * Marks the specified node as passively focused, also calling related + * lifecycle callback methods for both the node and its parent tree. This + * ensures that the node is properly styled to indicate its passive focus. + * + * This does not change the manager's currently tracked node, nor does it + * change any other nodes. + * + * @param node The node to be passively focused. + * @param nextTree The tree of the node receiving active focus, or null if no + * node will be actively focused. + */ private passivelyFocusNode( node: IFocusableNode, nextTree: IFocusableTree | null, @@ -371,18 +406,36 @@ export class FocusManager { this.setNodeToVisualPassiveFocus(node); } + /** + * Updates the node's styling to indicate that it should have an active focus + * indicator. + * + * @param node The node to be styled for active focus. + */ private setNodeToVisualActiveFocus(node: IFocusableNode): void { const element = node.getFocusableElement(); dom.addClass(element, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME); dom.removeClass(element, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME); } + /** + * Updates the node's styling to indicate that it should have a passive focus + * indicator. + * + * @param node The node to be styled for passive focus. + */ private setNodeToVisualPassiveFocus(node: IFocusableNode): void { const element = node.getFocusableElement(); dom.removeClass(element, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME); dom.addClass(element, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME); } + /** + * Removes any active/passive indicators for the specified node. + * + * @param node The node which should have neither passive nor active focus + * indication. + */ private removeHighlight(node: IFocusableNode): void { const element = node.getFocusableElement(); dom.removeClass(element, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME); From 096e7711cb85a719dcdf310dbe06fb3aa8265854 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 23 Apr 2025 21:24:05 +0000 Subject: [PATCH 7/7] chore: clean-up documentation comment. --- core/focus_manager.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/focus_manager.ts b/core/focus_manager.ts index 83755067c4c..88eef46b530 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -340,8 +340,8 @@ export class FocusManager { } /** - * Defocuses the current actively focused node tracked by the manager, if - * there is one iff the manager isn't in an ephemeral focus state. + * Defocuses the current actively focused node tracked by the manager, iff + * there's a node being tracked and the manager doesn't have ephemeral focus. */ private defocusCurrentFocusedNode(): void { // The current node will likely be defocused while ephemeral focus is held,