fix: Improve navigation and movement looping behavior (#9732)

* fix: Default navigation looping to on

* fix: Don't show the unconstrained move hint every time a block is moved to the workspace

* fix: Normalize block movement during drags

* fix: Offset proposed top-level blocks during constrained drags

* fix: Make constrained moves respect the navigator's looping setting

* fix: Fix tests

* chore: Fix docstring

* fix: Show unconstrained move hint only when there are no available connections

* refactor: Use constants

packages/blockly/core/dragging/block_drag_strategy.ts

@@ -120,7 +120,7 @@ export class BlockDragStrategy implements IDragStrategy {
       newBlock.workspace,
       screenCoordinate,
     );
-    newBlock.moveTo(workspaceCoordinates);
+    newBlock.moveDuringDrag(workspaceCoordinates);
   }
 
   /**
@@ -511,8 +511,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();
       }
     }
@@ -536,6 +539,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;
@@ -627,15 +665,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) {
@@ -800,7 +846,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);
@@ -830,10 +876,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;
@@ -846,9 +895,16 @@ 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 {
@@ -856,7 +912,13 @@ export class BlockDragStrategy implements IDragStrategy {
       }
     } 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 {
@@ -865,9 +927,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(

packages/blockly/tests/mocha/shortcut_items_test.js

@@ -39,8 +39,10 @@ suite('Keyboard Shortcut Items', function () {
    */
   function setSelectedBlock(workspace) {
     const block = workspace.newBlock('stack_block');
+    block.initSvg();
+    block.render();
     Blockly.common.setSelected(block);
-    sinon.stub(Blockly.getFocusManager(), 'getFocusedNode').returns(block);
+    Blockly.getFocusManager().focusNode(block);
     return block;
   }
 
@@ -146,9 +148,6 @@ suite('Keyboard Shortcut Items', function () {
     });
     // Do not delete anything if a connection is focused.
     test('Not called when connection is focused', function () {
-      // Restore the stub behavior called during setup
-      Blockly.getFocusManager().getFocusedNode.restore();
-
       setSelectedConnection(this.workspace);
       const event = createKeyDownEvent(Blockly.utils.KeyCodes.DELETE);
       this.injectionDiv.dispatchEvent(event);
@@ -203,9 +202,6 @@ suite('Keyboard Shortcut Items', function () {
       sinon.assert.notCalled(this.hideChaffSpy);
     });
     test('Not called when connection is focused', function () {
-      // Restore the stub behavior called during setup
-      Blockly.getFocusManager().getFocusedNode.restore();
-
       setSelectedConnection(this.workspace);
       const event = createKeyDownEvent(Blockly.utils.KeyCodes.C, [
         Blockly.utils.KeyCodes.CTRL,
@@ -216,7 +212,6 @@ suite('Keyboard Shortcut Items', function () {
     });
     // Copy a comment.
     test('Workspace comment', function () {
-      Blockly.getFocusManager().getFocusedNode.restore();
       this.comment = setSelectedComment(this.workspace);
       this.copySpy = sinon.spy(this.comment, 'toCopyData');
 
@@ -279,9 +274,6 @@ suite('Keyboard Shortcut Items', function () {
       sinon.assert.notCalled(this.hideChaffSpy);
     });
     test('Not called when connection is focused', function () {
-      // Restore the stub behavior called during setup
-      Blockly.getFocusManager().getFocusedNode.restore();
-
       setSelectedConnection(this.workspace);
       const event = createKeyDownEvent(Blockly.utils.KeyCodes.C, [
         Blockly.utils.KeyCodes.CTRL,
@@ -294,7 +286,6 @@ suite('Keyboard Shortcut Items', function () {
 
     // Cut a comment.
     test('Workspace comment', function () {
-      Blockly.getFocusManager().getFocusedNode.restore();
       this.comment = setSelectedComment(this.workspace);
       this.copySpy = sinon.spy(this.comment, 'toCopyData');
       this.disposeSpy = sinon.spy(this.comment, 'dispose');
@@ -435,7 +426,7 @@ suite('Keyboard Shortcut Items', function () {
           contextMenuKeyEvent,
         );
       for (const option of menuOptions) {
-        assert.include(menu.getElement().innerText, option.text);
+        assert.include(menu.getElement().textContent, option.text);
       }
     });
 
@@ -451,7 +442,7 @@ suite('Keyboard Shortcut Items', function () {
           contextMenuKeyEvent,
         );
       for (const option of menuOptions) {
-        assert.include(menu.getElement().innerText, option.text);
+        assert.include(menu.getElement().textContent, option.text);
       }
     });
 
@@ -468,7 +459,7 @@ suite('Keyboard Shortcut Items', function () {
           contextMenuKeyEvent,
         );
       for (const option of menuOptions) {
-        assert.include(menu.getElement().innerText, option.text);
+        assert.include(menu.getElement().textContent, option.text);
       }
     });
 
@@ -888,6 +879,7 @@ suite('Keyboard Shortcut Items', function () {
     });
 
     test('First stack navigating back is a no-op', function () {
+      this.workspace.getNavigator().setNavigationLoops(false);
       Blockly.getFocusManager().focusNode(this.block1);
       this.injectionDiv.dispatchEvent(keyPrevStack());
       assert.strictEqual(
@@ -896,7 +888,18 @@ suite('Keyboard Shortcut Items', function () {
       );
     });
 
+    test('First stack navigating back loops', function () {
+      this.workspace.getNavigator().setNavigationLoops(true);
+      Blockly.getFocusManager().focusNode(this.block1);
+      this.injectionDiv.dispatchEvent(keyPrevStack());
+      assert.strictEqual(
+        Blockly.getFocusManager().getFocusedNode(),
+        this.block3,
+      );
+    });
+
     test('Last stack navigating forward is a no-op', function () {
+      this.workspace.getNavigator().setNavigationLoops(false);
       Blockly.getFocusManager().focusNode(this.block3);
       this.injectionDiv.dispatchEvent(keyNextStack());
       assert.strictEqual(
@@ -905,6 +908,16 @@ suite('Keyboard Shortcut Items', function () {
       );
     });
 
+    test('Last stack navigating forward loops', function () {
+      this.workspace.getNavigator().setNavigationLoops(true);
+      Blockly.getFocusManager().focusNode(this.block3);
+      this.injectionDiv.dispatchEvent(keyNextStack());
+      assert.strictEqual(
+        Blockly.getFocusManager().getFocusedNode(),
+        this.block1,
+      );
+    });
+
     test('Block forward to block', function () {
       Blockly.getFocusManager().focusNode(this.block1);
       this.injectionDiv.dispatchEvent(keyNextStack());
@@ -1096,9 +1109,12 @@ suite('Keyboard Shortcut Items', function () {
         .getFirstChild(this.workspace.getFlyout().getWorkspace());
       assert.instanceOf(block, Blockly.BlockSvg);
       Blockly.getFocusManager().focusNode(block);
+      first.moveTo(new Blockly.utils.Coordinate(500, 500));
 
       const event = createKeyDownEvent(Blockly.utils.KeyCodes.ENTER);
       this.workspace.getInjectionDiv().dispatchEvent(event);
+      const event2 = createKeyDownEvent(Blockly.utils.KeyCodes.UP);
+      this.workspace.getInjectionDiv().dispatchEvent(event2);
 
       const movingBlock = Blockly.getFocusManager().getFocusedNode();
       assert.notEqual(block, movingBlock);

packages/blockly/tests/mocha/toolbox_test.js

@@ -301,6 +301,7 @@ suite('Toolbox', function () {
     });
 
     test('Down arrow on last item should be a no-op', function () {
+      this.toolbox.getNavigator().setNavigationLoops(false);
       const items = this.toolbox.getToolboxItems();
       Blockly.getFocusManager().focusNode(items[6]);
       const oldIndex = items.indexOf(this.toolbox.getSelectedItem());
@@ -367,6 +368,7 @@ suite('Toolbox', function () {
     });
 
     test('Up arrow on first item should be a no-op', function () {
+      this.toolbox.getNavigator().setNavigationLoops(false);
       const items = this.toolbox.getToolboxItems();
       Blockly.getFocusManager().focusNode(items[0]);
       const oldIndex = items.indexOf(this.toolbox.getSelectedItem());