fix: insert then cancel with workspace selected (#626)

* test: insert then cancel tests

The workspace selection case is broken

* fix: insert then cancel with workspace selected

Fixes #623

Author: microbit-matt-hillsdon

src/actions/enter.ts

@@ -24,7 +24,7 @@ import type {Block} from 'blockly/core';
 
 import * as Constants from '../constants';
 import type {Navigation} from '../navigation';
-import {Mover} from './mover';
+import {Mover, MoveType} from './mover';
 import {
   showConstrainedMovementHint,
   showHelpHint,
@@ -210,13 +210,19 @@ export class EnterAction {
     const insertStartPoint = stationaryNode
       ? this.navigation.findInsertStartPoint(stationaryNode, newBlock)
       : null;
+
     if (workspace.getTopBlocks().includes(newBlock)) {
       this.positionNewTopLevelBlock(workspace, newBlock);
     }
 
     workspace.setResizesEnabled(true);
 
-    this.mover.startMove(workspace, newBlock, insertStartPoint);
+    this.mover.startMove(
+      workspace,
+      newBlock,
+      MoveType.Insert,
+      insertStartPoint,
+    );
 
     const isStartBlock =
       !newBlock.outputConnection &&

src/actions/move.ts

@@ -19,7 +19,7 @@ import {
   ISelectable,
 } from 'blockly';
 import {Direction} from '../drag_direction';
-import {Mover} from './mover';
+import {Mover, MoveType} from './mover';
 import {getMenuItem} from '../shortcut_formatting';
 
 const KeyCodes = utils.KeyCodes;
@@ -57,7 +57,7 @@ export class MoveActions {
           }
           return (
             !!startDraggable &&
-            this.mover.startMove(workspace, startDraggable, null)
+            this.mover.startMove(workspace, startDraggable, MoveType.Move, null)
           );
         },
         keyCodes: [KeyCodes.M],
@@ -186,7 +186,7 @@ export class MoveActions {
           }
           return (
             !!startDraggable &&
-            this.mover.startMove(workspace, startDraggable, null)
+            this.mover.startMove(workspace, startDraggable, MoveType.Move, null)
           );
         },
         scopeType: ContextMenuRegistry.ScopeType.BLOCK,
@@ -209,7 +209,7 @@ export class MoveActions {
         callback: (scope) => {
           const comment = scope.comment;
           if (!comment) return false;
-          this.mover.startMove(comment.workspace, comment, null);
+          this.mover.startMove(comment.workspace, comment, MoveType.Move, null);
         },
         scopeType: ContextMenuRegistry.ScopeType.COMMENT,
         id: 'move_comment',

src/actions/mover.ts

@@ -47,6 +47,20 @@ const CONSTRAINED_ADDITIONAL_PADDING = 70;
  */
 const COMMIT_MOVE_SHORTCUT = 'commitMove';
 
+/**
+ * Whether this is an insert or a move.
+ */
+export enum MoveType {
+  /**
+   * An insert will remove the block if the move is aborted.
+   */
+  Insert,
+  /**
+   * A regular move of a pre-existing block.
+   */
+  Move,
+}
+
 /**
  * Low-level code for moving elements with keyboard shortcuts.
  */
@@ -108,17 +122,19 @@ export class Mover {
    *
    * @param workspace The workspace we might be moving on.
    * @param draggable The element to start dragging.
-   * @param insertStartPoint Where to insert the element, or null if the element
-   *     already existed.
+   * @param moveType Whether this is an insert or a move.
+   * @param startPoint Where to start the move, or null to use the current
+   *     location if any.
    * @returns True iff a move has successfully begun.
    */
   startMove(
     workspace: WorkspaceSvg,
     draggable: IDraggable & IFocusableNode & IBoundedElement & ISelectable,
-    insertStartPoint: RenderedConnection | null,
+    moveType: MoveType,
+    startPoint: RenderedConnection | null,
   ) {
     if (draggable instanceof BlockSvg) {
-      this.patchDragStrategy(draggable, insertStartPoint);
+      this.patchDragStrategy(draggable, moveType, startPoint);
     } else if (draggable instanceof comments.RenderedWorkspaceComment) {
       this.moveIndicator = new MoveIndicatorBubble(draggable);
     }
@@ -225,10 +241,7 @@ export class Mover {
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     const dragStrategy = (info.draggable as any)
       .dragStrategy as KeyboardDragStrategy;
-    this.patchDragger(
-      info.dragger as dragging.Dragger,
-      dragStrategy.isNewBlock,
-    );
+    this.patchDragger(info.dragger as dragging.Dragger, dragStrategy.moveType);
 
     // Save the position so we can put the cursor in a reasonable spot.
     // @ts-expect-error Access to private property connectionCandidate.
@@ -243,7 +256,7 @@ export class Mover {
       info.startLocation,
     );
 
-    if (dragStrategy.isNewBlock && target) {
+    if (dragStrategy.moveType === MoveType.Insert && target) {
       workspace.getCursor()?.setCurNode(target);
     }
 
@@ -348,16 +361,20 @@ export class Mover {
    * Monkeypatch: replace the block's drag strategy and cache the old value.
    *
    * @param block The block to patch.
-   * @param insertStartPoint Where to insert the block, or null if the block
-   *     already existed.
+   * @param moveType Whether this is an insert or a move.
+   * @param startPoint Where to start the move, or null to use the current
+   *     location if any.
    */
   private patchDragStrategy(
     block: BlockSvg,
-    insertStartPoint: RenderedConnection | null,
+    moveType: MoveType,
+    startPoint: RenderedConnection | null,
   ) {
     // @ts-expect-error block.dragStrategy is private.
     this.oldDragStrategy = block.dragStrategy;
-    block.setDragStrategy(new KeyboardDragStrategy(block, insertStartPoint));
+    block.setDragStrategy(
+      new KeyboardDragStrategy(block, moveType, startPoint),
+    );
   }
 
   /**
@@ -401,10 +418,10 @@ export class Mover {
    * an existing element.
    *
    * @param dragger The dragger to patch.
-   * @param isNewBlock Whether a moving block was created for this action.
+   * @param moveType Whether this is an insert or a move.
    */
-  private patchDragger(dragger: dragging.Dragger, isNewBlock: boolean) {
-    if (isNewBlock) {
+  private patchDragger(dragger: dragging.Dragger, moveType: MoveType) {
+    if (moveType === MoveType.Insert) {
       // Monkey patch dragger to trigger delete.
       // eslint-disable-next-line @typescript-eslint/no-explicit-any
       (dragger as any).wouldDeleteDraggable = () => true;

src/keyboard_drag_strategy.ts

@@ -14,6 +14,7 @@ import {
 import {Direction, getDirectionFromXY} from './drag_direction';
 import {showUnconstrainedMoveHint} from './hints';
 import {MoveIcon} from './move_icon';
+import {MoveType} from './actions/mover';
 
 // Copied in from core because it is not exported.
 interface ConnectionCandidate {
@@ -35,14 +36,12 @@ export class KeyboardDragStrategy extends dragging.BlockDragStrategy {
   /** Where a constrained movement should start when traversing the tree. */
   private searchNode: RenderedConnection | null = null;
 
-  isNewBlock: boolean;
-
   constructor(
     private block: BlockSvg,
-    private insertStartPoint: RenderedConnection | null,
+    public moveType: MoveType,
+    private startPoint: RenderedConnection | null,
   ) {
     super(block);
-    this.isNewBlock = !!this.insertStartPoint;
   }
 
   override startDrag(e?: PointerEvent) {
@@ -300,7 +299,7 @@ export class KeyboardDragStrategy extends dragging.BlockDragStrategy {
    */
   private createInitialCandidate(): ConnectionCandidate | null {
     // @ts-expect-error startParentConn is private.
-    const neighbour = this.insertStartPoint ?? this.startParentConn;
+    const neighbour = this.startPoint ?? this.startParentConn;
     if (neighbour) {
       this.searchNode = neighbour;
       switch (neighbour.type) {

test/webdriverio/test/insert_test.ts

@@ -15,6 +15,8 @@ import {
   testFileLocations,
   testSetup,
   keyRight,
+  getCurrentFocusedBlockId,
+  blockIsPresent,
   keyUp,
   tabNavigateToToolbox,
 } from './test_setup.js';
@@ -29,6 +31,42 @@ suite('Insert test', function () {
     await this.browser.pause(PAUSE_TIME);
   });
 
+  test('Insert and cancel with block selection', async function () {
+    // Navigate to draw_circle_1.
+    await tabNavigateToWorkspace(this.browser);
+    await focusOnBlock(this.browser, 'draw_circle_1');
+    // Insert 'if' block
+    await this.browser.keys('t');
+    await keyRight(this.browser);
+    await this.browser.keys(Key.Enter);
+    chai.assert.equal('controls_if', await getFocusedBlockType(this.browser));
+    const ifId = await getCurrentFocusedBlockId(this.browser);
+    chai.assert.ok(ifId);
+
+    // Cancel
+    await this.browser.keys(Key.Escape);
+
+    chai.assert.isFalse(await blockIsPresent(this.browser, ifId));
+  });
+
+  test('Insert and cancel with workspace selection', async function () {
+    // Navigate to workspace.
+    await tabNavigateToWorkspace(this.browser);
+    await this.browser.keys('w');
+    // Insert 'if' block
+    await this.browser.keys('t');
+    await keyRight(this.browser);
+    await this.browser.keys(Key.Enter);
+    chai.assert.equal('controls_if', await getFocusedBlockType(this.browser));
+    const ifId = await getCurrentFocusedBlockId(this.browser);
+    chai.assert.ok(ifId);
+
+    // Cancel
+    await this.browser.keys(Key.Escape);
+
+    chai.assert.isFalse(await blockIsPresent(this.browser, ifId));
+  });
+
   test('Insert C-shaped block with statement block selected', async function () {
     // Navigate to draw_circle_1.
     await tabNavigateToWorkspace(this.browser);