fix: take care making a selection when focusing the workspace

This can interfere with in-progress gestures causing them to act in unexpected
ways. We now clear the current node when focus moves away.

Avoid altering the selection in cursor.draw to prevent similar poorly timed
selections.

If the cursor position is lost, e.g. by click in the workspace, the arrow keys
now default it if needed. Tabbing to the workspace always shows a cursor.

Fixes:
- Click in workspace blank space now doesn't select a block on mouse down
- The mini-workspace for "if" and similar now behaves correctly again (no
  keyboard nav still though)
- Also fixes #287

Author: microbit-matt-hillsdon

src/actions/arrow_navigation.ts

@@ -64,7 +64,9 @@ export class ArrowNavigation {
             case Constants.STATE.WORKSPACE:
               isHandled = this.fieldShortcutHandler(workspace, shortcut);
               if (!isHandled && workspace) {
-                workspace.getCursor()?.in();
+                if (!this.navigation.defaultCursorPositionIfNeeded(workspace)) {
+                  workspace.getCursor()?.in();
+                }
                 isHandled = true;
               }
               return isHandled;
@@ -95,7 +97,9 @@ export class ArrowNavigation {
             case Constants.STATE.WORKSPACE:
               isHandled = this.fieldShortcutHandler(workspace, shortcut);
               if (!isHandled && workspace) {
-                workspace.getCursor()?.out();
+                if (!this.navigation.defaultCursorPositionIfNeeded(workspace)) {
+                  workspace.getCursor()?.out();
+                }
                 isHandled = true;
               }
               return isHandled;
@@ -125,7 +129,9 @@ export class ArrowNavigation {
             case Constants.STATE.WORKSPACE:
               isHandled = this.fieldShortcutHandler(workspace, shortcut);
               if (!isHandled && workspace) {
-                workspace.getCursor()?.next();
+                if (!this.navigation.defaultCursorPositionIfNeeded(workspace)) {
+                  workspace.getCursor()?.next();
+                }
                 isHandled = true;
               }
               return isHandled;
@@ -158,7 +164,14 @@ export class ArrowNavigation {
             case Constants.STATE.WORKSPACE:
               isHandled = this.fieldShortcutHandler(workspace, shortcut);
               if (!isHandled) {
-                workspace.getCursor()?.prev();
+                if (
+                  !this.navigation.defaultCursorPositionIfNeeded(
+                    workspace,
+                    'last',
+                  )
+                ) {
+                  workspace.getCursor()?.prev();
+                }
                 isHandled = true;
               }
               return isHandled;

src/actions/enter.ts

@@ -125,7 +125,7 @@ export class EnterAction {
    *     the block will be placed on.
    */
   private insertFromFlyout(workspace: WorkspaceSvg) {
-    const stationaryNode = workspace.getCursor()?.getCurNode();
+    const stationaryNode = this.navigation.getStationaryNode(workspace);
     const newBlock = this.createNewBlock(workspace);
     if (!newBlock) return;
     if (stationaryNode) {

src/index.ts

@@ -28,7 +28,7 @@ export class KeyboardNavigation {
   private focusListener: (e: Event) => void;
 
   /** Event handler run when the workspace loses focus. */
-  private blurListener: () => void;
+  private blurListener: (e: Event) => void;
 
   /** Event handler run when the toolbox gains focus. */
   private toolboxFocusListener: () => void;
@@ -129,8 +129,14 @@ export class KeyboardNavigation {
         this.navigationController.handleFocusWorkspace(workspace);
       }
     };
-    this.blurListener = () => {
-      this.navigationController.handleBlurWorkspace(workspace);
+    this.blurListener = (e: Event) => {
+      const relatedTarget = (e as FocusEvent).relatedTarget;
+      if (
+        relatedTarget !== this.workspace.getParentSvg() &&
+        relatedTarget !== this.workspace.getSvgGroup()
+      ) {
+        this.navigationController.handleBlurWorkspace(workspace);
+      }
     };
 
     workspace.getSvgGroup().addEventListener('focus', this.focusListener);

src/line_cursor.ts

@@ -15,7 +15,7 @@
 
 import * as Blockly from 'blockly/core';
 import {ASTNode, Marker} from 'blockly/core';
-import {scrollBoundsIntoView} from './workspace_utilities';
+import {getWorkspaceElement, scrollBoundsIntoView} from './workspace_utilities';
 
 /** Options object for LineCursor instances. */
 export type CursorOptions = {
@@ -474,47 +474,6 @@ export class LineCursor extends Marker {
     throw new Error('no valid nodes in this.potentialNodes');
   }
 
-  /**
-   * Get the current location of the cursor.
-   *
-   * Overrides superclass implementation to add a hack that attempts
-   * to detect if the user has moved focus by selecting a block and,
-   * if so, update the cursor location (and any highlighting) to
-   * match.
-   *
-   * Doing this only when getCurNode would naturally be called works
-   * reasonably well but has some glitches, most notably that if the
-   * cursor was not on a block (e.g. it was on a connection or the
-   * workspace) when the user selected a block then it will remain
-   * visible in its previous location until some keyboard navigation occurs.
-   *
-   * To ameliorate this, the LineCursor constructor adds an event
-   * listener that calls getCurNode in response to SELECTED events.
-   *
-   * Remove this hack once Blockly is modified to update the
-   * cursor/focus itself.
-   *
-   * @returns The current field, connection, or block the cursor is on.
-   */
-  override getCurNode(): ASTNode {
-    const curNode = super.getCurNode();
-    const selected = Blockly.common.getSelected();
-    if (selected?.workspace !== this.workspace) return curNode;
-
-    // Selected item is on workspace that this cursor belongs to.
-    const curLocation = curNode?.getLocation();
-    if (curLocation === selected) return curNode;
-
-    // Selected item is not where cursor is.  Try to move cursor.
-    if (!(selected instanceof Blockly.Block)) {
-      console.error('Selected item is not a block.  Ignoring');
-      return curNode;
-    }
-    const newNode = new ASTNode(ASTNode.types.BLOCK, selected);
-    this.setCurNode(newNode);
-    return newNode;
-  }
-
   /**
    * Set the location of the cursor and draw it.
    *
@@ -523,7 +482,25 @@ export class LineCursor extends Marker {
    *
    * @param newNode The new location of the cursor.
    */
-  override setCurNode(newNode: ASTNode) {
+  override setCurNode(newNode: ASTNode, selectionInSync = false) {
+    if (newNode?.getLocation() === this.getCurNode()?.getLocation()) {
+      return;
+    }
+    if (!selectionInSync) {
+      if (
+        newNode?.getType() === ASTNode.types.BLOCK &&
+        !(newNode.getLocation() as Blockly.BlockSvg).isShadow()
+      ) {
+        if (Blockly.common.getSelected() !== newNode.getLocation()) {
+          Blockly.common.setSelected(newNode.getLocation() as Blockly.BlockSvg);
+        }
+      } else {
+        if (Blockly.common.getSelected()) {
+          Blockly.common.setSelected(null);
+        }
+      }
+    }
+
     const oldNode = super.getCurNode();
     // Kludge: we can't set this.curNode directly, so we have to call
     // super.setCurNode(...) to do it for us - but that would call
@@ -533,6 +510,7 @@ export class LineCursor extends Marker {
     this.setDrawer(null as any); // Cast required since param is not nullable.
     super.setCurNode(newNode);
     this.setDrawer(drawer);
+
     // Draw this marker the way we want to.
     this.drawMarker(oldNode, newNode);
     // Try to scroll cursor into view.
@@ -545,22 +523,6 @@ export class LineCursor extends Marker {
     }
   }
 
-  override hide(): void {
-    super.hide();
-
-    // If there's a block currently selected, remove the selection since the
-    // cursor should now be hidden.
-    const curNode = this.getCurNode();
-    if (curNode && curNode.getType() === ASTNode.types.BLOCK) {
-      const block = curNode.getLocation() as Blockly.BlockSvg;
-      if (!block.isShadow()) {
-        Blockly.common.setSelected(null);
-      } else {
-        block.removeSelect();
-      }
-    }
-  }
-
   /**
    * Redraw the current marker.
    *
@@ -605,7 +567,7 @@ export class LineCursor extends Marker {
     if (oldNode?.getType() === ASTNode.types.BLOCK) {
       const block = oldNode.getLocation() as Blockly.BlockSvg;
       if (!block.isShadow()) {
-        Blockly.common.setSelected(null);
+        // Selection should already be in sync.
       } else {
         block.removeSelect();
       }
@@ -633,7 +595,7 @@ export class LineCursor extends Marker {
     } else if (curNodeType === ASTNode.types.BLOCK) {
       const block = curNode.getLocation() as Blockly.BlockSvg;
       if (!block.isShadow()) {
-        Blockly.common.setSelected(block);
+        // Selection should already be in sync.
       } else {
         block.addSelect();
       }
@@ -698,7 +660,22 @@ export class LineCursor extends Marker {
     if (event.type !== Blockly.Events.SELECTED) return;
     const selectedEvent = event as Blockly.Events.Selected;
     if (selectedEvent.workspaceId !== this.workspace.id) return;
-    this.getCurNode();
+    if (selectedEvent.newElementId) {
+      const block = this.workspace.getBlockById(selectedEvent.newElementId);
+      if (block) {
+        const node = ASTNode.createBlockNode(block);
+        if (node) {
+          this.setCurNode(node, true);
+        }
+      }
+    } else if (
+      this.getCurNode()?.getType() === ASTNode.types.BLOCK &&
+      !(this.getCurNode().getLocation() as Blockly.BlockSvg).isShadow()
+    ) {
+      // This does mean other cursor types remain if you click on the workspace
+      // background. Ideally gesture would be handling this with more context.
+      this.setCurNode(null as never, true);
+    }
   }
 }
 

src/navigation.ts

@@ -142,6 +142,18 @@ export class Navigation {
     return this.workspaceStates[workspace.id];
   }
 
+  /**
+   * Gets the node to use as context for insert operations.
+   *
+   * @param workspace The main workspace.
+   */
+  getStationaryNode(workspace: Blockly.WorkspaceSvg) {
+    return (
+      this.passiveFocusIndicator.getCurNode() ??
+      workspace.getCursor()?.getCurNode()
+    );
+  }
+
   /**
    * Adds all event listeners and cursors to the flyout that are needed for
    * keyboard navigation to work.
@@ -386,13 +398,19 @@ export class Navigation {
     this.setState(workspace, Constants.STATE.WORKSPACE);
     if (!Blockly.Gesture.inProgress()) {
       workspace.hideChaff();
+      // This will make a selection which would interfere with any gesture.
+      this.defaultCursorPositionIfNeeded(workspace);
     }
-    this.setCursorOnWorkspaceFocus(workspace, true);
 
     const cursor = workspace.getCursor();
     if (cursor) {
+      const passiveFocusNode = this.passiveFocusIndicator.getCurNode();
       this.passiveFocusIndicator.hide();
-      cursor.draw();
+      // If there's a gesture then it will either set the node or be a click
+      // that should not set one.
+      if (!Blockly.Gesture.inProgress() && passiveFocusNode) {
+        cursor.setCurNode(passiveFocusNode);
+      }
     }
   }
 
@@ -405,10 +423,11 @@ export class Navigation {
     this.setState(workspace, Constants.STATE.NOWHERE);
     const cursor = workspace.getCursor();
     if (cursor) {
-      cursor.hide();
       if (cursor.getCurNode()) {
         this.passiveFocusIndicator.show(cursor.getCurNode());
       }
+      // It's initially null so this is a valid state despite the types.
+      cursor.setCurNode(null as never);
     }
   }
 
@@ -546,43 +565,46 @@ export class Navigation {
   /**
    * Sets the cursor location when focusing the workspace.
    * Tries the following, in order, stopping after the first success:
-   *  - Resume editing by putting the cursor at the marker location, if any.
-   *  - Resume editing by returning the cursor to its previous location, if any.
+   *  - Resume editing by returning the cursor to its previous location, if valid.
    *  - Move the cursor to the top connection point on on the first top block.
    *  - Move the cursor to the default location on the workspace.
    *
    * @param workspace The main Blockly workspace.
-   * @param keepPosition Whether to retain the cursor's previous position.
+   * @return true if the cursor location was defaulted.
    */
-  setCursorOnWorkspaceFocus(
+  defaultCursorPositionIfNeeded(
     workspace: Blockly.WorkspaceSvg,
-    keepPosition: boolean,
+    prefer: 'first' | 'last' = 'first',
   ) {
     const topBlocks = workspace.getTopBlocks(true);
     const cursor = workspace.getCursor();
     if (!cursor) {
       return;
     }
-
     const disposed = cursor.getCurNode()?.getSourceBlock()?.disposed;
-    if (cursor.getCurNode() && !disposed && keepPosition) {
+    if (cursor.getCurNode() && !disposed) {
       // Retain the cursor's previous position since it's set, but only if not
       // disposed (which can happen when blocks are reloaded).
-      return;
+      return false;
     }
     const wsCoordinates = new Blockly.utils.Coordinate(
       this.DEFAULT_WS_COORDINATE.x / workspace.scale,
       this.DEFAULT_WS_COORDINATE.y / workspace.scale,
     );
     if (topBlocks.length > 0) {
-      cursor.setCurNode(Blockly.ASTNode.createTopNode(topBlocks[0])!);
+      cursor.setCurNode(
+        Blockly.ASTNode.createTopNode(
+          topBlocks[prefer === 'first' ? 0 : topBlocks.length - 1],
+        )!,
+      );
     } else {
       const wsNode = Blockly.ASTNode.createWorkspaceNode(
         workspace,
         wsCoordinates,
       );
       cursor.setCurNode(wsNode!);
     }
+    return true;
   }
 
   /**

src/passive_focus.ts

@@ -30,6 +30,15 @@ export class PassiveFocus {
     this.nextConnectionIndicator = this.createNextIndicator();
   }
 
+  /**
+   * Get the current passive focus node.
+   *
+   * @returns the node or null.
+   */
+  getCurNode(): ASTNode | null {
+    return this.curNode;
+  }
+
   /** Dispose of this indicator. Do any necessary cleanup. */
   dispose() {
     this.hide();
@@ -104,7 +113,11 @@ export class PassiveFocus {
    */
   hideAtBlock(node: ASTNode) {
     const block = node.getLocation() as BlockSvg;
-    utils.dom.removeClass(block.pathObject.svgPath, 'passiveBlockFocus');
+    // When a block is selected we can end up with a duplicate svgPath.
+    const svgPaths = block.getSvgRoot().querySelectorAll('.passiveBlockFocus');
+    svgPaths.forEach((svgPath) =>
+      utils.dom.removeClass(svgPath, 'passiveBlockFocus'),
+    );
   }
 
   /**