fix: Improve narration and navigation of C-shaped blocks. (#9416)

* fix: Improve narration and navigation of C-shaped blocks.

* chore: Satisfy the linter.

* chore: Refactor and comment `getBlockNavigationCandidates()`.

* refactor: Reduce code duplication in `LineCursor`.

* fix: Add missing case when labeling connections.

core/block_svg.ts

@@ -39,6 +39,7 @@ import {IconType} from './icons/icon_types.js';
 import {MutatorIcon} from './icons/mutator_icon.js';
 import {WarningIcon} from './icons/warning_icon.js';
 import type {Input} from './inputs/input.js';
+import {inputTypes} from './inputs/input_types.js';
 import type {IBoundedElement} from './interfaces/i_bounded_element.js';
 import {IContextMenu} from './interfaces/i_contextmenu.js';
 import type {ICopyable} from './interfaces/i_copyable.js';
@@ -267,6 +268,20 @@ export class BlockSvg
       blockTypeText = 'C-shaped block';
     }
 
+    let prefix = '';
+    const parentInput = (
+      this.previousConnection ?? this.outputConnection
+    )?.targetConnection?.getParentInput();
+    if (parentInput && parentInput.type === inputTypes.STATEMENT) {
+      prefix = `Begin ${parentInput.getFieldRowLabel()}, `;
+    } else if (
+      parentInput &&
+      parentInput.type === inputTypes.VALUE &&
+      this.getParent()?.statementInputCount
+    ) {
+      prefix = `${parentInput.getFieldRowLabel()} `;
+    }
+
     let additionalInfo = blockTypeText;
     if (inputSummary && !nestedStatementBlockCount) {
       additionalInfo = `${additionalInfo} with ${inputSummary}`;
@@ -279,7 +294,7 @@ export class BlockSvg
       }
     }
 
-    return blockSummary + ', ' + additionalInfo;
+    return prefix + blockSummary + ', ' + additionalInfo;
   }
 
   private computeAriaRole() {

core/inputs/input.ts

@@ -303,6 +303,21 @@ export class Input {
     }
   }
 
+  /**
+   * Returns a label for this input's row on its parent block.
+   *
+   * Generally this consists of the labels/values of the preceding fields, and
+   * is intended for accessibility descriptions.
+   *
+   * @internal
+   * @returns A description of this input's row on its parent block.
+   */
+  getFieldRowLabel() {
+    return this.fieldRow.reduce((label, field) => {
+      return `${label} ${field.EDITABLE ? field.getAriaName() : field.getValue()}`;
+    }, '');
+  }
+
   /**
    * Constructs a connection based on the type of this input's source block.
    * Properly handles constructing headless connections for headless blocks

core/keyboard_nav/block_navigation_policy.ts

@@ -124,24 +124,48 @@ function getBlockNavigationCandidates(
 
   for (const input of block.inputList) {
     if (!input.isVisible()) continue;
+
     candidates.push(...input.fieldRow);
-    if (input.connection?.targetBlock()) {
-      const connectedBlock = input.connection.targetBlock() as BlockSvg;
-      if (input.connection.type === ConnectionType.NEXT_STATEMENT && !forward) {
+
+    const connection = input.connection as RenderedConnection | null;
+    if (!connection) continue;
+
+    const connectedBlock = connection.targetBlock();
+    if (connectedBlock) {
+      if (connection.type === ConnectionType.NEXT_STATEMENT && !forward) {
         const lastStackBlock = connectedBlock
           .lastConnectionInStack(false)
           ?.getSourceBlock();
         if (lastStackBlock) {
+          // When navigating backward, the last block in a stack in a statement
+          // input is navigable.
           candidates.push(lastStackBlock);
         }
       } else {
+        // When navigating forward, a child block connected to a statement
+        // input is navigable.
         candidates.push(connectedBlock);
       }
-    } else if (input.connection?.type === ConnectionType.INPUT_VALUE) {
-      candidates.push(input.connection as RenderedConnection);
+    } else if (
+      connection.type === ConnectionType.INPUT_VALUE ||
+      connection.type === ConnectionType.NEXT_STATEMENT
+    ) {
+      // Empty input or statement connections are navigable.
+      candidates.push(connection);
     }
   }
 
+  if (
+    block.nextConnection &&
+    !block.nextConnection.targetBlock() &&
+    (block.lastConnectionInStack(true) !== block.nextConnection ||
+      !!block.getSurroundParent())
+  ) {
+    // The empty next connection on the last block in a stack inside of a
+    // statement input is navigable.
+    candidates.push(block.nextConnection);
+  }
+
   return candidates;
 }
 

core/keyboard_nav/line_cursor.ts

@@ -15,12 +15,24 @@
 
 import {BlockSvg} from '../block_svg.js';
 import {RenderedWorkspaceComment} from '../comments/rendered_workspace_comment.js';
+import {ConnectionType} from '../connection_type.js';
 import {getFocusManager} from '../focus_manager.js';
 import type {IFocusableNode} from '../interfaces/i_focusable_node.js';
 import * as registry from '../registry.js';
+import {RenderedConnection} from '../rendered_connection.js';
 import type {WorkspaceSvg} from '../workspace_svg.js';
 import {Marker} from './marker.js';
 
+/**
+ * Representation of the direction of travel within a navigation context.
+ */
+export enum NavigationDirection {
+  NEXT,
+  PREVIOUS,
+  IN,
+  OUT,
+}
+
 /**
  * Class for a line cursor.
  */
@@ -51,13 +63,7 @@ export class LineCursor extends Marker {
     }
     const newNode = this.getNextNode(
       curNode,
-      (candidate: IFocusableNode | null) => {
-        return (
-          (candidate instanceof BlockSvg &&
-            !candidate.outputConnection?.targetBlock()) ||
-          candidate instanceof RenderedWorkspaceComment
-        );
-      },
+      this.getValidationFunction(NavigationDirection.NEXT),
       true,
     );
 
@@ -80,7 +86,11 @@ export class LineCursor extends Marker {
       return null;
     }
 
-    const newNode = this.getNextNode(curNode, () => true, true);
+    const newNode = this.getNextNode(
+      curNode,
+      this.getValidationFunction(NavigationDirection.IN),
+      true,
+    );
 
     if (newNode) {
       this.setCurNode(newNode);
@@ -101,13 +111,7 @@ export class LineCursor extends Marker {
     }
     const newNode = this.getPreviousNode(
       curNode,
-      (candidate: IFocusableNode | null) => {
-        return (
-          (candidate instanceof BlockSvg &&
-            !candidate.outputConnection?.targetBlock()) ||
-          candidate instanceof RenderedWorkspaceComment
-        );
-      },
+      this.getValidationFunction(NavigationDirection.PREVIOUS),
       true,
     );
 
@@ -130,7 +134,11 @@ export class LineCursor extends Marker {
       return null;
     }
 
-    const newNode = this.getPreviousNode(curNode, () => true, true);
+    const newNode = this.getPreviousNode(
+      curNode,
+      this.getValidationFunction(NavigationDirection.OUT),
+      true,
+    );
 
     if (newNode) {
       this.setCurNode(newNode);
@@ -147,15 +155,14 @@ export class LineCursor extends Marker {
   atEndOfLine(): boolean {
     const curNode = this.getCurNode();
     if (!curNode) return false;
-    const inNode = this.getNextNode(curNode, () => true, true);
+    const inNode = this.getNextNode(
+      curNode,
+      this.getValidationFunction(NavigationDirection.IN),
+      true,
+    );
     const nextNode = this.getNextNode(
       curNode,
-      (candidate: IFocusableNode | null) => {
-        return (
-          candidate instanceof BlockSvg &&
-          !candidate.outputConnection?.targetBlock()
-        );
-      },
+      this.getValidationFunction(NavigationDirection.NEXT),
       true,
     );
 
@@ -298,6 +305,92 @@ export class LineCursor extends Marker {
     return this.getRightMostChild(newNode, stopIfFound);
   }
 
+  /**
+   * Returns a function that will be used to determine whether a candidate for
+   * navigation is valid.
+   *
+   * @param direction The direction in which the user is navigating.
+   * @returns A function that takes a proposed navigation candidate and returns
+   *     true if navigation should be allowed to proceed to it, or false to find
+   *     a different candidate.
+   */
+  getValidationFunction(
+    direction: NavigationDirection,
+  ): (node: IFocusableNode | null) => boolean {
+    switch (direction) {
+      case NavigationDirection.IN:
+      case NavigationDirection.OUT:
+        return () => true;
+      case NavigationDirection.NEXT:
+      case NavigationDirection.PREVIOUS:
+        return (candidate: IFocusableNode | null) => {
+          if (
+            (candidate instanceof BlockSvg &&
+              !candidate.outputConnection?.targetBlock()) ||
+            candidate instanceof RenderedWorkspaceComment ||
+            (candidate instanceof RenderedConnection &&
+              (candidate.type === ConnectionType.NEXT_STATEMENT ||
+                (candidate.type === ConnectionType.INPUT_VALUE &&
+                  candidate.getSourceBlock().statementInputCount &&
+                  candidate.getSourceBlock().inputList[0] !==
+                    candidate.getParentInput())))
+          ) {
+            return true;
+          }
+
+          const current = this.getSourceBlockFromNode(this.getCurNode());
+          if (candidate instanceof BlockSvg && current instanceof BlockSvg) {
+            // If the candidate's parent uses inline inputs, disallow the
+            // candidate; it follows that it must be on the same row as its
+            // parent.
+            if (candidate.outputConnection?.targetBlock()?.getInputsInline()) {
+              return false;
+            }
+
+            const candidateParents = this.getParents(candidate);
+            // If the candidate block is an (in)direct child of the current
+            // block, disallow it; it cannot be on a different row than the
+            // current block.
+            if (
+              current === this.getCurNode() &&
+              candidateParents.has(current)
+            ) {
+              return false;
+            }
+
+            const currentParents = this.getParents(current);
+
+            const sharedParents = currentParents.intersection(candidateParents);
+            // Allow the candidate if it and the current block have no parents
+            // in common, or if they have a shared parent with external inputs.
+            const result =
+              !sharedParents.size ||
+              sharedParents.values().some((block) => !block.getInputsInline());
+            return result;
+          }
+
+          return false;
+        };
+    }
+  }
+
+  /**
+   * Returns a set of all of the parent blocks of the given block.
+   *
+   * @param block The block to retrieve the parents of.
+   * @returns A set of the parents of the given block.
+   */
+  private getParents(block: BlockSvg): Set<BlockSvg> {
+    const parents = new Set<BlockSvg>();
+    let parent = block.getParent();
+    while (parent) {
+      parents.add(parent);
+      parent = parent.getParent();
+    }
+
+    return parents;
+  }
+
   /**
    * Prepare for the deletion of a block by making a list of nodes we
    * could move the cursor to afterwards and save it to

core/rendered_connection.ts

@@ -20,6 +20,7 @@ import {ConnectionType} from './connection_type.js';
 import * as ContextMenu from './contextmenu.js';
 import {ContextMenuRegistry} from './contextmenu_registry.js';
 import * as eventUtils from './events/utils.js';
+import {inputTypes} from './inputs/input_types.js';
 import {IContextMenu} from './interfaces/i_contextmenu.js';
 import type {IFocusableNode} from './interfaces/i_focusable_node.js';
 import type {IFocusableTree} from './interfaces/i_focusable_tree.js';
@@ -334,7 +335,32 @@ export class RenderedConnection
     if (highlightSvg) {
       highlightSvg.style.display = '';
       aria.setRole(highlightSvg, aria.Role.FIGURE);
-      aria.setState(highlightSvg, aria.State.LABEL, 'Open connection');
+      aria.setState(highlightSvg, aria.State.ROLEDESCRIPTION, 'Connection');
+      if (this.type === ConnectionType.NEXT_STATEMENT) {
+        const parentInput =
+          this.getParentInput() ??
+          this.getSourceBlock()
+            .getTopStackBlock()
+            .previousConnection?.targetConnection?.getParentInput();
+        if (parentInput && parentInput.type === inputTypes.STATEMENT) {
+          aria.setState(
+            highlightSvg,
+            aria.State.LABEL,
+            `${this.getParentInput() ? 'Begin' : 'End'} ${parentInput.getFieldRowLabel()}`,
+          );
+        }
+      } else if (
+        this.type === ConnectionType.INPUT_VALUE &&
+        this.getSourceBlock().statementInputCount
+      ) {
+        aria.setState(
+          highlightSvg,
+          aria.State.LABEL,
+          `${this.getParentInput()?.getFieldRowLabel()}`,
+        );
+      } else {
+        aria.setState(highlightSvg, aria.State.LABEL, 'Open connection');
+      }
     }
   }
 

tests/mocha/cursor_test.js

@@ -136,22 +136,22 @@ suite('Cursor', function () {
       assert.equal(curNode, fieldBlock);
     });
 
-    test('Prev - From previous connection does skip over next connection', function () {
+    test('Prev - From previous connection does not skip over next connection', function () {
       const prevConnection = this.blocks.B.previousConnection;
       const prevConnectionNode = prevConnection;
       this.cursor.setCurNode(prevConnectionNode);
       this.cursor.prev();
       const curNode = this.cursor.getCurNode();
-      assert.equal(curNode, this.blocks.A);
+      assert.equal(curNode, this.blocks.A.nextConnection);
     });
 
-    test('Prev - From first block loop to last block', function () {
+    test('Prev - From first block loop to last statement input', function () {
       const prevConnection = this.blocks.A;
       const prevConnectionNode = prevConnection;
       this.cursor.setCurNode(prevConnectionNode);
       this.cursor.prev();
       const curNode = this.cursor.getCurNode();
-      assert.equal(curNode, this.blocks.D);
+      assert.equal(curNode, this.blocks.D.getInput('NAME4').connection);
     });
 
     test('Out - From field does not skip over block node', function () {
@@ -253,12 +253,16 @@ suite('Cursor', function () {
     test('In - from field in nested statement block to next nested statement block', function () {
       this.cursor.setCurNode(this.secondStatement.getField('NAME'));
       this.cursor.in();
+      // Skip over the next connection
+      this.cursor.in();
       const curNode = this.cursor.getCurNode();
       assert.equal(curNode, this.thirdStatement);
     });
     test('In - from field in nested statement block to next stack', function () {
       this.cursor.setCurNode(this.thirdStatement.getField('NAME'));
       this.cursor.in();
+      // Skip over the next connection
+      this.cursor.in();
       const curNode = this.cursor.getCurNode();
       assert.equal(curNode, this.multiStatement2);
     });
@@ -266,6 +270,8 @@ suite('Cursor', function () {
     test('Out - from nested statement block to last field of previous nested statement block', function () {
       this.cursor.setCurNode(this.thirdStatement);
       this.cursor.out();
+      // Skip over the previous next connection
+      this.cursor.out();
       const curNode = this.cursor.getCurNode();
       assert.equal(curNode, this.secondStatement.getField('NAME'));
     });
@@ -273,6 +279,8 @@ suite('Cursor', function () {
     test('Out - from root block to last field of last nested statement block in previous stack', function () {
       this.cursor.setCurNode(this.multiStatement2);
       this.cursor.out();
+      // Skip over the previous next connection
+      this.cursor.out();
       const curNode = this.cursor.getCurNode();
       assert.equal(curNode, this.thirdStatement.getField('NAME'));
     });
@@ -395,7 +403,7 @@ suite('Cursor', function () {
       });
       test('getLastNode', function () {
         const node = this.cursor.getLastNode();
-        assert.equal(node, this.blockA);
+        assert.equal(node, this.blockA.inputList[0].connection);
       });
     });
 

tests/mocha/navigation_test.js

@@ -531,12 +531,13 @@ suite('Navigation', function () {
         );
         assert.equal(nextNode, field);
       });
-      test('fromBlockToFieldSkippingInput', function () {
-        const field = this.blocks.buttonBlock.getField('BUTTON3');
+      test('fromInputToStatementConnection', function () {
+        const connection =
+          this.blocks.buttonBlock.getInput('STATEMENT1').connection;
         const nextNode = this.navigator.getNextSibling(
           this.blocks.buttonInput2,
         );
-        assert.equal(nextNode, field);
+        assert.equal(nextNode, connection);
       });
       test('skipsChildrenOfCollapsedBlocks', function () {
         this.blocks.buttonBlock.setCollapsed(true);
@@ -546,9 +547,9 @@ suite('Navigation', function () {
       test('fromFieldSkipsHiddenInputs', function () {
         this.blocks.buttonBlock.inputList[2].setVisible(false);
         const fieldStart = this.blocks.buttonBlock.getField('BUTTON2');
-        const fieldEnd = this.blocks.buttonBlock.getField('BUTTON3');
+        const end = this.blocks.buttonBlock.getInput('STATEMENT1').connection;
         const nextNode = this.navigator.getNextSibling(fieldStart);
-        assert.equal(nextNode.name, fieldEnd.name);
+        assert.equal(nextNode, end);
       });
     });
 
@@ -693,9 +694,9 @@ suite('Navigation', function () {
       test('fromFieldSkipsHiddenInputs', function () {
         this.blocks.buttonBlock.inputList[2].setVisible(false);
         const fieldStart = this.blocks.buttonBlock.getField('BUTTON3');
-        const fieldEnd = this.blocks.buttonBlock.getField('BUTTON2');
+        const end = this.blocks.buttonBlock.getInput('STATEMENT1').connection;
         const nextNode = this.navigator.getPreviousSibling(fieldStart);
-        assert.equal(nextNode.name, fieldEnd.name);
+        assert.equal(nextNode, end);
       });
     });
 

tsconfig.json

@@ -15,6 +15,7 @@
     "moduleResolution": "node",
     "target": "ES2020",
     "strict": true,
+    "lib": ["esnext", "dom"],
 
     // This does not understand enums only used to define other enums, so we
     // cannot leave it enabled.