leave it up to user to terminate looping

packages/excalidraw/components/App.tsx

@@ -603,7 +603,7 @@ class App extends React.Component<AppProps, AppState> {
   private elementsPendingErasure: ElementsPendingErasure = new Set();
 
   public flowChartCreator: FlowChartCreator = new FlowChartCreator();
-  private flowChartNavigator: FlowChartNavigator = new FlowChartNavigator();
+  private flowChartNavigator: FlowChartNavigator = new FlowChartNavigator(this);
 
   hitLinkElement?: NonDeletedExcalidrawElement;
   lastPointerDownEvent: React.PointerEvent<HTMLElement> | null = null;
@@ -4143,51 +4143,10 @@ class App extends React.Component<AppProps, AppState> {
           if (selectedElements.length === 1 && arrowKeyPressed) {
             event.preventDefault();
 
-            const nextId = this.flowChartNavigator.exploreByDirection(
+            return this.flowChartNavigator.exploreByDirection(
               selectedElements[0],
-              this.scene.getNonDeletedElementsMap(),
               getLinkDirectionFromKey(event.key),
             );
-
-            if (nextId) {
-              this.setState((prevState) => ({
-                selectedElementIds: makeNextSelectedElementIds(
-                  {
-                    [nextId]: true,
-                  },
-                  prevState,
-                ),
-              }));
-
-              const nextNode = this.scene
-                .getNonDeletedElementsMap()
-                .get(nextId);
-
-              if (
-                nextNode &&
-                !isElementCompletelyInViewport(
-                  [nextNode],
-                  this.canvas.width / window.devicePixelRatio,
-                  this.canvas.height / window.devicePixelRatio,
-                  {
-                    offsetLeft: this.state.offsetLeft,
-                    offsetTop: this.state.offsetTop,
-                    scrollX: this.state.scrollX,
-                    scrollY: this.state.scrollY,
-                    zoom: this.state.zoom,
-                  },
-                  this.scene.getNonDeletedElementsMap(),
-                  this.getEditorUIOffsets(),
-                )
-              ) {
-                this.scrollToContent(nextNode, {
-                  animate: true,
-                  duration: 300,
-                  canvasOffsets: this.getEditorUIOffsets(),
-                });
-              }
-            }
-            return;
           }
         }
       }

packages/excalidraw/element/flowchart.test.tsx

@@ -310,95 +310,4 @@ describe("flow chart navigation", () => {
     Keyboard.keyUp(KEYS.ALT);
     expect(h.state.selectedElementIds[rectangle.id]).toBe(true);
   });
-
-  it("take the most obvious link when possible", () => {
-    /**
-     * ▨ → ▨   ▨ → ▨
-     *     ↓   ↑
-     *     ▨ → ▨
-     */
-
-    API.clearSelection();
-    const rectangle = API.createElement({
-      type: "rectangle",
-      width: 200,
-      height: 100,
-    });
-
-    API.setElements([rectangle]);
-    API.setSelectedElements([rectangle]);
-
-    Keyboard.withModifierKeys({ ctrl: true }, () => {
-      Keyboard.keyPress(KEYS.ARROW_RIGHT);
-    });
-    Keyboard.keyUp(KEYS.CTRL_OR_CMD);
-
-    Keyboard.withModifierKeys({ ctrl: true }, () => {
-      Keyboard.keyPress(KEYS.ARROW_DOWN);
-    });
-    Keyboard.keyUp(KEYS.CTRL_OR_CMD);
-
-    Keyboard.withModifierKeys({ ctrl: true }, () => {
-      Keyboard.keyPress(KEYS.ARROW_RIGHT);
-    });
-    Keyboard.keyUp(KEYS.CTRL_OR_CMD);
-
-    Keyboard.withModifierKeys({ ctrl: true }, () => {
-      Keyboard.keyPress(KEYS.ARROW_UP);
-    });
-    Keyboard.keyUp(KEYS.CTRL_OR_CMD);
-
-    Keyboard.withModifierKeys({ ctrl: true }, () => {
-      Keyboard.keyPress(KEYS.ARROW_RIGHT);
-    });
-    Keyboard.keyUp(KEYS.CTRL_OR_CMD);
-
-    // last node should be the one that's selected
-    const rightMostNode = h.elements[h.elements.length - 2];
-    expect(rightMostNode.type).toBe("rectangle");
-    expect(h.state.selectedElementIds[rightMostNode.id]).toBe(true);
-
-    Keyboard.withModifierKeys({ alt: true }, () => {
-      Keyboard.keyPress(KEYS.ARROW_LEFT);
-      Keyboard.keyPress(KEYS.ARROW_LEFT);
-      Keyboard.keyPress(KEYS.ARROW_LEFT);
-      Keyboard.keyPress(KEYS.ARROW_LEFT);
-      Keyboard.keyPress(KEYS.ARROW_LEFT);
-    });
-    Keyboard.keyUp(KEYS.ALT);
-
-    expect(h.state.selectedElementIds[rectangle.id]).toBe(true);
-
-    // going any direction takes us to the predecessor as well
-    const predecessorToRightMostNode = h.elements[h.elements.length - 4];
-    expect(predecessorToRightMostNode.type).toBe("rectangle");
-
-    API.setSelectedElements([rightMostNode]);
-    Keyboard.withModifierKeys({ alt: true }, () => {
-      Keyboard.keyPress(KEYS.ARROW_RIGHT);
-    });
-    Keyboard.keyUp(KEYS.ALT);
-    expect(h.state.selectedElementIds[rightMostNode.id]).not.toBe(true);
-    expect(h.state.selectedElementIds[predecessorToRightMostNode.id]).toBe(
-      true,
-    );
-    API.setSelectedElements([rightMostNode]);
-    Keyboard.withModifierKeys({ alt: true }, () => {
-      Keyboard.keyPress(KEYS.ARROW_UP);
-    });
-    Keyboard.keyUp(KEYS.ALT);
-    expect(h.state.selectedElementIds[rightMostNode.id]).not.toBe(true);
-    expect(h.state.selectedElementIds[predecessorToRightMostNode.id]).toBe(
-      true,
-    );
-    API.setSelectedElements([rightMostNode]);
-    Keyboard.withModifierKeys({ alt: true }, () => {
-      Keyboard.keyPress(KEYS.ARROW_DOWN);
-    });
-    Keyboard.keyUp(KEYS.ALT);
-    expect(h.state.selectedElementIds[rightMostNode.id]).not.toBe(true);
-    expect(h.state.selectedElementIds[predecessorToRightMostNode.id]).toBe(
-      true,
-    );
-  });
 });

packages/excalidraw/element/flowchart.ts

@@ -34,6 +34,9 @@ import { invariant, toBrandedType } from "../utils";
 import { pointFrom, type LocalPoint } from "../../math";
 import { aabbForElement } from "../shapes";
 import { updateElbowArrowPoints } from "./elbowArrow";
+import type App from "../components/App";
+import { makeNextSelectedElementIds } from "../scene/selection";
+import { isElementCompletelyInViewport } from "./sizeHelpers";
 
 type LinkDirection = "up" | "right" | "down" | "left";
 
@@ -491,62 +494,54 @@ const createBindingArrow = (
 
 export class FlowChartNavigator {
   isExploring: boolean = false;
-  // nodes that are ONE link away (successor and predecessor both included)
-  private sameLevelNodes: ExcalidrawElement[] = [];
-  private sameLevelIndex: number = 0;
-  // set it to the opposite of the defalut creation direction
+
+  private app: App;
+  private siblingNodes: ExcalidrawElement[] = [];
+  private siblingIndex: number = 0;
   private direction: LinkDirection | null = null;
-  // for speedier navigation
-  private visitedNodes: Set<ExcalidrawElement["id"]> = new Set();
+
+  constructor(app: App) {
+    this.app = app;
+  }
 
   clear() {
     this.isExploring = false;
-    this.sameLevelNodes = [];
-    this.sameLevelIndex = 0;
+    this.siblingNodes = [];
+    this.siblingIndex = 0;
     this.direction = null;
-    this.visitedNodes.clear();
   }
 
-  exploreByDirection(
-    element: ExcalidrawElement,
-    elementsMap: ElementsMap,
-    direction: LinkDirection,
-  ): ExcalidrawElement["id"] | null {
+  /**
+   * Explore the flowchart by the given direction.
+   *
+   * The exploration follows a (near) breadth-first approach: when there're multiple
+   * nodes at the same level, we allow the user to traverse through them.
+   */
+  exploreByDirection(element: ExcalidrawElement, direction: LinkDirection) {
     if (!isBindableElement(element)) {
-      return null;
+      return;
     }
 
+    const elementsMap = this.app.scene.getNonDeletedElementsMap();
+
     // clear if going at a different direction
     if (direction !== this.direction) {
       this.clear();
     }
 
-    // add the current node to the visited
-    if (!this.visitedNodes.has(element.id)) {
-      this.visitedNodes.add(element.id);
-    }
-
     /**
-     * CASE:
-     * - already started exploring, AND
-     * - there are multiple nodes at the same level, AND
-     * - still going at the same direction, AND
-     *
-     * RESULT:
-     * - loop through nodes at the same level
-     *
-     * WHY:
-     * - provides user the capability to loop through nodes at the same level
+     * if we're already exploring (holding the alt key)
+     * and the direction is the same as the previous one
+     * and there're multiple nodes at the same level
+     * then we should traverse through them before moving to the next level
      */
     if (
       this.isExploring &&
       direction === this.direction &&
-      this.sameLevelNodes.length > 1
+      this.siblingNodes.length > 1
     ) {
-      this.sameLevelIndex =
-        (this.sameLevelIndex + 1) % this.sameLevelNodes.length;
-
-      return this.sameLevelNodes[this.sameLevelIndex].id;
+      this.siblingIndex = (this.siblingIndex + 1) % this.siblingNodes.length;
+      return this.goToNode(this.siblingNodes[this.siblingIndex].id);
     }
 
     const nodes = [
@@ -554,70 +549,52 @@ export class FlowChartNavigator {
       ...getPredecessors(element, elementsMap, direction),
     ];
 
-    /**
-     * CASE:
-     * - just started exploring at the given direction
-     *
-     * RESULT:
-     * - go to the first node in the given direction
-     */
     if (nodes.length > 0) {
-      this.sameLevelIndex = 0;
+      this.siblingIndex = 0;
       this.isExploring = true;
-      this.sameLevelNodes = nodes;
+      this.siblingNodes = nodes;
       this.direction = direction;
-      this.visitedNodes.add(nodes[0].id);
 
-      return nodes[0].id;
+      this.goToNode(nodes[0].id);
     }
-
-    /**
-     * CASE:
-     * - (just started exploring or still going at the same direction) OR
-     * - there're no nodes at the given direction
-     *
-     * RESULT:
-     * - go to some other unvisited linked node
-     *
-     * WHY:
-     * - provide a speedier navigation from a given node to some predecessor
-     *   without the user having to change arrow key
-     */
-    if (direction === this.direction || !this.isExploring) {
-      if (!this.isExploring) {
-        // just started and no other nodes at the given direction
-        // so the current node is technically the first visited node
-        // (this is needed so that we don't get stuck between looping through )
-        this.visitedNodes.add(element.id);
-      }
-
-      const otherDirections: LinkDirection[] = [
-        "up",
-        "right",
-        "down",
-        "left",
-      ].filter((dir): dir is LinkDirection => dir !== direction);
-
-      const otherLinkedNodes = otherDirections
-        .map((dir) => [
-          ...getSuccessors(element, elementsMap, dir),
-          ...getPredecessors(element, elementsMap, dir),
-        ])
-        .flat()
-        .filter((linkedNode) => !this.visitedNodes.has(linkedNode.id));
-
-      for (const linkedNode of otherLinkedNodes) {
-        if (!this.visitedNodes.has(linkedNode.id)) {
-          this.visitedNodes.add(linkedNode.id);
-          this.isExploring = true;
-          this.direction = direction;
-          return linkedNode.id;
-        }
-      }
-    }
-
-    return null;
   }
+
+  private goToNode = (nodeId: ExcalidrawElement["id"]) => {
+    this.app.setState((prevState) => ({
+      selectedElementIds: makeNextSelectedElementIds(
+        {
+          [nodeId]: true,
+        },
+        prevState,
+      ),
+    }));
+
+    const nextNode = this.app.scene.getNonDeletedElementsMap().get(nodeId);
+
+    if (
+      nextNode &&
+      !isElementCompletelyInViewport(
+        [nextNode],
+        this.app.canvas.width / window.devicePixelRatio,
+        this.app.canvas.height / window.devicePixelRatio,
+        {
+          offsetLeft: this.app.state.offsetLeft,
+          offsetTop: this.app.state.offsetTop,
+          scrollX: this.app.state.scrollX,
+          scrollY: this.app.state.scrollY,
+          zoom: this.app.state.zoom,
+        },
+        this.app.scene.getNonDeletedElementsMap(),
+        this.app.getEditorUIOffsets(),
+      )
+    ) {
+      this.app.scrollToContent(nextNode, {
+        animate: true,
+        duration: 300,
+        canvasOffsets: this.app.getEditorUIOffsets(),
+      });
+    }
+  };
 }
 
 export class FlowChartCreator {