feat: Allow visiting empty input connections. (#9104)

* feat: Update navigation policies to allow visiting empty input connections.

* fix: Fix tests.

* chore: Add JSDoc.

* fix: Add missing import.

* fix: Fix JSDoc.

* chore: Remove double comments.

core/keyboard_nav/block_navigation_policy.ts

@@ -5,9 +5,12 @@
  */
 
 import {BlockSvg} from '../block_svg.js';
+import {ConnectionType} from '../connection_type.js';
 import type {Field} from '../field.js';
+import type {Icon} from '../icons/icon.js';
 import type {IFocusableNode} from '../interfaces/i_focusable_node.js';
 import type {INavigationPolicy} from '../interfaces/i_navigation_policy.js';
+import {RenderedConnection} from '../rendered_connection.js';
 import {WorkspaceSvg} from '../workspace_svg.js';
 
 /**
@@ -21,21 +24,8 @@ export class BlockNavigationPolicy implements INavigationPolicy<BlockSvg> {
    * @returns The first field or input of the given block, if any.
    */
   getFirstChild(current: BlockSvg): IFocusableNode | null {
-    const icons = current.getIcons();
+    const candidates = getBlockNavigationCandidates(current);
-    if (icons.length) return icons[0];
+    return candidates[0];
-
-    for (const input of current.inputList) {
-      if (!input.isVisible()) {
-        continue;
-      }
-      for (const field of input.fieldRow) {
-        return field;
-      }
-      if (input.connection?.targetBlock())
-        return input.connection.targetBlock() as BlockSvg;
-    }
-
-    return null;
   }
 
   /**
@@ -66,36 +56,10 @@ export class BlockNavigationPolicy implements INavigationPolicy<BlockSvg> {
   getNextSibling(current: BlockSvg): IFocusableNode | null {
     if (current.nextConnection?.targetBlock()) {
       return current.nextConnection?.targetBlock();
-    }
+    } else if (current.outputConnection?.targetBlock()) {
-
+      return navigateBlock(current, 1);
-    const parent = this.getParent(current);
+    } else if (this.getParent(current) instanceof WorkspaceSvg) {
-    let navigatingCrossStacks = false;
+      return navigateStacks(current, 1);
-    let siblings: (BlockSvg | Field)[] = [];
-    if (parent instanceof BlockSvg) {
-      for (let i = 0, input; (input = parent.inputList[i]); i++) {
-        if (!input.isVisible()) {
-          continue;
-        }
-        siblings.push(...input.fieldRow);
-        const child = input.connection?.targetBlock();
-        if (child) {
-          siblings.push(child as BlockSvg);
-        }
-      }
-    } else if (parent instanceof WorkspaceSvg) {
-      siblings = parent.getTopBlocks(true);
-      navigatingCrossStacks = true;
-    } else {
-      return null;
-    }
-
-    const currentIndex = siblings.indexOf(
-      navigatingCrossStacks ? current.getRootBlock() : current,
-    );
-    if (currentIndex >= 0 && currentIndex < siblings.length - 1) {
-      return siblings[currentIndex + 1];
-    } else if (currentIndex === siblings.length - 1 && navigatingCrossStacks) {
-      return siblings[0];
     }
 
     return null;
@@ -111,46 +75,15 @@ export class BlockNavigationPolicy implements INavigationPolicy<BlockSvg> {
   getPreviousSibling(current: BlockSvg): IFocusableNode | null {
     if (current.previousConnection?.targetBlock()) {
       return current.previousConnection?.targetBlock();
+    } else if (current.outputConnection?.targetBlock()) {
+      return navigateBlock(current, -1);
+    } else if (this.getParent(current) instanceof WorkspaceSvg) {
+      return navigateStacks(current, -1);
     }
 
-    const parent = this.getParent(current);
-    let navigatingCrossStacks = false;
-    let siblings: (BlockSvg | Field)[] = [];
-    if (parent instanceof BlockSvg) {
-      for (let i = 0, input; (input = parent.inputList[i]); i++) {
-        if (!input.isVisible()) {
-          continue;
-        }
-        siblings.push(...input.fieldRow);
-        const child = input.connection?.targetBlock();
-        if (child) {
-          siblings.push(child as BlockSvg);
-        }
-      }
-    } else if (parent instanceof WorkspaceSvg) {
-      siblings = parent.getTopBlocks(true);
-      navigatingCrossStacks = true;
-    } else {
     return null;
   }
 
-    const currentIndex = siblings.indexOf(current);
-    let result: IFocusableNode | null = null;
-    if (currentIndex >= 1) {
-      result = siblings[currentIndex - 1];
-    } else if (currentIndex === 0 && navigatingCrossStacks) {
-      result = siblings[siblings.length - 1];
-    }
-
-    // If navigating to a previous stack, our previous sibling is the last
-    // block in it.
-    if (navigatingCrossStacks && result instanceof BlockSvg) {
-      return result.lastConnectionInStack(false)?.getSourceBlock() ?? result;
-    }
-
-    return result;
-  }
-
   /**
    * Returns whether or not the given block can be navigated to.
    *
@@ -171,3 +104,88 @@ export class BlockNavigationPolicy implements INavigationPolicy<BlockSvg> {
     return current instanceof BlockSvg;
   }
 }
+
+/**
+ * Returns a list of the navigable children of the given block.
+ *
+ * @param block The block to retrieve the navigable children of.
+ * @returns A list of navigable/focusable children of the given block.
+ */
+function getBlockNavigationCandidates(block: BlockSvg): IFocusableNode[] {
+  const candidates: IFocusableNode[] = block.getIcons();
+
+  for (const input of block.inputList) {
+    if (!input.isVisible()) continue;
+    candidates.push(...input.fieldRow);
+    if (input.connection?.targetBlock()) {
+      candidates.push(input.connection.targetBlock() as BlockSvg);
+    } else if (input.connection?.type === ConnectionType.INPUT_VALUE) {
+      candidates.push(input.connection as RenderedConnection);
+    }
+  }
+
+  return candidates;
+}
+
+/**
+ * Returns the next/previous stack relative to the given block's stack.
+ *
+ * @param current The block whose stack will be navigated relative to.
+ * @param delta The difference in index to navigate; positive values navigate
+ *     to the nth next stack, while negative values navigate to the nth previous
+ *     stack.
+ * @returns The first block in the stack offset by `delta` relative to the
+ *     current block's stack, or the last block in the stack offset by `delta`
+ *     relative to the current block's stack when navigating backwards.
+ */
+export function navigateStacks(current: BlockSvg, delta: number) {
+  const stacks = current.workspace.getTopBlocks(true);
+  const currentIndex = stacks.indexOf(current.getRootBlock());
+  const targetIndex = currentIndex + delta;
+  let result: BlockSvg | null = null;
+  if (targetIndex >= 0 && targetIndex < stacks.length) {
+    result = stacks[targetIndex];
+  } else if (targetIndex < 0) {
+    result = stacks[stacks.length - 1];
+  } else if (targetIndex >= stacks.length) {
+    result = stacks[0];
+  }
+
+  // When navigating to a previous stack, our previous sibling is the last
+  // block in it.
+  if (delta < 0 && result) {
+    return result.lastConnectionInStack(false)?.getSourceBlock() ?? result;
+  }
+
+  return result;
+}
+
+/**
+ * Returns the next navigable item relative to the provided block child.
+ *
+ * @param current The navigable block child item to navigate relative to.
+ * @param delta The difference in index to navigate; positive values navigate
+ *     forward by n, while negative values navigate backwards by n.
+ * @returns The navigable block child offset by `delta` relative to `current`.
+ */
+export function navigateBlock(
+  current: Icon | Field | RenderedConnection | BlockSvg,
+  delta: number,
+): IFocusableNode | null {
+  const block =
+    current instanceof BlockSvg
+      ? current.outputConnection.targetBlock()
+      : current.getSourceBlock();
+  if (!(block instanceof BlockSvg)) return null;
+
+  const candidates = getBlockNavigationCandidates(block);
+  const currentIndex = candidates.indexOf(current);
+  if (currentIndex === -1) return null;
+
+  const targetIndex = currentIndex + delta;
+  if (targetIndex >= 0 && targetIndex < candidates.length) {
+    return candidates[targetIndex];
+  }
+
+  return null;
+}

core/keyboard_nav/connection_navigation_policy.ts

@@ -9,6 +9,7 @@ import {ConnectionType} from '../connection_type.js';
 import type {IFocusableNode} from '../interfaces/i_focusable_node.js';
 import type {INavigationPolicy} from '../interfaces/i_navigation_policy.js';
 import {RenderedConnection} from '../rendered_connection.js';
+import {navigateBlock} from './block_navigation_policy.js';
 
 /**
  * Set of rules controlling keyboard navigation from a connection.
@@ -37,19 +38,9 @@ export class ConnectionNavigationPolicy
    * @returns The given connection's parent connection or block.
    */
   getParent(current: RenderedConnection): IFocusableNode | null {
-    if (current.type === ConnectionType.OUTPUT_VALUE) {
-      return current.targetConnection ?? current.getSourceBlock();
-    } else if (current.getParentInput()) {
     return current.getSourceBlock();
   }
 
-    const topBlock = current.getSourceBlock().getTopStackBlock();
-    return (
-      (this.getParentConnection(topBlock)?.targetConnection?.getParentInput()
-        ?.connection as RenderedConnection) ?? topBlock
-    );
-  }
-
   /**
    * Returns the next element following the given connection.
    *
@@ -58,19 +49,7 @@ export class ConnectionNavigationPolicy
    */
   getNextSibling(current: RenderedConnection): IFocusableNode | null {
     if (current.getParentInput()) {
-      const parentInput = current.getParentInput();
+      return navigateBlock(current, 1);
-      const block = parentInput?.getSourceBlock();
-      if (!block || !parentInput) return null;
-
-      const curIdx = block.inputList.indexOf(parentInput);
-      for (let i = curIdx + 1; i < block.inputList.length; i++) {
-        const input = block.inputList[i];
-        const fieldRow = input.fieldRow;
-        if (fieldRow.length) return fieldRow[0];
-        if (input.connection) return input.connection as RenderedConnection;
-      }
-
-      return null;
     } else if (current.type === ConnectionType.NEXT_STATEMENT) {
       const nextBlock = current.targetConnection;
       // If this connection is the last one in the stack, our next sibling is
@@ -103,20 +82,7 @@ export class ConnectionNavigationPolicy
    */
   getPreviousSibling(current: RenderedConnection): IFocusableNode | null {
     if (current.getParentInput()) {
-      const parentInput = current.getParentInput();
+      return navigateBlock(current, -1);
-      const block = parentInput?.getSourceBlock();
-      if (!block || !parentInput) return null;
-
-      const curIdx = block.inputList.indexOf(parentInput);
-      for (let i = curIdx; i >= 0; i--) {
-        const input = block.inputList[i];
-        if (input.connection && input !== parentInput) {
-          return input.connection as RenderedConnection;
-        }
-        const fieldRow = input.fieldRow;
-        if (fieldRow.length) return fieldRow[fieldRow.length - 1];
-      }
-      return null;
     } else if (
       current.type === ConnectionType.PREVIOUS_STATEMENT ||
       current.type === ConnectionType.OUTPUT_VALUE

core/keyboard_nav/field_navigation_policy.ts

@@ -8,6 +8,7 @@ import type {BlockSvg} from '../block_svg.js';
 import {Field} from '../field.js';
 import type {IFocusableNode} from '../interfaces/i_focusable_node.js';
 import type {INavigationPolicy} from '../interfaces/i_navigation_policy.js';
+import {navigateBlock} from './block_navigation_policy.js';
 
 /**
  * Set of rules controlling keyboard navigation from a field.
@@ -40,24 +41,7 @@ export class FieldNavigationPolicy implements INavigationPolicy<Field<any>> {
    * @returns The next field or input in the given field's block.
    */
   getNextSibling(current: Field<any>): IFocusableNode | null {
-    const input = current.getParentInput();
+    return navigateBlock(current, 1);
-    const block = current.getSourceBlock();
-    if (!block) return null;
-
-    const curIdx = block.inputList.indexOf(input);
-    let fieldIdx = input.fieldRow.indexOf(current) + 1;
-    for (let i = curIdx; i < block.inputList.length; i++) {
-      const newInput = block.inputList[i];
-      if (newInput.isVisible()) {
-        const fieldRow = newInput.fieldRow;
-        if (fieldIdx < fieldRow.length) return fieldRow[fieldIdx];
-        if (newInput.connection?.targetBlock()) {
-          return newInput.connection.targetBlock() as BlockSvg;
-        }
-      }
-      fieldIdx = 0;
-    }
-    return null;
   }
 
   /**
@@ -67,29 +51,7 @@ export class FieldNavigationPolicy implements INavigationPolicy<Field<any>> {
    * @returns The preceding field or input in the given field's block.
    */
   getPreviousSibling(current: Field<any>): IFocusableNode | null {
-    const parentInput = current.getParentInput();
+    return navigateBlock(current, -1);
-    const block = current.getSourceBlock();
-    if (!block) return null;
-
-    const curIdx = block.inputList.indexOf(parentInput);
-    let fieldIdx = parentInput.fieldRow.indexOf(current) - 1;
-    for (let i = curIdx; i >= 0; i--) {
-      const input = block.inputList[i];
-      if (input.isVisible()) {
-        if (input.connection?.targetBlock() && input !== parentInput) {
-          return input.connection.targetBlock() as BlockSvg;
-        }
-        const fieldRow = input.fieldRow;
-        if (fieldIdx > -1) return fieldRow[fieldIdx];
-      }
-      // Reset the fieldIdx to the length of the field row of the previous
-      // input.
-      if (i - 1 >= 0) {
-        fieldIdx = block.inputList[i - 1].fieldRow.length - 1;
-      }
-    }
-
-    return block.getIcons().pop() ?? null;
   }
 
   /**

core/keyboard_nav/icon_navigation_policy.ts

@@ -8,6 +8,7 @@ import {BlockSvg} from '../block_svg.js';
 import {Icon} from '../icons/icon.js';
 import type {IFocusableNode} from '../interfaces/i_focusable_node.js';
 import type {INavigationPolicy} from '../interfaces/i_navigation_policy.js';
+import {navigateBlock} from './block_navigation_policy.js';
 
 /**
  * Set of rules controlling keyboard navigation from an icon.
@@ -40,21 +41,7 @@ export class IconNavigationPolicy implements INavigationPolicy<Icon> {
    * @returns The next icon, field or input following this icon, if any.
    */
   getNextSibling(current: Icon): IFocusableNode | null {
-    const block = current.getSourceBlock() as BlockSvg;
+    return navigateBlock(current, 1);
-    const icons = block.getIcons();
-    const currentIndex = icons.indexOf(current);
-    if (currentIndex >= 0 && currentIndex + 1 < icons.length) {
-      return icons[currentIndex + 1];
-    }
-
-    for (const input of block.inputList) {
-      if (input.fieldRow.length) return input.fieldRow[0];
-
-      if (input.connection?.targetBlock())
-        return input.connection.targetBlock() as BlockSvg;
-    }
-
-    return null;
   }
 
   /**
@@ -64,14 +51,7 @@ export class IconNavigationPolicy implements INavigationPolicy<Icon> {
    * @returns The icon's previous icon, if any.
    */
   getPreviousSibling(current: Icon): IFocusableNode | null {
-    const block = current.getSourceBlock() as BlockSvg;
+    return navigateBlock(current, -1);
-    const icons = block.getIcons();
-    const currentIndex = icons.indexOf(current);
-    if (currentIndex >= 1) {
-      return icons[currentIndex - 1];
-    }
-
-    return null;
   }
 
   /**

tests/mocha/cursor_test.js

@@ -246,7 +246,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);
       });
     });
     suite('one c-hat block', function () {
@@ -340,7 +340,7 @@ suite('Cursor', function () {
       test('getLastNode', function () {
         const node = this.cursor.getLastNode();
         const blockB = this.workspace.getBlockById('B');
-        assert.equal(node, blockB);
+        assert.equal(node, blockB.inputList[0].connection);
       });
     });
 

tests/mocha/navigation_test.js

@@ -72,6 +72,20 @@ suite('Navigation', function () {
         'tooltip': '',
         'helpUrl': '',
       },
+      {
+        'type': 'double_value_input',
+        'message0': '%1 %2',
+        'args0': [
+          {
+            'type': 'input_value',
+            'name': 'NAME1',
+          },
+          {
+            'type': 'input_value',
+            'name': 'NAME2',
+          },
+        ],
+      },
     ]);
     this.workspace = Blockly.inject('blocklyDiv', {});
     this.navigator = this.workspace.getNavigator();
@@ -80,6 +94,7 @@ suite('Navigation', function () {
     const statementInput3 = this.workspace.newBlock('input_statement');
     const statementInput4 = this.workspace.newBlock('input_statement');
     const fieldWithOutput = this.workspace.newBlock('field_input');
+    const doubleValueInput = this.workspace.newBlock('double_value_input');
     const valueInput = this.workspace.newBlock('value_input');
 
     statementInput1.nextConnection.connect(statementInput2.previousConnection);
@@ -97,6 +112,7 @@ suite('Navigation', function () {
       statementInput4: statementInput4,
       fieldWithOutput: fieldWithOutput,
       valueInput: valueInput,
+      doubleValueInput,
     };
   });
   teardown(function () {
@@ -431,16 +447,9 @@ suite('Navigation', function () {
         assert.equal(nextNode, prevConnection);
       });
       test('fromInputToInput', function () {
-        const input = this.blocks.statementInput1.inputList[0];
+        const input = this.blocks.doubleValueInput.inputList[0];
         const inputConnection =
-          this.blocks.statementInput1.inputList[1].connection;
+          this.blocks.doubleValueInput.inputList[1].connection;
-        const nextNode = this.navigator.getNextSibling(input.connection);
-        assert.equal(nextNode, inputConnection);
-      });
-      test('fromInputToStatementInput', function () {
-        const input = this.blocks.fieldAndInputs2.inputList[1];
-        const inputConnection =
-          this.blocks.fieldAndInputs2.inputList[2].connection;
         const nextNode = this.navigator.getNextSibling(input.connection);
         assert.equal(nextNode, inputConnection);
       });
@@ -575,6 +584,11 @@ suite('Navigation', function () {
         assert.equal(prevNode, this.blocks.statementInput1);
       });
       test('fromInputToField', function () {
+        // Disconnect the block that was connected to the input we're testing,
+        // because we only navigate to/from empty input connections (if they're
+        // connected navigation targets the connected block, bypassing the
+        // connection).
+        this.blocks.fieldWithOutput.outputConnection.disconnect();
         const input = this.blocks.statementInput1.inputList[0];
         const prevNode = this.navigator.getPreviousSibling(input.connection);
         assert.equal(prevNode, input.fieldRow[1]);
@@ -585,9 +599,9 @@ suite('Navigation', function () {
         assert.isNull(prevNode);
       });
       test('fromInputToInput', function () {
-        const input = this.blocks.fieldAndInputs2.inputList[2];
+        const input = this.blocks.doubleValueInput.inputList[1];
         const inputConnection =
-          this.blocks.fieldAndInputs2.inputList[1].connection;
+          this.blocks.doubleValueInput.inputList[0].connection;
         const prevNode = this.navigator.getPreviousSibling(input.connection);
         assert.equal(prevNode, inputConnection);
       });
@@ -711,10 +725,10 @@ suite('Navigation', function () {
         const inNode = this.navigator.getFirstChild(input.connection);
         assert.equal(inNode, previousConnection);
       });
-      test('fromBlockToField', function () {
+      test('fromBlockToInput', function () {
-        const field = this.blocks.valueInput.getField('NAME');
+        const connection = this.blocks.valueInput.inputList[0].connection;
         const inNode = this.navigator.getFirstChild(this.blocks.valueInput);
-        assert.equal(inNode, field);
+        assert.equal(inNode, connection);
       });
       test('fromBlockToField', function () {
         const inNode = this.navigator.getFirstChild(
@@ -731,7 +745,10 @@ suite('Navigation', function () {
         const inNode = this.navigator.getFirstChild(
           this.blocks.dummyInputValue,
         );
-        assert.equal(inNode, null);
+        assert.equal(
+          inNode,
+          this.blocks.dummyInputValue.inputList[1].connection,
+        );
       });
       test('fromOuputToNull', function () {
         const output = this.blocks.fieldWithOutput.outputConnection;
@@ -787,13 +804,10 @@ suite('Navigation', function () {
         const outNode = this.navigator.getParent(input.connection);
         assert.equal(outNode, this.blocks.statementInput1);
       });
-      test('fromOutputToInput', function () {
+      test('fromOutputToBlock', function () {
         const output = this.blocks.fieldWithOutput.outputConnection;
         const outNode = this.navigator.getParent(output);
-        assert.equal(
+        assert.equal(outNode, this.blocks.fieldWithOutput);
-          outNode,
-          this.blocks.statementInput1.inputList[0].connection,
-        );
       });
       test('fromOutputToBlock', function () {
         const output = this.blocks.fieldWithOutput2.outputConnection;
@@ -805,43 +819,29 @@ suite('Navigation', function () {
         const outNode = this.navigator.getParent(field);
         assert.equal(outNode, this.blocks.statementInput1);
       });
-      test('fromPreviousToInput', function () {
-        const previous = this.blocks.statementInput3.previousConnection;
-        const inputConnection =
-          this.blocks.statementInput2.inputList[1].connection;
-        const outNode = this.navigator.getParent(previous);
-        assert.equal(outNode, inputConnection);
-      });
       test('fromPreviousToBlock', function () {
         const previous = this.blocks.statementInput2.previousConnection;
         const outNode = this.navigator.getParent(previous);
-        assert.equal(outNode, this.blocks.statementInput1);
+        assert.equal(outNode, this.blocks.statementInput2);
-      });
-      test('fromNextToInput', function () {
-        const next = this.blocks.statementInput3.nextConnection;
-        const inputConnection =
-          this.blocks.statementInput2.inputList[1].connection;
-        const outNode = this.navigator.getParent(next);
-        assert.equal(outNode, inputConnection);
       });
       test('fromNextToBlock', function () {
         const next = this.blocks.statementInput2.nextConnection;
         const outNode = this.navigator.getParent(next);
-        assert.equal(outNode, this.blocks.statementInput1);
+        assert.equal(outNode, this.blocks.statementInput2);
       });
       test('fromNextToBlock_NoPreviousConnection', function () {
         const next = this.blocks.secondBlock.nextConnection;
         const outNode = this.navigator.getParent(next);
-        assert.equal(outNode, this.blocks.noPrevConnection);
+        assert.equal(outNode, this.blocks.secondBlock);
       });
       /**
        * This is where there is a block with both an output connection and a
        * next connection attached to an input.
        */
-      test('fromNextToInput_OutputAndPreviousConnection', function () {
+      test('fromNextToBlock_OutputAndPreviousConnection', function () {
         const next = this.blocks.outputNextBlock.nextConnection;
         const outNode = this.navigator.getParent(next);
-        assert.equal(outNode, this.blocks.secondBlock.inputList[0].connection);
+        assert.equal(outNode, this.blocks.outputNextBlock);
       });
       test('fromBlockToWorkspace', function () {
         const outNode = this.navigator.getParent(this.blocks.statementInput2);