fix: Allow some things to handle the enter key on read only workspaces (#597)

RoboErikG · web-flow · commit fd1e4fb8bb81 · 2025-06-18T12:32:39.000-07:00
* Allow some things to handle return on read only workspaces

* Allow navigating to the toolbox on readonly workspaces

* Allow navigating to the main workspace from readonly workspaces

* Make tests with mouse more robust

* Minor fixes and refactor out a method to check if ws should handle return

* Respond to comments

* Change default for handling enter to false.
diff --git a/src/actions/action_menu.ts b/src/actions/action_menu.ts
@@ -52,7 +52,7 @@ export class ActionMenu {
         );
       },
       callback: (workspace) => {
-        switch (this.navigation.getState(workspace)) {
+        switch (this.navigation.getState()) {
           case Constants.STATE.WORKSPACE:
           case Constants.STATE.FLYOUT:
             return this.openActionMenu(workspace);
diff --git a/src/actions/arrow_navigation.ts b/src/actions/arrow_navigation.ts
@@ -63,7 +63,7 @@ export class ArrowNavigation {
         ? workspace.targetWorkspace?.getFlyout()
         : workspace.getFlyout();
       let isHandled = false;
-      switch (this.navigation.getState(workspace)) {
+      switch (this.navigation.getState()) {
         case Constants.STATE.WORKSPACE:
           isHandled = this.fieldShortcutHandler(workspace, shortcut);
           if (!isHandled && workspace) {
@@ -97,7 +97,7 @@ export class ArrowNavigation {
         ? workspace.targetWorkspace?.getToolbox()
         : workspace.getToolbox();
       let isHandled = false;
-      switch (this.navigation.getState(workspace)) {
+      switch (this.navigation.getState()) {
         case Constants.STATE.WORKSPACE:
           isHandled = this.fieldShortcutHandler(workspace, shortcut);
           if (!isHandled && workspace) {
@@ -161,7 +161,7 @@ export class ArrowNavigation {
         callback: (workspace, e, shortcut) => {
           keyboardNavigationController.setIsActive(true);
           let isHandled = false;
-          switch (this.navigation.getState(workspace)) {
+          switch (this.navigation.getState()) {
             case Constants.STATE.WORKSPACE:
               isHandled = this.fieldShortcutHandler(workspace, shortcut);
               if (!isHandled && workspace) {
@@ -223,7 +223,7 @@ export class ArrowNavigation {
         callback: (workspace, e, shortcut) => {
           keyboardNavigationController.setIsActive(true);
           let isHandled = false;
-          switch (this.navigation.getState(workspace)) {
+          switch (this.navigation.getState()) {
             case Constants.STATE.WORKSPACE:
               isHandled = this.fieldShortcutHandler(workspace, shortcut);
               if (!isHandled) {
diff --git a/src/actions/clipboard.ts b/src/actions/clipboard.ts
@@ -285,7 +285,9 @@ export class Clipboard {
       !!this.oldCopyShortcut?.callback &&
       this.oldCopyShortcut.callback(workspace, e, shortcut, scope);
     if (didCopy) {
-      this.copyWorkspace = workspace;
+      this.copyWorkspace = workspace.isFlyout
+        ? workspace.targetWorkspace
+        : workspace;
       showCopiedHint(workspace);
     }
     return didCopy;
diff --git a/src/actions/disconnect.ts b/src/actions/disconnect.ts
@@ -58,7 +58,7 @@ export class DisconnectAction {
         this.navigation.canCurrentlyEdit(workspace),
       callback: (workspace) => {
         keyboardNavigationController.setIsActive(true);
-        switch (this.navigation.getState(workspace)) {
+        switch (this.navigation.getState()) {
           case Constants.STATE.WORKSPACE:
             this.disconnectBlocks(workspace);
             return true;
diff --git a/src/actions/enter.ts b/src/actions/enter.ts
@@ -6,7 +6,6 @@
 
 import {
   Events,
-  Msg,
   ShortcutRegistry,
   utils as BlocklyUtils,
   getFocusManager,
@@ -54,33 +53,48 @@ export class EnterAction {
      */
     ShortcutRegistry.registry.register({
       name: Constants.SHORTCUT_NAMES.EDIT_OR_CONFIRM,
-      preconditionFn: (workspace) =>
-        this.navigation.canCurrentlyEdit(workspace),
-      callback: (workspace, event) => {
+      preconditionFn: (workspace): boolean => {
+        switch (this.navigation.getState()) {
+          case Constants.STATE.WORKSPACE:
+            return this.shouldHandleEnterForWS(workspace);
+          case Constants.STATE.FLYOUT: {
+            // If we're in the flyout the only supported actions are inserting
+            // blocks or clicking buttons, so don't handle this if the
+            // main workspace is read only.
+            const targetWorkspace = workspace.isFlyout
+              ? workspace.targetWorkspace
+              : workspace;
+            return !!targetWorkspace && !targetWorkspace.isReadOnly();
+          }
+          default:
+            return false;
+        }
+      },
+      callback: (workspace, event): boolean => {
         event.preventDefault();
 
+        const targetWorkspace = workspace.isFlyout
+          ? workspace.targetWorkspace
+          : workspace;
+        if (!targetWorkspace) return false;
+
         let flyoutCursor;
         let curNode;
 
-        switch (this.navigation.getState(workspace)) {
+        switch (this.navigation.getState()) {
           case Constants.STATE.WORKSPACE:
-            this.handleEnterForWS(workspace);
-            return true;
+            return this.handleEnterForWS(workspace);
           case Constants.STATE.FLYOUT:
-            if (!workspace.targetWorkspace) return false;
-            flyoutCursor = this.navigation.getFlyoutCursor(
-              workspace.targetWorkspace,
-            );
+            flyoutCursor = this.navigation.getFlyoutCursor(targetWorkspace);
             if (!flyoutCursor) {
               return false;
             }
             curNode = flyoutCursor.getCurNode();
             if (curNode instanceof BlockSvg) {
-              this.insertFromFlyout(workspace.targetWorkspace);
+              this.insertFromFlyout(targetWorkspace);
             } else if (curNode instanceof FlyoutButton) {
               this.triggerButtonCallback(workspace);
             }
-
             return true;
           default:
             return false;
@@ -90,30 +104,57 @@ export class EnterAction {
     });
   }
 
+  /**
+   * Checks if the enter key should do anything for this ws.
+   *
+   * @param workspace The workspace to check.
+   * @returns True if the enter action should be handled.
+   */
+  private shouldHandleEnterForWS(workspace: WorkspaceSvg): boolean {
+    const cursor = workspace.getCursor();
+    const curNode = cursor?.getCurNode();
+    if (!curNode) return false;
+    if (curNode instanceof Field) return curNode.isClickable();
+    if (
+      curNode instanceof RenderedConnection ||
+      curNode instanceof WorkspaceSvg
+    ) {
+      return !workspace.isReadOnly();
+    }
+    // Returning true is sometimes incorrect for icons, but there's no API to check.
+    if (curNode instanceof icons.Icon) return true;
+    return false;
+  }
+
   /**
    * Handles hitting the enter key on the workspace.
    *
    * @param workspace The workspace.
+   * @returns True if the enter was handled, false otherwise.
    */
-  private handleEnterForWS(workspace: WorkspaceSvg) {
+  private handleEnterForWS(workspace: WorkspaceSvg): boolean {
     const cursor = workspace.getCursor();
-    if (!cursor) return;
-    const curNode = cursor.getCurNode();
-    if (!curNode) return;
+    const curNode = cursor?.getCurNode();
+    if (!curNode) return false;
     if (curNode instanceof Field) {
       curNode.showEditor();
+      return true;
     } else if (curNode instanceof BlockSvg) {
       if (!this.tryShowFullBlockFieldEditor(curNode)) {
         showHelpHint(workspace);
       }
+      return true;
     } else if (
       curNode instanceof RenderedConnection ||
       curNode instanceof WorkspaceSvg
     ) {
       this.navigation.openToolboxOrFlyout(workspace);
+      return true;
     } else if (curNode instanceof icons.Icon) {
       curNode.onClick();
+      return true;
     }
+    return false;
   }
 
   /**
diff --git a/src/actions/exit.ts b/src/actions/exit.ts
@@ -31,7 +31,7 @@ export class ExitAction {
       preconditionFn: (workspace) =>
         this.navigation.canCurrentlyNavigate(workspace),
       callback: (workspace) => {
-        switch (this.navigation.getState(workspace)) {
+        switch (this.navigation.getState()) {
           case Constants.STATE.FLYOUT:
           case Constants.STATE.TOOLBOX:
             getFocusManager().focusTree(workspace.targetWorkspace ?? workspace);
diff --git a/src/actions/mover.ts b/src/actions/mover.ts
@@ -85,7 +85,7 @@ export class Mover {
    */
   canMove(workspace: WorkspaceSvg, block: BlockSvg) {
     return !!(
-      this.navigation.getState(workspace) === Constants.STATE.WORKSPACE &&
+      this.navigation.getState() === Constants.STATE.WORKSPACE &&
       this.navigation.canCurrentlyEdit(workspace) &&
       !this.moves.has(workspace) && // No move in progress.
       block?.isMovable()
diff --git a/src/actions/ws_movement.ts b/src/actions/ws_movement.ts
@@ -68,11 +68,16 @@ export class WorkspaceMovement {
     /** Move the cursor to the workspace. */
     {
       name: Constants.SHORTCUT_NAMES.CREATE_WS_CURSOR,
-      preconditionFn: (workspace) =>
-        this.navigation.canCurrentlyEdit(workspace),
+      preconditionFn: (workspace) => {
+        return true;
+      },
       callback: (workspace) => {
+        const targetWorkspace = workspace.isFlyout
+          ? workspace.targetWorkspace
+          : workspace;
+        if (!targetWorkspace) return false;
         keyboardNavigationController.setIsActive(true);
-        return this.createWSCursor(workspace);
+        return this.createWSCursor(targetWorkspace);
       },
       keyCodes: [KeyCodes.W],
     },
diff --git a/src/navigation.ts b/src/navigation.ts
@@ -93,10 +93,9 @@ export class Navigation {
    * Note that this assumes a workspace with passive focus (including for its
    * toolbox or flyout) has a state of NOWHERE.
    *
-   * @param workspace The workspace to get the state of.
    * @returns The state of the given workspace.
    */
-  getState(workspace: Blockly.WorkspaceSvg): Constants.STATE {
+  getState(): Constants.STATE {
     const focusedTree = Blockly.getFocusManager().getFocusedTree();
     if (focusedTree instanceof Blockly.WorkspaceSvg) {
       if (focusedTree.isFlyout) {
@@ -105,9 +104,7 @@ export class Navigation {
         return Constants.STATE.WORKSPACE;
       }
     } else if (focusedTree instanceof Blockly.Toolbox) {
-      if (workspace === focusedTree.getWorkspace()) {
-        return Constants.STATE.TOOLBOX;
-      }
+      return Constants.STATE.TOOLBOX;
     } else if (focusedTree instanceof Blockly.Flyout) {
       return Constants.STATE.FLYOUT;
     }
@@ -222,7 +219,7 @@ export class Navigation {
       }
     } else if (
       e.type === Blockly.Events.BLOCK_CREATE &&
-      this.getState(mainWorkspace) === Constants.STATE.FLYOUT
+      this.getState() === Constants.STATE.FLYOUT
     ) {
       // When variables are created, that recreates the flyout contents, leaving the
       // cursor in an invalid state.
@@ -825,7 +822,7 @@ export class Navigation {
       : workspace.keyboardAccessibilityMode;
     return (
       !!accessibilityMode &&
-      this.getState(workspace) !== Constants.STATE.NOWHERE &&
+      this.getState() !== Constants.STATE.NOWHERE &&
       !Blockly.getFocusManager().ephemeralFocusTaken()
     );
   }
diff --git a/src/navigation_controller.ts b/src/navigation_controller.ts
@@ -196,11 +196,10 @@ export class NavigationController {
     /** Move focus to or from the toolbox. */
     focusToolbox: {
       name: Constants.SHORTCUT_NAMES.TOOLBOX,
-      preconditionFn: (workspace) =>
-        !workspace.isDragging() && this.navigation.canCurrentlyEdit(workspace),
+      preconditionFn: (workspace) => !workspace.isDragging(),
       callback: (workspace) => {
         keyboardNavigationController.setIsActive(true);
-        switch (this.navigation.getState(workspace)) {
+        switch (this.navigation.getState()) {
           case Constants.STATE.WORKSPACE:
             Blockly.getFocusManager().focusTree(
               workspace.getToolbox() ??
diff --git a/test/webdriverio/test/keyboard_mode_test.ts b/test/webdriverio/test/keyboard_mode_test.ts
@@ -13,6 +13,7 @@ import {
   PAUSE_TIME,
   getBlockElementById,
   tabNavigateToWorkspace,
+  clickBlock,
 } from './test_setup.js';
 import {Key} from 'webdriverio';
 
@@ -125,8 +126,7 @@ suite(
 
       await this.browser.pause(PAUSE_TIME);
       // Right click a block
-      const element = await getBlockElementById(this.browser, 'controls_if_1');
-      await element.click({button: 'right'});
+      clickBlock(this.browser, 'controls_if_1', {button: 'right'});
       await this.browser.pause(PAUSE_TIME);
 
       chai.assert.isFalse(await isKeyboardNavigating(this.browser));
@@ -140,6 +140,15 @@ suite(
       await this.browser.pause(PAUSE_TIME);
       // Drag a block
       const element = await getBlockElementById(this.browser, 'controls_if_1');
+
+      await this.browser.execute(() => {
+        const ws = Blockly.getMainWorkspace() as Blockly.WorkspaceSvg;
+        const block = ws.getBlockById('controls_if_1') as Blockly.BlockSvg;
+        ws.scrollBoundsIntoView(
+          block.getBoundingRectangleWithoutChildren(),
+          10,
+        );
+      });
       await element.dragAndDrop({x: 10, y: 10});
       await this.browser.pause(PAUSE_TIME);
 
diff --git a/test/webdriverio/test/test_setup.ts b/test/webdriverio/test/test_setup.ts
@@ -555,3 +555,59 @@ export async function contextMenuExists(
   const item = await browser.$(`div=${itemText}`);
   return await item.waitForExist({timeout: 200, reverse: reverse});
 }
+
+/**
+ * Find a clickable element on the block and click it.
+ * We can't always use the block's SVG root because clicking will always happen
+ * in the middle of the block's bounds (including children) by default, which
+ * causes problems if it has holes (e.g. statement inputs). Instead, this tries
+ * to get the first text field on the block. It falls back on the block's SVG root.
+ *
+ * @param browser The active WebdriverIO Browser object.
+ * @param blockId The id of the block to click, as an interactable element.
+ * @param clickOptions The options to pass to webdriverio's element.click function.
+ * @return A Promise that resolves when the actions are completed.
+ */
+export async function clickBlock(
+  browser: WebdriverIO.Browser,
+  blockId: string,
+  clickOptions?: Partial<webdriverio.ClickOptions> | undefined,
+) {
+  const findableId = 'clickTargetElement';
+  // In the browser context, find the element that we want and give it a findable ID.
+  await browser.execute(
+    (blockId, newElemId) => {
+      const ws = Blockly.getMainWorkspace() as Blockly.WorkspaceSvg;
+      const block = ws.getBlockById(blockId) as Blockly.BlockSvg;
+      // Ensure the block we want to click is within the viewport.
+      ws.scrollBoundsIntoView(block.getBoundingRectangleWithoutChildren(), 10);
+      if (!block.isCollapsed()) {
+        for (const input of block.inputList) {
+          for (const field of input.fieldRow) {
+            if (field instanceof Blockly.FieldLabel) {
+              const svgRoot = field.getSvgRoot();
+              if (svgRoot) {
+                svgRoot.id = newElemId;
+                return;
+              }
+            }
+          }
+        }
+      }
+      // No label field found. Fall back to the block's SVG root.
+      block.getSvgRoot().id = newElemId;
+    },
+    blockId,
+    findableId,
+  );
+
+  // In the test context, get the Webdriverio Element that we've identified.
+  const elem = await browser.$(`#${findableId}`);
+
+  await elem.click(clickOptions);
+
+  // In the browser context, remove the ID.
+  await browser.execute((elemId) => {
+    document.getElementById(elemId)?.removeAttribute('id');
+  }, findableId);
+}
