feat: Add flag to enable/disable cursor visiting stack nodes (#197)

* feat: Add options object for LineCursor

  Add a CursorOptions type and add options objects of that type
  to LineCursor and its constructor.

  The only available (but not yet implemented) option is
  stackConnections, defaulting to true.

* refactor: Ensure predicates can access this

  Use bind when passing isValidLineNode and isValidInlineNode
  as arguments to getNextNode and getPreviousNode, so that
  those methods can access this.options.

* refactor: Use protected instead of @Protected

* fix: have isValidInlineNode use isValidLineNode

  This ensures that all valid line nodes are also valid inline
  nodes, which was the intention (but had accidentally been
  broken by PR #162, and could easily be further broken
  by this PR without sufficient care).

* feat: Implement the stackConnections: false option

  Prevent stack connections, except for unconnected 'next'
  connections (and unconnected next statement inputs), from
  being visited if stackConnections option is false.

* feat: Plumb CursorOptions through KeyboardNavigation

  Add a NavigationOptions type and use it for an options object
  for the KeyboardNavigation constructor.  For now it just has
  a single property of type CursorOptions, but I anticipate
  adding flags for different test scenarios (e.g. which key opens
  context menu, on which nodes various options are presented).

  Will plumb this through the test code in the next RP.

Author: cpcallen

src/index.ts

@@ -6,7 +6,17 @@
 
 import * as Blockly from 'blockly/core';
 import {NavigationController} from './navigation_controller';
-import {LineCursor} from './line_cursor';
+import {CursorOptions, LineCursor} from './line_cursor';
+
+/** Options object for KeyboardNavigation instances. */
+export type NavigationOptions = {
+  cursor: Partial<CursorOptions>;
+};
+
+/** Default options for LineCursor instances. */
+const defaultOptions: NavigationOptions = {
+  cursor: {},
+};
 
 /** Plugin for keyboard navigation. */
 export class KeyboardNavigation {
@@ -39,9 +49,15 @@ export class KeyboardNavigation {
    * @param workspace The workspace that the plugin will
    *     be added to.
    */
-  constructor(workspace: Blockly.WorkspaceSvg) {
+  constructor(
+    workspace: Blockly.WorkspaceSvg,
+    options: Partial<NavigationOptions>,
+  ) {
     this.workspace = workspace;
 
+    // Regularise options and apply defaults.
+    options = {...defaultOptions, ...options};
+
     this.navigationController = new NavigationController();
     this.navigationController.init();
     this.navigationController.addWorkspace(workspace);
@@ -51,7 +67,7 @@ export class KeyboardNavigation {
     this.originalTheme = workspace.getTheme();
     this.setGlowTheme();
 
-    this.cursor = new LineCursor(workspace);
+    this.cursor = new LineCursor(workspace, options.cursor);
     this.cursor.install();
 
     // Ensure that only the root SVG G (group) has a tab index.

src/line_cursor.ts

@@ -16,12 +16,29 @@
 import * as Blockly from 'blockly/core';
 import {ASTNode, Marker} from 'blockly/core';
 
+/** Options object for LineCursor instances. */
+export type CursorOptions = {
+  /**
+   * Can the cursor visit all stack connections (next/previous), or
+   * (if false) only unconnected next connections?
+   */
+  stackConnections: boolean;
+};
+
+/** Default options for LineCursor instances. */
+const defaultOptions: CursorOptions = {
+  stackConnections: true,
+};
+
 /**
  * Class for a line cursor.
  */
 export class LineCursor extends Marker {
   override type = 'cursor';
 
+  /** Options for this line cursor. */
+  private readonly options: CursorOptions;
+
   /** Has the cursor been installed in a workspace's marker manager? */
   private installed = false;
 
@@ -31,10 +48,15 @@ export class LineCursor extends Marker {
   /**
    * @param workspace The workspace this cursor belongs to.
    */
-  constructor(public readonly workspace: Blockly.WorkspaceSvg) {
+  constructor(
+    public readonly workspace: Blockly.WorkspaceSvg,
+    options?: Partial<CursorOptions>,
+  ) {
     super();
     // Bind selectListener to facilitate future install/uninstall.
     this.selectListener = this.selectListener.bind(this);
+    // Regularise options and apply defaults.
+    this.options = {...defaultOptions, ...options};
   }
 
   /**
@@ -79,7 +101,7 @@ export class LineCursor extends Marker {
     if (!curNode) {
       return null;
     }
-    let newNode = this.getNextNode(curNode, this.validLineNode);
+    let newNode = this.getNextNode(curNode, this.validLineNode.bind(this));
 
     if (newNode) {
       this.setCurNode(newNode);
@@ -99,7 +121,7 @@ export class LineCursor extends Marker {
     if (!curNode) {
       return null;
     }
-    const newNode = this.getNextNode(curNode, this.validInLineNode);
+    const newNode = this.getNextNode(curNode, this.validInLineNode.bind(this));
 
     if (newNode) {
       this.setCurNode(newNode);
@@ -118,7 +140,7 @@ export class LineCursor extends Marker {
     if (!curNode) {
       return null;
     }
-    let newNode = this.getPreviousNode(curNode, this.validLineNode);
+    let newNode = this.getPreviousNode(curNode, this.validLineNode.bind(this));
 
     if (newNode) {
       this.setCurNode(newNode);
@@ -138,7 +160,10 @@ export class LineCursor extends Marker {
     if (!curNode) {
       return null;
     }
-    const newNode = this.getPreviousNode(curNode, this.validInLineNode);
+    const newNode = this.getPreviousNode(
+      curNode,
+      this.validInLineNode.bind(this),
+    );
 
     if (newNode) {
       this.setCurNode(newNode);
@@ -158,11 +183,18 @@ export class LineCursor extends Marker {
    *   This is to facilitate connecting additional blocks to a
    *   stack/substack.
    *
+   * If options.stackConnections is true (the default) then allow the
+   * cursor to visit all (useful) stack connection by additionally
+   * returning true for:
+   *
+   *   - Any next statement input
+   *   - Any 'next' connection.
+   *   - An unconnected previous statement input.
+   *
    * @param node The AST node to check.
    * @returns True if the node should be visited, false otherwise.
-   * @protected
    */
-  validLineNode(node: ASTNode | null): boolean {
+  protected validLineNode(node: ASTNode | null): boolean {
     if (!node) return false;
     const location = node.getLocation();
     const type = node && node.getType();
@@ -171,38 +203,47 @@ export class LineCursor extends Marker {
         return !(location as Blockly.Block).outputConnection?.isConnected();
       case ASTNode.types.INPUT:
         const connection = location as Blockly.Connection;
-        return connection.type === Blockly.NEXT_STATEMENT;
+        return (
+          connection.type === Blockly.NEXT_STATEMENT &&
+          (this.options.stackConnections || !connection.isConnected())
+        );
       case ASTNode.types.NEXT:
-        return true;
+        return (
+          this.options.stackConnections ||
+          !(location as Blockly.Connection).isConnected()
+        );
       case ASTNode.types.PREVIOUS:
-        return !(location as Blockly.Connection).isConnected();
+        return (
+          this.options.stackConnections &&
+          !(location as Blockly.Connection).isConnected()
+        );
       default:
         return false;
     }
   }
 
   /**
    * Returns true iff the given node can be visited by the cursor when
-   * using the left/right arrow keys.  Specifically, if the node is for:
+   * using the left/right arrow keys.  Specifically, if the node is
+   * for any node for which valideLineNode would return true, plus:
    *
    * - Any block.
-   * - Any field.
+   * - Any field that is not a full block field.
    * - Any unconnected next or input connection.  This is to
    *   facilitate connecting additional blocks.
    *
    * @param node The AST node to check whether it is valid.
    * @returns True if the node should be visited, false otherwise.
-   * @protected
    */
-  validInLineNode(node: ASTNode | null): boolean {
+  protected validInLineNode(node: ASTNode | null): boolean {
     if (!node) return false;
+    if (this.validLineNode(node)) return true;
     const location = node.getLocation();
     const type = node && node.getType();
     switch (type) {
       case ASTNode.types.BLOCK:
         return true;
       case ASTNode.types.INPUT:
-      case ASTNode.types.NEXT:
         return !(location as Blockly.Connection).isConnected();
       case ASTNode.types.FIELD:
         // @ts-expect-error isFullBlockField is a protected method.