fix: right click menu issues

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: (e: Event) => 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;
 
@@ -142,6 +145,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);
@@ -197,6 +216,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);
   }