fix: Improve navigation and movement looping behavior

packages/blockly/core/dragging/block_drag_strategy.ts

@@ -117,7 +117,7 @@ export class BlockDragStrategy implements IDragStrategy {
       newBlock.workspace,
       screenCoordinate,
     );
-    newBlock.moveTo(workspaceCoordinates);
+    newBlock.moveDuringDrag(workspaceCoordinates);
   }
 
   /**
@@ -455,8 +455,11 @@ export class BlockDragStrategy implements IDragStrategy {
       // No connection was available or adequately close to the dragged block;
       // suggest using unconstrained mode to arbitrarily position the block if
       // we're in keyboard-driven constrained mode.
-      if (this.moveMode === MoveMode.CONSTRAINED) {
-        showUnconstrainedMoveHint(this.workspace, true);
+      if (
+        this.moveMode === MoveMode.CONSTRAINED &&
+        !this.allConnectionPairs.length
+      ) {
+        showUnconstrainedMoveHint(this.workspace);
         this.workspace.getAudioManager().playErrorBeep();
       }
     }
@@ -479,6 +482,41 @@ export class BlockDragStrategy implements IDragStrategy {
     const newCandidate = this.getConnectionCandidate(delta);
 
     if (!newCandidate) {
+      // Position above or below the first/last block.
+      const connectedBlock = currCandidate?.neighbour.getSourceBlock();
+      let root = connectedBlock?.getRootBlock() ?? connectedBlock;
+      if (root === draggingBlock) root = connectedBlock;
+      const direction = this.getDirectionToNewLocation(
+        Coordinate.sum(this.startLoc!, delta),
+      );
+      const bounds = root?.getBoundingRectangle();
+      if (!bounds) return;
+
+      let destination: Coordinate;
+      switch (direction) {
+        case Direction.LEFT:
+        case Direction.UP:
+          destination = new Coordinate(
+            bounds.getOrigin().x,
+            bounds.getOrigin().y -
+              this.BLOCK_CONNECTION_OFFSET * 2 -
+              draggingBlock.getHeightWidth().height,
+          );
+          break;
+        case Direction.RIGHT:
+        case Direction.DOWN:
+        default:
+          destination = new Coordinate(
+            bounds.getOrigin().x,
+            bounds.getOrigin().y +
+              bounds.getHeight() +
+              this.BLOCK_CONNECTION_OFFSET * 2,
+          );
+          break;
+      }
+
+      draggingBlock.moveDuringDrag(destination);
+
       this.connectionPreviewer?.hidePreview();
       this.connectionCandidate = null;
       return;
@@ -570,15 +608,23 @@ export class BlockDragStrategy implements IDragStrategy {
     delta: Coordinate,
   ): ConnectionCandidate | null {
     if (this.moveMode === MoveMode.CONSTRAINED) {
-      const direction = this.getDirectionToNewLocation(
-        Coordinate.sum(this.startLoc!, delta),
-      );
-      return this.findTraversalCandidate(direction);
+      return this.findTraversalCandidate(delta);
     }
 
     // If we do not have a candidate yet, we fallback to the closest one nearby.
+    return this.getClosestCandidate(this.block, delta);
+  }
+
+  /**
+   * Returns the closest connection candidate for the given block.
+   *
+   * @param block The block to find a connection for.
+   * @param delta The distance the block has traveled since dragging began.
+   * @returns The closest available connection candidate, if any.
+   */
+  private getClosestCandidate(block: BlockSvg, delta: Coordinate) {
     let radius = this.getSearchRadius();
-    const localConns = this.getLocalConnections(this.block);
+    const localConns = this.getLocalConnections(block);
     let candidate: ConnectionCandidate | null = null;
 
     for (const conn of localConns) {
@@ -743,7 +789,7 @@ export class BlockDragStrategy implements IDragStrategy {
           }
       }
     } else {
-      this.block.moveTo(this.startLoc!, ['drag']);
+      this.block.moveDuringDrag(this.startLoc!);
       this.workspace
         .getLayerManager()
         ?.moveOffDragLayer(this.block, layers.BLOCK);
@@ -772,10 +818,13 @@ export class BlockDragStrategy implements IDragStrategy {
   /**
    * Get the nearest valid candidate connection in traversal order.
    *
-   * @param direction The cardinal direction in which the block is being moved.
+   * @param delta The distance the block has moved since this drag began.
    * @returns A candidate connection and radius, or null if none was found.
    */
-  findTraversalCandidate(direction: Direction): ConnectionCandidate | null {
+  findTraversalCandidate(delta: Coordinate): ConnectionCandidate | null {
+    const direction = this.getDirectionToNewLocation(
+      Coordinate.sum(this.startLoc!, delta),
+    );
     const pairs = this.allConnectionPairs;
     if (direction === Direction.NONE || !pairs.length) {
       return this.connectionCandidate;
@@ -788,17 +837,30 @@ export class BlockDragStrategy implements IDragStrategy {
         this.connectionCandidate?.neighbour === pair.neighbour,
     );
 
+    const navigator = this.block.workspace.getNavigator();
     if (forwardTraversal) {
       if (currentPairIndex === -1) {
-        return this.pairToCandidate(pairs[0]);
+        const terminal = this.isInTerminalPosition(this.block, Direction.DOWN);
+        if (navigator.getNavigationLoops()) {
+          return this.pairToCandidate(pairs[0]);
+        } else if (!terminal) {
+          return this.getClosestCandidate(this.block, delta);
+        }
+        return null;
       } else if (currentPairIndex === pairs.length - 1) {
         return null;
       } else {
         return this.pairToCandidate(pairs[currentPairIndex + 1]);
       }
     } else {
       if (currentPairIndex === -1) {
-        return this.pairToCandidate(pairs[pairs.length - 1]);
+        const terminal = this.isInTerminalPosition(this.block, Direction.UP);
+        if (navigator.getNavigationLoops()) {
+          return this.pairToCandidate(pairs[pairs.length - 1]);
+        } else if (!terminal) {
+          return this.getClosestCandidate(this.block, delta);
+        }
+        return null;
       } else if (currentPairIndex === 0) {
         return null;
       } else {
@@ -807,9 +869,63 @@ export class BlockDragStrategy implements IDragStrategy {
     }
   }
 
+  /**
+   * Returns whether or not the given block is at a terminal position (start or
+   * end) of the blocks on the workspace. This helps distinguish between a block
+   * that is at the end of the line because all valid connections have been
+   * visited and the proposed constrained move destination is now to drop it on
+   * the workspace as a top-level block (in which case it will be in a terminal
+   * position), and a block that just entered move mode as a top-level block,
+   * and should therefore still be able to move to another connection point
+   * even if looping is disabled.
+   *
+   * @param block The block to check.
+   * @param direction The current dragging direction.
+   * @returns True if the block is at the start or end of its possible positions
+   *     on the workspace.
+   */
+  private isInTerminalPosition(
+    block: BlockSvg,
+    direction: Direction.UP | Direction.DOWN,
+  ) {
+    if (block.getParent()) {
+      return false;
+    }
+
+    const topBlocks = block.workspace.getTopBlocks(true);
+
+    const index = topBlocks.indexOf(block);
+    const delta = direction === Direction.UP ? -1 : 1;
+    const start = index + delta;
+
+    // Generally terminal blocks will be at the start or end of the sorted list
+    // of top blocks, but it still counts if all of the blocks before/after it
+    // have no valid connection points for the block in question.
+    const blockConnections = block.getConnections_(false);
+    for (let i = start; i >= 0 && i < topBlocks.length; i += delta) {
+      const topBlock = topBlocks[i];
+      for (const a of blockConnections) {
+        for (const b of topBlock.getConnections_(false)) {
+          if (
+            block.workspace.connectionChecker.canConnect(a, b, true, Infinity)
+          ) {
+            return false;
+          }
+        }
+      }
+    }
+
+    return true;
+  }
+
+  /**
+   * Converts a connection pair to a connection candidate with a default
+   * distance of 0.
+   */
   private pairToCandidate(pair: ConnectionPair): ConnectionCandidate {
     return {...pair, distance: 0};
   }
+
   /**
    * Returns the cardinal direction that the block being dragged would have to
    * move in to reach the given location.

packages/blockly/core/keyboard_nav/navigators/navigator.ts

@@ -56,7 +56,7 @@ export class Navigator {
   ];
 
   /** Whether or not navigation loops around when reaching the end. */
-  protected navigationLoops = false;
+  protected navigationLoops = true;
 
   /**
    * Adds a navigation ruleset to this Navigator.

packages/blockly/tests/mocha/keyboard_navigation_test.js

@@ -561,6 +561,7 @@ suite('Toolbox and flyout arrow navigation by layout', function () {
       });
 
       test('Previous toolbox item from first is no-op', function () {
+        this.workspace.getToolbox().getNavigator().setNavigationLoops(false);
         Blockly.getFocusManager().focusNode(this.firstToolboxItem);
         pressKey(this.workspace, this.keys.previousItem);
         assert.equal(
@@ -569,6 +570,16 @@ suite('Toolbox and flyout arrow navigation by layout', function () {
         );
       });
 
+      test('Previous toolbox item from first loops to last', function () {
+        this.workspace.getToolbox().getNavigator().setNavigationLoops(true);
+        Blockly.getFocusManager().focusNode(this.firstToolboxItem);
+        pressKey(this.workspace, this.keys.previousItem);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.lastToolboxItem,
+        );
+      });
+
       test('Previous toolbox item', function () {
         Blockly.getFocusManager().focusNode(this.lastToolboxItem);
         pressKey(this.workspace, this.keys.previousItem);
@@ -579,6 +590,7 @@ suite('Toolbox and flyout arrow navigation by layout', function () {
       });
 
       test('Next toolbox item from last is no-op', function () {
+        this.workspace.getToolbox().getNavigator().setNavigationLoops(false);
         Blockly.getFocusManager().focusNode(this.lastToolboxItem);
         pressKey(this.workspace, this.keys.nextItem);
         assert.equal(
@@ -587,6 +599,16 @@ suite('Toolbox and flyout arrow navigation by layout', function () {
         );
       });
 
+      test('Next toolbox item from last loops', function () {
+        this.workspace.getToolbox().getNavigator().setNavigationLoops(true);
+        Blockly.getFocusManager().focusNode(this.lastToolboxItem);
+        pressKey(this.workspace, this.keys.nextItem);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.firstToolboxItem,
+        );
+      });
+
       test('Next toolbox item', function () {
         Blockly.getFocusManager().focusNode(this.firstToolboxItem);
         pressKey(this.workspace, this.keys.nextItem);
@@ -615,6 +637,11 @@ suite('Toolbox and flyout arrow navigation by layout', function () {
       });
 
       test('Previous flyout item from first is no-op', function () {
+        this.workspace
+          .getFlyout()
+          .getWorkspace()
+          .getNavigator()
+          .setNavigationLoops(false);
         pressKey(this.workspace, Blockly.utils.KeyCodes.T);
         Blockly.getFocusManager().focusNode(
           this.workspace.getFlyout().getWorkspace().getTopBlocks()[0],
@@ -626,6 +653,23 @@ suite('Toolbox and flyout arrow navigation by layout', function () {
         );
       });
 
+      test('Previous flyout item from first loops', function () {
+        this.workspace
+          .getFlyout()
+          .getWorkspace()
+          .getNavigator()
+          .setNavigationLoops(true);
+        pressKey(this.workspace, Blockly.utils.KeyCodes.T);
+        Blockly.getFocusManager().focusNode(
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[0],
+        );
+        pressKey(this.workspace, this.keys.previousItem);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[1],
+        );
+      });
+
       test('Previous flyout item', function () {
         pressKey(this.workspace, Blockly.utils.KeyCodes.T);
         Blockly.getFocusManager().focusNode(
@@ -639,6 +683,11 @@ suite('Toolbox and flyout arrow navigation by layout', function () {
       });
 
       test('Next flyout item from last is no-op', function () {
+        this.workspace
+          .getFlyout()
+          .getWorkspace()
+          .getNavigator()
+          .setNavigationLoops(false);
         pressKey(this.workspace, Blockly.utils.KeyCodes.T);
         Blockly.getFocusManager().focusNode(
           this.workspace.getFlyout().getWorkspace().getTopBlocks()[1],
@@ -650,6 +699,23 @@ suite('Toolbox and flyout arrow navigation by layout', function () {
         );
       });
 
+      test('Next flyout item from last loops', function () {
+        this.workspace
+          .getFlyout()
+          .getWorkspace()
+          .getNavigator()
+          .setNavigationLoops(true);
+        pressKey(this.workspace, Blockly.utils.KeyCodes.T);
+        Blockly.getFocusManager().focusNode(
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[1],
+        );
+        pressKey(this.workspace, this.keys.nextItem);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[0],
+        );
+      });
+
       test('Next flyout item', function () {
         pressKey(this.workspace, Blockly.utils.KeyCodes.T);
         Blockly.getFocusManager().focusNode(