feat: Update line cursor to use focus manager (#8941)

## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

Fixes #8940
Fixes #8954
Fixes #8955

### Proposed Changes

This updates `LineCursor` to use `FocusManager` rather than selection (principally) as the source of truth.

### Reason for Changes

Ensuring that keyboard navigation works correctly with eventual screen reader support requires ensuring that ever navigated component is focused, and this is primarily what `FocusManager` has been designed to do. Since these nodes are already focused, `FocusManager` can be used as the primary source of truth for determining where the user currently has navigated, and where to go next.

Previously, `LineCursor` relied on selection for this purpose, but selection is now automatically updated (for blocks) using focus-controlled `focus` and `blur` callbacks. Note that the cursor will still fall back to synchronizing with selection state, though this will be removed once the remaining work to eliminate `MarkerSvg` has concluded (which requires further consideration on the keyboard navigation side viz-a-viz styling and CSS decisions) and once mouse clicks are synchronized with focus management.

Note that the changes in this PR are closely tied to RaspberryPiFoundation/blockly-keyboard-experimentation#482 as both are necessary in order for the keyboard navigation plugin to correctly work with `FocusManager`.

Some other noteworthy changes:
- Some special handling exists for flyouts to handle navigating across stacks (per the current cursor design).
- `FocusableTreeTraverser` is needed by the keyboard navigation plugin (in RaspberryPiFoundation/blockly-keyboard-experimentation#482) so it's now being exported.
- `FocusManager` had one bug that's now patched and tested in this PR: it didn't handle the case of the browser completely forcing focus loss. It would continue to maintain active focus even though no tracked elements now hold focus. One such case is the element being deleted, but there are other cases where this can happen (such as with dialog prompts).
- `FocusManager` had some issues from #8909 wherein it would overeagerly call tree focus callbacks and slightly mismanage the passive node. Since tests haven't yet been added for these lifecycle callbacks, these cases weren't originally caught (per #8910).
- `FocusManager` was updated to move the tracked manager into a static function so that it can be replaced in tests. This was done to facilitate changes to setup_teardown.js to ensure that a unique `FocusManager` exists _per-test_. It's possible for DOM focus state to still bleed across tests, but `FocusManager` largely guarantees eventual consistency. This change prevents a class of focus errors from being possible when running tests.
- A number of cursor tests needed to be updated to ensure that a connections are properly rendered (as this is a requirement for focusable nodes, and cursor is now focusing nodes). One test for output connections was changed to use an input connection, instead, since output connections can no longer be navigated to (and aren't rendered, thus are not focusable). It's possible this will need to be changed in the future if we decide to reintroduce support for output connections in cursor, but it seems like a reasonable stopgap. Huge thanks to @rachel-fenichel for helping investigate and providing an alternative for the output connection test.

**Current gaps** to be fixed after this PR is merged:
- The flyout automatically closes when creating a variable with with keyboard or mouse (I think this is only for the keyboard navigation plugin). I believe this is a regression from previous behavior due to how the navigation plugin is managing state. It would know the flyout should be open and thus ensure it stays open even when things like dialog prompts try to close it with a blur event. However, the new implementation in RaspberryPiFoundation/blockly-keyboard-experimentation#482 complicates this since state is now inferred from `FocusManager`, and the flyout _losing_ focus will force it closed. There was a fix introduced in this PR to fix it for keyboard navigation, but fails for clicks because the flyout never receives focus when the create variable button is clicked. It also caused the advanced compilation tests to fail due to a subtle circular dependency from importing `WorkspaceSvg` directly rather than its type.
- The flyout, while it stays open, does not automatically update past the first variable being created without closing and reopening it. I'm actually not at all sure why this particular behavior has regressed.

### Test Coverage

No new non-`FocusManager` tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of #8915.

Some new `FocusManager` tests were added, but more are still needed and this is tracked as part of #8910.

### Documentation

No new documentation should be needed for these changes.

### Additional Information

This includes changes that have been pulled from #8875.

Author: BenHenning

core/blockly.ts

@@ -120,6 +120,7 @@ import * as inputs from './inputs.js';
 import {IFlyoutInflater} from './interfaces/i_flyout_inflater.js';
 import {LabelFlyoutInflater} from './label_flyout_inflater.js';
 import {SeparatorFlyoutInflater} from './separator_flyout_inflater.js';
+import {FocusableTreeTraverser} from './utils/focusable_tree_traverser.js';
 
 import {Input} from './inputs/input.js';
 import {InsertionMarkerPreviewer} from './insertion_marker_previewer.js';
@@ -527,6 +528,7 @@ export {
   FlyoutMetricsManager,
   FlyoutSeparator,
   FocusManager,
+  FocusableTreeTraverser,
   CodeGenerator as Generator,
   Gesture,
   Grid,

core/focus_manager.ts

@@ -65,26 +65,15 @@ export class FocusManager {
   constructor(
     addGlobalEventListener: (type: string, listener: EventListener) => void,
   ) {
-    // Register root document focus listeners for tracking when focus leaves all
-    // tracked focusable trees.
-    addGlobalEventListener('focusin', (event) => {
-      if (!(event instanceof FocusEvent)) return;
-
-      // The target that now has focus.
-      const activeElement = document.activeElement;
+    // Note that 'element' here is the element *gaining* focus.
+    const maybeFocus = (element: Element | EventTarget | null) => {
       let newNode: IFocusableNode | null | undefined = null;
-      if (
-        activeElement instanceof HTMLElement ||
-        activeElement instanceof SVGElement
-      ) {
-        // If the target losing focus maps to any tree, then it should be
-        // updated. Per the contract of findFocusableNodeFor only one tree
-        // should claim the element.
+      if (element instanceof HTMLElement || element instanceof SVGElement) {
+        // If the target losing or gaining focus maps to any tree, then it
+        // should be updated. Per the contract of findFocusableNodeFor only one
+        // tree should claim the element, so the search can be exited early.
         for (const tree of this.registeredTrees) {
-          newNode = FocusableTreeTraverser.findFocusableNodeFor(
-            activeElement,
-            tree,
-          );
+          newNode = FocusableTreeTraverser.findFocusableNodeFor(element, tree);
           if (newNode) break;
         }
       }
@@ -103,6 +92,26 @@ export class FocusManager {
       } else {
         this.defocusCurrentFocusedNode();
       }
+    };
+
+    // Register root document focus listeners for tracking when focus leaves all
+    // tracked focusable trees. Note that focusin and focusout can be somewhat
+    // overlapping in the information that they provide. This is fine because
+    // they both aim to check for focus changes on the element gaining or having
+    // received focus, and maybeFocus should behave relatively deterministic.
+    addGlobalEventListener('focusin', (event) => {
+      if (!(event instanceof FocusEvent)) return;
+
+      // When something receives focus, always use the current active element as
+      // the source of truth.
+      maybeFocus(document.activeElement);
+    });
+    addGlobalEventListener('focusout', (event) => {
+      if (!(event instanceof FocusEvent)) return;
+
+      // When something loses focus, it seems that document.activeElement may
+      // not necessarily be correct. Instead, use relatedTarget.
+      maybeFocus(event.relatedTarget);
     });
   }
 
@@ -247,7 +256,7 @@ export class FocusManager {
 
     const prevNode = this.focusedNode;
     const prevTree = prevNode?.getFocusableTree();
-    if (prevNode && prevTree !== nextTree) {
+    if (prevNode) {
       this.passivelyFocusNode(prevNode, nextTree);
     }
 
@@ -374,7 +383,9 @@ export class FocusManager {
     // 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);
+    if (node.getFocusableTree() !== prevTree) {
+      node.getFocusableTree().onTreeFocus(node, prevTree);
+    }
     node.onNodeFocus();
     this.lockFocusStateChanges = false;
 
@@ -399,11 +410,15 @@ export class FocusManager {
     nextTree: IFocusableTree | null,
   ): void {
     this.lockFocusStateChanges = true;
-    node.getFocusableTree().onTreeBlur(nextTree);
+    if (node.getFocusableTree() !== nextTree) {
+      node.getFocusableTree().onTreeBlur(nextTree);
+    }
     node.onNodeBlur();
     this.lockFocusStateChanges = false;
 
-    this.setNodeToVisualPassiveFocus(node);
+    if (node.getFocusableTree() !== nextTree) {
+      this.setNodeToVisualPassiveFocus(node);
+    }
   }
 
   /**
@@ -441,19 +456,24 @@ export class FocusManager {
     dom.removeClass(element, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME);
     dom.removeClass(element, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME);
   }
-}
 
-let focusManager: FocusManager | null = null;
+  private static focusManager: FocusManager | null = null;
 
-/**
- * Returns the page-global FocusManager.
- *
- * The returned instance is guaranteed to not change across function calls, but
- * may change across page loads.
- */
-export function getFocusManager(): FocusManager {
-  if (!focusManager) {
-    focusManager = new FocusManager(document.addEventListener);
+  /**
+   * Returns the page-global FocusManager.
+   *
+   * The returned instance is guaranteed to not change across function calls, but
+   * may change across page loads.
+   */
+  static getFocusManager(): FocusManager {
+    if (!FocusManager.focusManager) {
+      FocusManager.focusManager = new FocusManager(document.addEventListener);
+    }
+    return FocusManager.focusManager;
   }
-  return focusManager;
+}
+
+/** Convenience function for FocusManager.getFocusManager. */
+export function getFocusManager(): FocusManager {
+  return FocusManager.getFocusManager();
 }

core/keyboard_nav/line_cursor.ts

@@ -18,11 +18,9 @@ import {BlockSvg} from '../block_svg.js';
 import * as common from '../common.js';
 import type {Connection} from '../connection.js';
 import {ConnectionType} from '../connection_type.js';
-import type {Abstract} from '../events/events_abstract.js';
-import {Click, ClickTarget} from '../events/events_click.js';
-import {EventType} from '../events/type.js';
-import * as eventUtils from '../events/utils.js';
 import type {Field} from '../field.js';
+import {getFocusManager} from '../focus_manager.js';
+import {isFocusableNode} from '../interfaces/i_focusable_node.js';
 import * as registry from '../registry.js';
 import type {MarkerSvg} from '../renderers/common/marker_svg.js';
 import type {PathObject} from '../renderers/zelos/path_object.js';
@@ -70,23 +68,12 @@ export class LineCursor extends Marker {
     options?: Partial<CursorOptions>,
   ) {
     super();
-    // Bind changeListener to facilitate future disposal.
-    this.changeListener = this.changeListener.bind(this);
-    this.workspace.addChangeListener(this.changeListener);
     // Regularise options and apply defaults.
     this.options = {...defaultOptions, ...options};
 
     this.isZelos = workspace.getRenderer() instanceof Renderer;
   }
 
-  /**
-   * Clean up this cursor.
-   */
-  dispose() {
-    this.workspace.removeChangeListener(this.changeListener);
-    super.dispose();
-  }
-
   /**
    * Moves the cursor to the next previous connection, next connection or block
    * in the pre order traversal. Finds the next node in the pre order traversal.
@@ -331,8 +318,8 @@ export class LineCursor extends Marker {
    * @param node The current position in the AST.
    * @param isValid A function true/false depending on whether the given node
    *     should be traversed.
-   * @param loop Whether to loop around to the beginning of the workspace if
-   *     novalid node was found.
+   * @param loop Whether to loop around to the beginning of the workspace if no
+   *     valid node was found.
    * @returns The next node in the traversal.
    */
   getNextNode(
@@ -385,8 +372,8 @@ export class LineCursor extends Marker {
    * @param node The current position in the AST.
    * @param isValid A function true/false depending on whether the given node
    *     should be traversed.
-   * @param loop Whether to loop around to the end of the workspace if no
-   *     valid node was found.
+   * @param loop Whether to loop around to the end of the workspace if no valid
+   *     node was found.
    * @returns The previous node in the traversal or null if no previous node
    *     exists.
    */
@@ -527,7 +514,12 @@ export class LineCursor extends Marker {
    * @returns The current field, connection, or block the cursor is on.
    */
   override getCurNode(): ASTNode | null {
-    this.updateCurNodeFromSelection();
+    if (!this.updateCurNodeFromFocus()) {
+      // Fall back to selection if focus fails to sync. This can happen for
+      // non-focusable nodes or for cases when focus may not properly propagate
+      // (such as for mouse clicks).
+      this.updateCurNodeFromSelection();
+    }
     return super.getCurNode();
   }
 
@@ -572,16 +564,15 @@ export class LineCursor extends Marker {
    * this.drawMarker() instead of this.drawer.draw() directly.
    *
    * @param newNode The new location of the cursor.
-   * @param updateSelection If true (the default) we'll update the selection
-   *     too.
    */
-  override setCurNode(newNode: ASTNode | null, updateSelection = true) {
-    if (updateSelection) {
-      this.updateSelectionFromNode(newNode);
-    }
-
+  override setCurNode(newNode: ASTNode | null) {
     super.setCurNode(newNode);
 
+    const newNodeLocation = newNode?.getLocation();
+    if (isFocusableNode(newNodeLocation)) {
+      getFocusManager().focusNode(newNodeLocation);
+    }
+
     // Try to scroll cursor into view.
     if (newNode?.getType() === ASTNode.types.BLOCK) {
       const block = newNode.getLocation() as BlockSvg;
@@ -709,32 +700,6 @@ export class LineCursor extends Marker {
     }
   }
 
-  /**
-   * Event listener that syncs the cursor location to the selected block on
-   * SELECTED events.
-   *
-   * This does not run early enough in all cases so `getCurNode()` also updates
-   * the node from the selection.
-   *
-   * @param event The `Selected` event.
-   */
-  private changeListener(event: Abstract) {
-    switch (event.type) {
-      case EventType.SELECTED:
-        this.updateCurNodeFromSelection();
-        break;
-      case EventType.CLICK: {
-        const click = event as Click;
-        if (
-          click.workspaceId === this.workspace.id &&
-          click.targetType === ClickTarget.WORKSPACE
-        ) {
-          this.setCurNode(null);
-        }
-      }
-    }
-  }
-
   /**
    * Updates the current node to match the selection.
    *
@@ -749,7 +714,7 @@ export class LineCursor extends Marker {
     const selected = common.getSelected();
 
     if (selected === null && curNode?.getType() === ASTNode.types.BLOCK) {
-      this.setCurNode(null, false);
+      this.setCurNode(null);
       return;
     }
     if (selected?.workspace !== this.workspace) {
@@ -770,36 +735,35 @@ export class LineCursor extends Marker {
         block = block.getParent();
       }
       if (block) {
-        this.setCurNode(ASTNode.createBlockNode(block), false);
+        this.setCurNode(ASTNode.createBlockNode(block));
       }
     }
   }
 
   /**
-   * Updates the selection from the node.
+   * Updates the current node to match what's currently focused.
    *
-   * Clears the selection for non-block nodes.
-   * Clears the selection for shadow blocks as the selection is drawn on
-   * the parent but the cursor will be drawn on the shadow block itself.
-   * We need to take care not to later clear the current node due to that null
-   * selection, so we track the latest selection we're in sync with.
-   *
-   * @param newNode The new node.
+   * @returns Whether the current node has been set successfully from the
+   *     current focused node.
    */
-  private updateSelectionFromNode(newNode: ASTNode | null) {
-    if (newNode?.getType() === ASTNode.types.BLOCK) {
-      if (common.getSelected() !== newNode.getLocation()) {
-        eventUtils.disable();
-        common.setSelected(newNode.getLocation() as BlockSvg);
-        eventUtils.enable();
-      }
-    } else {
-      if (common.getSelected()) {
-        eventUtils.disable();
-        common.setSelected(null);
-        eventUtils.enable();
+  private updateCurNodeFromFocus(): boolean {
+    const focused = getFocusManager().getFocusedNode();
+
+    if (focused instanceof BlockSvg) {
+      const block: BlockSvg | null = focused;
+      if (block && block.workspace === this.workspace) {
+        if (block.getRootBlock() === block && this.workspace.isFlyout) {
+          // This block actually represents a stack. Note that this is needed
+          // because ASTNode special cases stack for cross-block navigation.
+          this.setCurNode(ASTNode.createStackNode(block));
+        } else {
+          this.setCurNode(ASTNode.createBlockNode(block));
+        }
+        return true;
       }
     }
+
+    return false;
   }
 
   /**

tests/mocha/cursor_test.js

@@ -6,6 +6,7 @@
 
 import {ASTNode} from '../../build/src/core/keyboard_nav/ast_node.js';
 import {assert} from '../../node_modules/chai/chai.js';
+import {createRenderedBlock} from './test_helpers/block_definitions.js';
 import {
   sharedTestSetup,
   sharedTestTeardown,
@@ -63,11 +64,11 @@ suite('Cursor', function () {
       ]);
       this.workspace = Blockly.inject('blocklyDiv', {});
       this.cursor = this.workspace.getCursor();
-      const blockA = this.workspace.newBlock('input_statement');
-      const blockB = this.workspace.newBlock('input_statement');
-      const blockC = this.workspace.newBlock('input_statement');
-      const blockD = this.workspace.newBlock('input_statement');
-      const blockE = this.workspace.newBlock('field_input');
+      const blockA = createRenderedBlock(this.workspace, 'input_statement');
+      const blockB = createRenderedBlock(this.workspace, 'input_statement');
+      const blockC = createRenderedBlock(this.workspace, 'input_statement');
+      const blockD = createRenderedBlock(this.workspace, 'input_statement');
+      const blockE = createRenderedBlock(this.workspace, 'field_input');
 
       blockA.nextConnection.connect(blockB.previousConnection);
       blockA.inputList[0].connection.connect(blockE.outputConnection);
@@ -105,12 +106,12 @@ suite('Cursor', function () {
       );
     });
 
-    test('In - From output connection', function () {
+    test('In - From attached input connection', function () {
       const fieldBlock = this.blocks.E;
-      const outputNode = ASTNode.createConnectionNode(
-        fieldBlock.outputConnection,
+      const inputConnectionNode = ASTNode.createConnectionNode(
+        this.blocks.A.inputList[0].connection,
       );
-      this.cursor.setCurNode(outputNode);
+      this.cursor.setCurNode(inputConnectionNode);
       this.cursor.in();
       const curNode = this.cursor.getCurNode();
       assert.equal(curNode.getLocation(), fieldBlock);