fix: right click menu issues (#331)

* fix: right click menu issues

PR #326 introduced an issue with stale current node when showing the context
menu by mouse. It's only possible to fix this by reinstating the updating of
curNode from the selection.

We also attempt to avoid showing passive focus when using the context menu and
widgets (via mouse and keyboard) for consistency with the normal Blockly
experience.

Fixes an issue clicking on shadow blocks which could previously show an outline
around the shadow and parent block.

* chore: remove reference to fixed bug

* chore: typo in comment

* fix: null node case for action menu

* fix: stale curNode when clicking the workspace

Due to an unintentially commented out line, clicking the workspace wouldn't
clear the current node so e.g. delete would act on a block that didn't have a
visual selection.

Re-enabling that required revisiting shadow blocks to prevent getCurNode
unexpectedly clearing based on a null selection. We now always set/use the
selection for shadow blocks and take care not to move the cursor unexpectedly
as it can be in two positions for a shadow block selection.

---------

Co-authored-by: Grace <[email protected]>

Author: microbit-matt-hillsdon

src/actions/action_menu.ts

@@ -111,7 +111,7 @@ export class ActionMenu {
     const cursor = workspace.getCursor();
     if (!cursor) throw new Error('workspace has no cursor');
     const node = cursor.getCurNode();
-    if (!node) throw new Error('No node is currently selected');
+    if (!node) return false;
     const nodeType = node.getType();
     switch (nodeType) {
       case ASTNode.types.BLOCK:

src/index.ts

@@ -30,6 +30,9 @@ export class KeyboardNavigation {
   /** Event handler run when the workspace loses focus. */
   private blurListener: () => void;
 
+  /** Event handler run when the widget or dropdown div loses focus. */
+  private widgetDropDownDivFocusOutListener: (e: Event) => void;
+
   /** Event handler run when the toolbox gains focus. */
   private toolboxFocusListener: () => void;
 
@@ -158,6 +161,22 @@ export class KeyboardNavigation {
     workspace.getSvgGroup().addEventListener('focus', this.focusListener);
     workspace.getSvgGroup().addEventListener('blur', this.blurListener);
 
+    this.widgetDropDownDivFocusOutListener = (e: Event) => {
+      this.navigationController.handleFocusOutWidgetDropdownDiv(
+        workspace,
+        (e as FocusEvent).relatedTarget,
+      );
+    };
+
+    Blockly.WidgetDiv.getDiv()?.addEventListener(
+      'focusout',
+      this.widgetDropDownDivFocusOutListener,
+    );
+    Blockly.DropDownDiv.getContentDiv()?.addEventListener(
+      'focusout',
+      this.widgetDropDownDivFocusOutListener,
+    );
+
     const toolboxElement = getToolboxElement(workspace);
     this.toolboxFocusListener = () => {
       this.navigationController.handleFocusToolbox(workspace);
@@ -208,6 +227,15 @@ export class KeyboardNavigation {
       .getSvgGroup()
       .removeEventListener('focus', this.focusListener);
 
+    Blockly.WidgetDiv.getDiv()?.removeEventListener(
+      'focusout',
+      this.widgetDropDownDivFocusOutListener,
+    );
+    Blockly.DropDownDiv.getContentDiv()?.removeEventListener(
+      'focusout',
+      this.widgetDropDownDivFocusOutListener,
+    );
+
     const toolboxElement = getToolboxElement(this.workspace);
     toolboxElement?.removeEventListener('focus', this.toolboxFocusListener);
     toolboxElement?.removeEventListener('blur', this.toolboxBlurListener);

src/line_cursor.ts

@@ -15,7 +15,7 @@
 
 import * as Blockly from 'blockly/core';
 import {ASTNode, Marker} from 'blockly/core';
-import {getWorkspaceElement, scrollBoundsIntoView} from './workspace_utilities';
+import {scrollBoundsIntoView} from './workspace_utilities';
 
 /** Options object for LineCursor instances. */
 export type CursorOptions = {
@@ -61,7 +61,7 @@ export class LineCursor extends Marker {
   ) {
     super();
     // Bind selectListener to facilitate future install/uninstall.
-    this.selectListener = this.selectListener.bind(this);
+    this.changeListener = this.changeListener.bind(this);
     // Regularise options and apply defaults.
     this.options = {...defaultOptions, ...options};
 
@@ -81,7 +81,7 @@ export class LineCursor extends Marker {
     markerManager.setCursor(this);
     const oldCursorNode = this.oldCursor?.getCurNode();
     if (oldCursorNode) this.setCurNode(oldCursorNode);
-    this.workspace.addChangeListener(this.selectListener);
+    this.workspace.addChangeListener(this.changeListener);
     this.installed = true;
   }
 
@@ -92,7 +92,7 @@ export class LineCursor extends Marker {
    */
   uninstall() {
     if (!this.installed) throw new Error('LineCursor not yet installed');
-    this.workspace.removeChangeListener(this.selectListener.bind(this));
+    this.workspace.removeChangeListener(this.changeListener.bind(this));
     if (this.oldCursor) {
       this.workspace.getMarkerManager().setCursor(this.oldCursor);
     }
@@ -475,6 +475,21 @@ export class LineCursor extends Marker {
     throw new Error('no valid nodes in this.potentialNodes');
   }
 
+  /**
+   * Get the current location of the cursor.
+   *
+   * Overrides normal Marker getCurNode to update the current node from the selected
+   * block. This typically happens via the selection listener but that is not called
+   * immediately when `Gesture` calls `Blockly.common.setSelected`.
+   * In particular the listener runs after showing the context menu.
+   *
+   * @returns The current field, connection, or block the cursor is on.
+   */
+  override getCurNode(): Blockly.ASTNode | null {
+    this.updateCurNodeFromSelection();
+    return super.getCurNode();
+  }
+
   /**
    * Sets the object in charge of drawing the marker.
    *
@@ -516,28 +531,11 @@ export class LineCursor extends Marker {
    * this.drawMarker() instead of this.drawer.draw() directly.
    *
    * @param newNode The new location of the cursor.
+   * @param selectionUpToDate If false (the default) we'll update the selection too.
    */
-  override setCurNode(newNode: ASTNode | null, 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.Events.disable();
-          Blockly.common.setSelected(newNode.getLocation() as Blockly.BlockSvg);
-          Blockly.Events.enable();
-        }
-      } else {
-        if (Blockly.common.getSelected()) {
-          Blockly.Events.disable();
-          Blockly.common.setSelected(null);
-          Blockly.Events.enable();
-        }
-      }
+  override setCurNode(newNode: ASTNode | null, selectionUpToDate = false) {
+    if (!selectionUpToDate) {
+      this.updateSelectionFromNode(newNode);
     }
 
     super.setCurNode(newNode);
@@ -616,6 +614,7 @@ export class LineCursor extends Marker {
         // Selection should already be in sync.
       } else {
         block.addSelect();
+        block.getParent()?.removeSelect();
       }
     }
 
@@ -671,23 +670,98 @@ export class LineCursor extends Marker {
   }
 
   /**
-   * Event listener that syncs the cursor location to the selected
-   * block on SELECTED events.
+   * Event listener that syncs the cursor location to the selected block on
+   * SELECTED events.
+   *
+   * This does not run early enough in all cases so `getCurNode()` also updates
+   * the node from the selection.
+   *
+   * @param event The `Selected` event.
    */
-  private selectListener(event: Blockly.Events.Abstract) {
-    if (event.type !== Blockly.Events.SELECTED) return;
-    const selectedEvent = event as Blockly.Events.Selected;
-    if (selectedEvent.workspaceId !== this.workspace.id) return;
-    if (selectedEvent.newElementId) {
-      const block = this.workspace.getBlockById(selectedEvent.newElementId);
-      if (block) {
-        const node = ASTNode.createBlockNode(block);
-        if (node) {
-          this.setCurNode(node, true);
+  private changeListener(event: Blockly.Events.Abstract) {
+    switch (event.type) {
+      case Blockly.Events.SELECTED:
+        this.updateCurNodeFromSelection();
+        break;
+      case Blockly.Events.CLICK: {
+        const click = event as Blockly.Events.Click;
+        if (
+          click.workspaceId === this.workspace.id &&
+          click.targetType === Blockly.Events.ClickTarget.WORKSPACE
+        ) {
+          this.setCurNode(null);
         }
       }
+    }
+  }
+
+  /**
+   * Updates the current node to match the selection.
+   *
+   * Clears the current node if it's on a block but the selection is null.
+   * Sets the node to a block if selected for our workspace.
+   * For shadow blocks selections the parent is used by default (unless we're
+   * already on the shadow block via keyboard) as that's where the visual
+   * selection is.
+   */
+  private updateCurNodeFromSelection() {
+    const curNode = super.getCurNode();
+    const selected = Blockly.common.getSelected();
+
+    if (
+      selected === null &&
+      curNode?.getType() === Blockly.ASTNode.types.BLOCK
+    ) {
+      this.setCurNode(null, true);
+      return;
+    }
+    if (selected?.workspace !== this.workspace) {
+      return;
+    }
+    if (selected instanceof Blockly.BlockSvg) {
+      let block: Blockly.BlockSvg | null = selected;
+      if (selected.isShadow()) {
+        // OK if the current node is on the parent OR the shadow block.
+        // The former happens for clicks, the latter for keyboard nav.
+        if (
+          curNode &&
+          (curNode.getLocation() === block ||
+            curNode.getLocation() === block.getParent())
+        ) {
+          return;
+        }
+        block = block.getParent();
+      }
+      if (block) {
+        this.setCurNode(Blockly.ASTNode.createBlockNode(block)!, true);
+      }
+    }
+  }
+
+  /**
+   * Updates the selection from the node.
+   *
+   * Clears the selection for non-block nodes.
+   * Clears the selection for shadow blocks as the selection is drawn on
+   * the parent but the cursor will be drawn on the shadow block itself.
+   * We need to take care not to later clear the current node due to that null
+   * selection, so we track the latest selection we're in sync with.
+   *
+   * @param newNode The new node.
+   */
+  private updateSelectionFromNode(newNode: Blockly.ASTNode | null) {
+    if (newNode?.getType() === ASTNode.types.BLOCK) {
+      if (Blockly.common.getSelected() !== newNode.getLocation()) {
+        Blockly.Events.disable();
+        Blockly.common.setSelected(newNode.getLocation() as Blockly.BlockSvg);
+        Blockly.Events.enable();
+      }
     } else {
-      this.setCurNode(null as never, true);
+      if (Blockly.common.getSelected()) {
+        Blockly.Events.disable();
+        Blockly.common.setSelected(null);
+        Blockly.Events.enable();
+      }
     }
   }
 }

src/navigation.ts

@@ -415,14 +415,22 @@ export class Navigation {
   }
 
   /**
-   * Clears the navigation state and switches to using the passive focus indicator.
+   * Clears navigation state and switches to using the passive focus indicator
+   * if it is not the context menu / field input that is causing blur.
    *
    * @param workspace The workspace that has lost focus.
+   * @param ignorePopUpDivs Whether to skip the focus indicator change when
+   *     the widget/dropdown divs are open.
    */
-  handleBlurWorkspace(workspace: Blockly.WorkspaceSvg) {
+  handleBlurWorkspace(
+    workspace: Blockly.WorkspaceSvg,
+    ignorePopUpDivs = false,
+  ) {
     this.setState(workspace, Constants.STATE.NOWHERE);
     const cursor = workspace.getCursor();
-    if (cursor) {
+    const popUpDivsShowing =
+      Blockly.WidgetDiv.isVisible() || Blockly.DropDownDiv.isVisible();
+    if (cursor && (ignorePopUpDivs || !popUpDivsShowing)) {
       const curNode = cursor.getCurNode();
       if (curNode) {
         this.passiveFocusIndicator.show(curNode);
@@ -432,6 +440,32 @@ export class Navigation {
     }
   }
 
+  /**
+   * Handle the widget or dropdown div losing focus (via focusout).
+   *
+   * Because we skip the widget/dropdown div cases in `handleBlurWorkspace` we need
+   * to catch them here.
+   *
+   * @param workspace The workspace.
+   * @param relatedTarget The related target (newly focused element if any).
+   */
+  handleFocusOutWidgetDropdownDiv(
+    workspace: Blockly.WorkspaceSvg,
+    relatedTarget: EventTarget | null,
+  ) {
+    if (relatedTarget === null) {
+      // Workaround:
+      // Skip document.body/null case until this blur bug is fixed to avoid
+      // flipping to passive focus as the user moves their mouse over the
+      // colour picker.
+      // https://github.com/google/blockly-samples/issues/2498
+      return;
+    }
+    if (relatedTarget !== getWorkspaceElement(workspace)) {
+      this.handleBlurWorkspace(workspace, true);
+    }
+  }
+
   /**
    * Sets browser focus to the toolbox (if any).
    *

src/navigation_controller.ts

@@ -196,6 +196,13 @@ export class NavigationController {
     this.navigation.handleBlurWorkspace(workspace);
   }
 
+  handleFocusOutWidgetDropdownDiv(
+    workspace: Blockly.WorkspaceSvg,
+    relatedTarget: EventTarget | null,
+  ) {
+    this.navigation.handleFocusOutWidgetDropdownDiv(workspace, relatedTarget);
+  }
+
   focusToolbox(workspace: Blockly.WorkspaceSvg) {
     this.navigation.focusToolbox(workspace);
   }