Skip to content

Commit a3b3ea7

Browse files
authored
fix: Improve missing node resiliency (#8997)
## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #8994 ### Proposed Changes This removes an error that was previously thrown by `FocusManager` when attempting to focus an invalid node (such as one that's been removed from its parent). ### Reason for Changes #8994 (comment) goes into more detail. While this error did cover legitimately wrong cases to try and focus things (and helped to catch some real problems), fixing this 'properly' may become a leaky boat problem where we have to track down every possible asynchronous scenario that could produce such a case. One class of this is ephemeral focus which had robustness improvements itself in #8981 that, by effect, caused this issue in the first place. Holistically fixing this with enforced API contracts alone isn't simple due to the nature of how these components interact. This change ensures that there's a sane default to fall back on if an invalid node is passed in. Note that `FocusManager` was designed specifically to disallow defocusing a node (since fallbacks can get messy and introduce unpredictable user experiences), and this sort of allows that now. However, this seems like a reasonable approach as it defaults to the behavior when focusing a tree explicitly which allows the tree to fallback to a more suitable default (such as the first item to select in the toolbox for that particular tree). In many cases this will default back to the tree's root node (such as the workspace root group) since sometimes the removed node is still the "last focused node" of the tree (and is considered valid for the purpose of determining a fallback; tree implementations could further specialize by checking whether that node is still valid). ### Test Coverage Some new tests were added to cover this case, but more may be useful to add as part of #8910. ### Documentation No documentation needs to be added or updated as part of this (beyond code documentation changes). ### Additional Information This original issue was found by @RoboErikG when testing #8995. I also verified this against the keyboard navigation plugin repository.
1 parent 86c831a commit a3b3ea7

File tree

3 files changed

+49
-13
lines changed

3 files changed

+49
-13
lines changed

core/focus_manager.ts

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ export class FocusManager {
227227
}
228228

229229
/**
230-
* Focuses DOM input on the selected node, and marks it as actively focused.
230+
* Focuses DOM input on the specified node, and marks it as actively focused.
231231
*
232232
* Any previously focused node will be updated to be passively highlighted (if
233233
* it's in a different focusable tree) or blurred (if it's in the same one).
@@ -244,17 +244,20 @@ export class FocusManager {
244244
}
245245

246246
// Safety check for ensuring focusNode() doesn't get called for a node that
247-
// isn't actually hooked up to its parent tree correctly (since this can
248-
// cause weird inconsistencies).
247+
// isn't actually hooked up to its parent tree correctly. This usually
248+
// happens when calls to focusNode() interleave with asynchronous clean-up
249+
// operations (which can happen due to ephemeral focus and in other cases).
250+
// Fall back to a reasonable default since there's no valid node to focus.
249251
const matchedNode = FocusableTreeTraverser.findFocusableNodeFor(
250252
focusableNode.getFocusableElement(),
251253
nextTree,
252254
);
255+
const prevNodeNextTree = FocusableTreeTraverser.findFocusedNode(nextTree);
256+
let nodeToFocus = focusableNode;
253257
if (matchedNode !== focusableNode) {
254-
throw Error(
255-
`Attempting to focus node which isn't recognized by its parent tree: ` +
256-
`${focusableNode}.`,
257-
);
258+
const nodeToRestore = nextTree.getRestoredFocusableNode(prevNodeNextTree);
259+
const rootFallback = nextTree.getRootFocusableNode();
260+
nodeToFocus = nodeToRestore ?? prevNodeNextTree ?? rootFallback;
258261
}
259262

260263
const prevNode = this.focusedNode;
@@ -264,27 +267,26 @@ export class FocusManager {
264267
}
265268

266269
// If there's a focused node in the new node's tree, ensure it's reset.
267-
const prevNodeNextTree = FocusableTreeTraverser.findFocusedNode(nextTree);
268270
const nextTreeRoot = nextTree.getRootFocusableNode();
269271
if (prevNodeNextTree) {
270272
this.removeHighlight(prevNodeNextTree);
271273
}
272274
// For caution, ensure that the root is always reset since getFocusedNode()
273275
// is expected to return null if the root was highlighted, if the root is
274276
// not the node now being set to active.
275-
if (nextTreeRoot !== focusableNode) {
277+
if (nextTreeRoot !== nodeToFocus) {
276278
this.removeHighlight(nextTreeRoot);
277279
}
278280

279281
if (!this.currentlyHoldsEphemeralFocus) {
280282
// Only change the actively focused node if ephemeral state isn't held.
281-
this.activelyFocusNode(focusableNode, prevTree ?? null);
283+
this.activelyFocusNode(nodeToFocus, prevTree ?? null);
282284
}
283-
this.updateFocusedNode(focusableNode);
285+
this.updateFocusedNode(nodeToFocus);
284286
}
285287

286288
/**
287-
* Ephemerally captures focus for a selected element until the returned lambda
289+
* Ephemerally captures focus for a specific element until the returned lambda
288290
* is called. This is expected to be especially useful for ephemeral UI flows
289291
* like dialogs.
290292
*

core/interfaces/i_focusable_tree.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,10 @@ export interface IFocusableTree {
5252
* bypass this method.
5353
* 3. The default behavior (i.e. returning null here) involves either
5454
* restoring the previous node (previousNode) or focusing the tree's root.
55+
* 4. The provided node may sometimes no longer be valid, such as in the case
56+
* an attempt is made to focus a node that has been recently removed from
57+
* its parent tree. Implementations can check for the validity of the node
58+
* in order to specialize the node to which focus should fall back.
5559
*
5660
* This method is largely intended to provide tree implementations with the
5761
* means of specifying a better default node than their root.

tests/mocha/focus_manager_test.js

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ class FocusableTreeImpl {
3838
this.nestedTrees = nestedTrees;
3939
this.idToNodeMap = {};
4040
this.rootNode = this.addNode(rootElement);
41+
this.fallbackNode = null;
4142
}
4243

4344
addNode(element) {
@@ -46,12 +47,16 @@ class FocusableTreeImpl {
4647
return node;
4748
}
4849

50+
removeNode(node) {
51+
delete this.idToNodeMap[node.getFocusableElement().id];
52+
}
53+
4954
getRootFocusableNode() {
5055
return this.rootNode;
5156
}
5257

5358
getRestoredFocusableNode() {
54-
return null;
59+
return this.fallbackNode;
5560
}
5661

5762
getNestedTrees() {
@@ -385,6 +390,31 @@ suite('FocusManager', function () {
385390
// There should be exactly 1 focus event fired from focusNode().
386391
assert.strictEqual(focusCount, 1);
387392
});
393+
394+
test('for orphaned node returns tree root by default', function () {
395+
this.focusManager.registerTree(this.testFocusableTree1);
396+
this.testFocusableTree1.removeNode(this.testFocusableTree1Node1);
397+
398+
this.focusManager.focusNode(this.testFocusableTree1Node1);
399+
400+
// Focusing an invalid node should fall back to the tree root when it has no restoration
401+
// fallback node.
402+
const currentNode = this.focusManager.getFocusedNode();
403+
const treeRoot = this.testFocusableTree1.getRootFocusableNode();
404+
assert.strictEqual(currentNode, treeRoot);
405+
});
406+
407+
test('for orphaned node returns specified fallback node', function () {
408+
this.focusManager.registerTree(this.testFocusableTree1);
409+
this.testFocusableTree1.fallbackNode = this.testFocusableTree1Node2;
410+
this.testFocusableTree1.removeNode(this.testFocusableTree1Node1);
411+
412+
this.focusManager.focusNode(this.testFocusableTree1Node1);
413+
414+
// Focusing an invalid node should fall back to the restored fallback.
415+
const currentNode = this.focusManager.getFocusedNode();
416+
assert.strictEqual(currentNode, this.testFocusableTree1Node2);
417+
});
388418
});
389419

390420
suite('getFocusManager()', function () {

0 commit comments

Comments
 (0)