fix: Make left and right arrow keys only navigate within the current block/comment (#9531)

* fix: Make left and right arrow keys only navigate within the current block/comment

* fix: Fix tests

* fix: Remove .only from test suite

* test: Reenable statement block navigation tests with modifications

* chore: Clarify comments

core/comments/comment_editor.ts

@@ -4,7 +4,9 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import type {BlockSvg} from '../block_svg.js';
 import * as browserEvents from '../browser_events.js';
+import type {RenderedWorkspaceComment} from '../comments/rendered_workspace_comment.js';
 import {getFocusManager} from '../focus_manager.js';
 import {IFocusableNode} from '../interfaces/i_focusable_node.js';
 import {IFocusableTree} from '../interfaces/i_focusable_tree.js';
@@ -40,6 +42,8 @@ export class CommentEditor implements IFocusableNode {
   /** The current text of the comment. Updates on text area change. */
   private text: string = '';
 
+  private parent?: BlockSvg | RenderedWorkspaceComment;
+
   constructor(
     public workspace: WorkspaceSvg,
     commentId?: string,
@@ -207,4 +211,21 @@ export class CommentEditor implements IFocusableNode {
     if (this.id) return true;
     return false;
   }
+
+  /**
+   * Sets the parent object that owns this comment editor.
+   *
+   * @param newParent The parent of this comment editor.
+   * @internal
+   */
+  setParent(newParent: BlockSvg | RenderedWorkspaceComment) {
+    this.parent = newParent;
+  }
+
+  /**
+   * Returns the parent object that owns this comment editor, if any.
+   */
+  getParent() {
+    return this.parent;
+  }
 }

core/comments/rendered_workspace_comment.ts

@@ -31,6 +31,7 @@ import {Rect} from '../utils/rect.js';
 import {Size} from '../utils/size.js';
 import * as svgMath from '../utils/svg_math.js';
 import {WorkspaceSvg} from '../workspace_svg.js';
+import type {CommentEditor} from './comment_editor.js';
 import {CommentView} from './comment_view.js';
 import {WorkspaceComment} from './workspace_comment.js';
 
@@ -60,6 +61,7 @@ export class RenderedWorkspaceComment
     this.workspace = workspace;
 
     this.view = new CommentView(workspace, this.id);
+    (this.view.getEditorFocusableNode() as CommentEditor).setParent(this);
     // Set the size to the default size as defined in the superclass.
     this.view.setSize(this.getSize());
     this.view.setEditable(this.isEditable());

core/icons/comment_icon.ts

@@ -381,6 +381,9 @@ export class CommentIcon extends Icon implements IHasBubble, ISerializable {
       this.getBubbleOwnerRect(),
       this,
     );
+    this.textInputBubble
+      .getEditor()
+      .setParent(this.getSourceBlock() as BlockSvg);
     this.textInputBubble.setText(this.getText());
     this.textInputBubble.setSize(this.bubbleSize, true);
     if (this.bubbleLocation) {

core/keyboard_nav/line_cursor.ts

@@ -345,7 +345,34 @@ export class LineCursor extends Marker {
     switch (direction) {
       case NavigationDirection.IN:
       case NavigationDirection.OUT:
-        return () => true;
+        return (candidate: IFocusableNode | null) => {
+          const candidateBlock = this.getSourceBlockFromNode(candidate);
+          const currentBlock = this.getSourceBlock();
+
+          // Preventing escaping the current block/comment/etc by:
+          // Disallow moving from a node with a block to a non-block node (other than a block comment editor)
+          // Disallow moving from a non-block node to a block node
+          // Disallow moving to the workspace
+          if (
+            (currentBlock && !candidateBlock) ||
+            (!currentBlock && candidateBlock) ||
+            candidate === this.workspace
+          ) {
+            return false;
+          }
+
+          if (!candidateBlock || !currentBlock) return true;
+
+          const currentParents = this.getOutputParents(currentBlock);
+          const candidateParents = this.getOutputParents(candidateBlock);
+          // If we're navigating from a block (or nested element) to a block
+          // (or nested element), ensure that we're not crossing a statement
+          // block boundary (i.e. moving to a next or previous block vertically)
+          // by verifying that the two blocks in question are either the same
+          // or have a common parent accessible only by traversing output
+          // connections, meaning that they are part of the same row.
+          return candidateParents.intersection(currentParents).size > 0;
+        };
       case NavigationDirection.NEXT:
       case NavigationDirection.PREVIOUS:
         return (candidate: IFocusableNode | null) => {
@@ -452,6 +479,25 @@ export class LineCursor extends Marker {
     return parents;
   }
 
+  /**
+   * Returns a set of all of the parent blocks connected to an output of the
+   * given block or one of its parents. Also includes the given block.
+   *
+   * @param block The block to retrieve the output-connected parents of.
+   * @returns A set of the output-connected parents of the given block.
+   */
+  private getOutputParents(block: BlockSvg): Set<BlockSvg> {
+    const parents = new Set<BlockSvg>();
+    parents.add(block);
+    let parent = block.outputConnection?.targetBlock();
+    while (parent) {
+      parents.add(parent);
+      parent = parent.outputConnection?.targetBlock();
+    }
+
+    return parents;
+  }
+
   /**
    * Prepare for the deletion of a block by making a list of nodes we
    * could move the cursor to afterwards and save it to

core/keyboard_nav/marker.ts

@@ -13,6 +13,8 @@
 // Former goog.module ID: Blockly.Marker
 
 import {BlockSvg} from '../block_svg.js';
+import {TextInputBubble} from '../bubbles/textinput_bubble.js';
+import {CommentEditor} from '../comments/comment_editor.js';
 import {Field} from '../field.js';
 import {Icon} from '../icons/icon.js';
 import type {IFocusableNode} from '../interfaces/i_focusable_node.js';
@@ -69,6 +71,18 @@ export class Marker {
       return node.getSourceBlock();
     } else if (node instanceof Icon) {
       return node.getSourceBlock() as BlockSvg;
+    } else if (node instanceof TextInputBubble) {
+      const owner = node.getOwner();
+      if (owner instanceof BlockSvg) {
+        return owner;
+      } else if (owner) {
+        return this.getSourceBlockFromNode(owner);
+      }
+    } else if (node instanceof CommentEditor) {
+      const parent = node.getParent();
+      if (parent instanceof BlockSvg) {
+        return parent;
+      }
     }
 
     return null;

tests/mocha/cursor_test.js

@@ -163,13 +163,13 @@ suite('Cursor', function () {
       assert.equal(curNode, this.blocks.E);
     });
 
-    test('Out - From first connection loop to last next connection', function () {
+    test('Out - From previous connection loop to last input connection on block', function () {
       const prevConnection = this.blocks.A.previousConnection;
       const prevConnectionNode = prevConnection;
       this.cursor.setCurNode(prevConnectionNode);
       this.cursor.out();
       const curNode = this.cursor.getCurNode();
-      assert.equal(curNode, this.blocks.D.nextConnection);
+      assert.equal(curNode, this.blocks.A.getInput('NAME4').connection);
     });
   });
 
@@ -252,37 +252,38 @@ suite('Cursor', function () {
 
     test('In - from field in nested statement block to next nested statement block', function () {
       this.cursor.setCurNode(this.secondStatement.getField('NAME'));
-      this.cursor.in();
+      this.cursor.next();
       // Skip over the next connection
-      this.cursor.in();
+      this.cursor.next();
       const curNode = this.cursor.getCurNode();
       assert.equal(curNode, this.thirdStatement);
     });
+
     test('In - from field in nested statement block to next stack', function () {
       this.cursor.setCurNode(this.thirdStatement.getField('NAME'));
-      this.cursor.in();
+      this.cursor.next();
       // Skip over the next connection
-      this.cursor.in();
+      this.cursor.next();
       const curNode = this.cursor.getCurNode();
       assert.equal(curNode, this.multiStatement2);
     });
 
-    test('Out - from nested statement block to last field of previous nested statement block', function () {
+    test('Out - from nested statement block to previous nested statement block', function () {
       this.cursor.setCurNode(this.thirdStatement);
-      this.cursor.out();
+      this.cursor.prev();
       // Skip over the previous next connection
-      this.cursor.out();
+      this.cursor.prev();
       const curNode = this.cursor.getCurNode();
-      assert.equal(curNode, this.secondStatement.getField('NAME'));
+      assert.equal(curNode, this.secondStatement);
     });
 
-    test('Out - from root block to last field of last nested statement block in previous stack', function () {
+    test('Out - from root block to last nested statement block in previous stack', function () {
       this.cursor.setCurNode(this.multiStatement2);
-      this.cursor.out();
+      this.cursor.prev();
       // Skip over the previous next connection
-      this.cursor.out();
+      this.cursor.prev();
       const curNode = this.cursor.getCurNode();
-      assert.equal(curNode, this.thirdStatement.getField('NAME'));
+      assert.equal(curNode, this.thirdStatement);
     });
   });