fix: Fix navigation between toolbox and flyout in all layouts (#9681)

* fix: Fix navigation between toolbox and flyout in all layouts

* test: Add tests

packages/blockly/core/keyboard_nav/navigators/flyout_navigator.ts

@@ -6,9 +6,11 @@
 
 import {IFocusableNode} from '../../blockly.js';
 import type {IFlyout} from '../../interfaces/i_flyout.js';
+import {Position} from '../../utils/toolbox.js';
 import {FlyoutButtonNavigationPolicy} from '../navigation_policies/flyout_button_navigation_policy.js';
 import {FlyoutSeparatorNavigationPolicy} from '../navigation_policies/flyout_separator_navigation_policy.js';
 import {Navigator} from './navigator.js';
+import {getPhysicalToolboxPosition} from './toolbox_navigator.js';
 
 /**
  * Navigator that handles keyboard navigation within a flyout.
@@ -23,15 +25,114 @@ export class FlyoutNavigator extends Navigator {
   }
 
   /**
-   * Returns the toolbox when navigating to the left in a flyout.
+   * Returns the parent toolbox item or previous flyout item when navigating out
+   * (left arrow) from a flyout.
+   *
+   * @param node The flyout item to navigate relative to.
+   * @param bypassAdjustments True to skip adjusting navigation based on the
+   *     flyout's layout; false (default) to take it into account.
    */
-  override getOutNode(): IFocusableNode | null {
+  override getOutNode(
+    node?: IFocusableNode | null,
+    bypassAdjustments = false,
+  ): IFocusableNode | null {
+    if (!bypassAdjustments && this.flyout.targetWorkspace) {
+      const position = getPhysicalToolboxPosition(this.flyout.targetWorkspace);
+      switch (position) {
+        case Position.TOP:
+        case Position.BOTTOM:
+          return this.flyout.RTL
+            ? this.getNextNode(node, true)
+            : this.getPreviousNode(node, true);
+        case Position.RIGHT:
+          return null;
+      }
+    }
+
     const toolbox = this.flyout.targetWorkspace?.getToolbox();
     if (toolbox) return toolbox.getSelectedItem();
 
     return null;
   }
 
+  /**
+   * Returns the parent toolbox item or next flyout item when navigating in
+   * (right arrow) from a flyout.
+   *
+   * @param node The flyout item to navigate relative to.
+   * @param bypassAdjustments True to skip adjusting navigation based on the
+   *     flyout's layout; false (default) to take it into account.
+   */
+  override getInNode(
+    node?: IFocusableNode | null,
+    bypassAdjustments = false,
+  ): IFocusableNode | null {
+    if (!bypassAdjustments && this.flyout.targetWorkspace) {
+      const position = getPhysicalToolboxPosition(this.flyout.targetWorkspace);
+      switch (position) {
+        case Position.TOP:
+        case Position.BOTTOM:
+          return this.flyout.RTL
+            ? this.getPreviousNode(node, true)
+            : this.getNextNode(node, true);
+        case Position.RIGHT:
+          return this.getOutNode(node, true);
+      }
+    }
+
+    return super.getInNode(node);
+  }
+
+  /**
+   * Returns the parent toolbox item or next flyout item when navigating next
+   * (down arrow) from a flyout.
+   *
+   * @param node The flyout item to navigate relative to.
+   * @param bypassAdjustments True to skip adjusting navigation based on the
+   *     flyout's layout; false (default) to take it into account.
+   */
+  override getNextNode(
+    node?: IFocusableNode | null,
+    bypassAdjustments = false,
+  ): IFocusableNode | null {
+    if (!bypassAdjustments && this.flyout.targetWorkspace) {
+      const position = getPhysicalToolboxPosition(this.flyout.targetWorkspace);
+      switch (position) {
+        case Position.TOP:
+          return null;
+        case Position.BOTTOM:
+          return this.getOutNode(node, true);
+      }
+    }
+
+    return super.getNextNode(node);
+  }
+
+  /**
+   * Returns the parent toolbox item or previous flyout item when navigating
+   * previous (up arrow) from a flyout.
+   *
+   * @param node The flyout item to navigate relative to.
+   * @param bypassAdjustments True to skip adjusting navigation based on the
+   *     flyout's layout; false (default) to take it into account.
+   */
+  override getPreviousNode(
+    node?: IFocusableNode | null,
+    bypassAdjustments = false,
+  ): IFocusableNode | null {
+    if (!bypassAdjustments && this.flyout.targetWorkspace) {
+      const position = getPhysicalToolboxPosition(this.flyout.targetWorkspace);
+      switch (position) {
+        case Position.TOP:
+          return this.getOutNode(node, true);
+        case Position.BOTTOM:
+          return null;
+      }
+    }
+
+    return super.getPreviousNode(node);
+  }
+
   /**
    * Returns a list of top-level navigable flyout items.
    */

packages/blockly/core/keyboard_nav/navigators/navigator.ts

@@ -363,7 +363,7 @@ export class Navigator {
     const root = getFocusManager().getFocusedTree()?.getRootFocusableNode();
     if (!root) return null;
 
-    return this.getFirstChild(root);
+    return this.getTopLevelItems(root)[0];
   }
 
   /**
@@ -372,12 +372,10 @@ export class Navigator {
    * @returns The last navigable node on the workspace, or null.
    */
   getLastNode(): IFocusableNode | null {
-    const first = this.getFirstNode();
-    const oldLooping = this.getNavigationLoops();
-    this.setNavigationLoops(true);
-    const lastNode = this.getPreviousNode(first);
-    this.setNavigationLoops(oldLooping);
-    return lastNode;
+    const root = getFocusManager().getFocusedTree()?.getRootFocusableNode();
+    if (!root) return null;
+
+    return this.getTopLevelItems(root).slice(-1)[0];
   }
 
   /**

packages/blockly/core/keyboard_nav/navigators/toolbox_navigator.ts

@@ -8,6 +8,8 @@ import {getFocusManager} from '../../focus_manager.js';
 import type {IFocusableNode} from '../../interfaces/i_focusable_node.js';
 import {isSelectableToolboxItem} from '../../interfaces/i_selectable_toolbox_item.js';
 import type {IToolbox} from '../../interfaces/i_toolbox.js';
+import {Position} from '../../utils/toolbox.js';
+import type {WorkspaceSvg} from '../../workspace_svg.js';
 import {ToolboxItemNavigationPolicy} from '../navigation_policies/toolbox_item_navigation_policy.js';
 import {Navigator} from './navigator.js';
 
@@ -21,20 +23,118 @@ export class ToolboxNavigator extends Navigator {
   }
 
   /**
-   * Returns the flyout's first item when navigating to the right in a toolbox
-   * from a toolbox item that has a flyout.
+   * Returns the flyout's first item (if any) or next toolbox item when
+   * navigating in (right arrow) from a toolbox.
+   *
+   * @param node The toolbox item to navigate relative to.
+   * @param bypassAdjustments True to skip adjusting navigation based on the
+   *     toolbox's layout; false (default) to take it into account.
    */
   override getInNode(
-    current = getFocusManager().getFocusedNode(),
+    node = getFocusManager().getFocusedNode(),
+    bypassAdjustments = false,
   ): IFocusableNode | null {
-    if (isSelectableToolboxItem(current) && !current.getContents().length) {
+    const position = getPhysicalToolboxPosition(this.toolbox.getWorkspace());
+    if (!bypassAdjustments) {
+      switch (position) {
+        case Position.TOP:
+        case Position.BOTTOM:
+          return this.getNextNode(node, true);
+        case Position.RIGHT:
+          return this.getOutNode(node, true);
+      }
+    }
+
+    if (isSelectableToolboxItem(node) && !node.getContents().length) {
       return null;
     }
 
-    return (
-      this.toolbox.getFlyout()?.getWorkspace().getRestoredFocusableNode(null) ??
-      null
-    );
+    const flyoutNavigator = this.toolbox
+      .getFlyout()
+      ?.getWorkspace()
+      .getNavigator();
+    if (!flyoutNavigator) return null;
+
+    return this.toolbox.getWorkspace().RTL &&
+      (position === Position.TOP || position === Position.BOTTOM)
+      ? flyoutNavigator.getLastNode()
+      : flyoutNavigator.getFirstNode();
+  }
+
+  /**
+   * Returns the flyout's first item (if any) or previous toolbox item when
+   * navigating out (left arrow) from a toolbox.
+   *
+   * @param node The toolbox item to navigate relative to.
+   * @param bypassAdjustments True to skip adjusting navigation based on the
+   *     toolbox's layout; false (default) to take it into account.
+   */
+  override getOutNode(
+    node?: IFocusableNode | null,
+    bypassAdjustments = false,
+  ): IFocusableNode | null {
+    if (!bypassAdjustments) {
+      const position = getPhysicalToolboxPosition(this.toolbox.getWorkspace());
+      switch (position) {
+        case Position.TOP:
+        case Position.BOTTOM:
+          return this.getPreviousNode(node, true);
+        case Position.RIGHT:
+          return this.getInNode(node, true);
+      }
+    }
+
+    return super.getOutNode(node);
+  }
+
+  /**
+   * Returns the flyout's first item (if any) or next toolbox item when
+   * navigating next (down arrow) from a toolbox.
+   *
+   * @param node The toolbox item to navigate relative to.
+   * @param bypassAdjustments True to skip adjusting navigation based on the
+   *     toolbox's layout; false (default) to take it into account.
+   */
+  override getNextNode(
+    node?: IFocusableNode | null,
+    bypassAdjustments = false,
+  ): IFocusableNode | null {
+    if (!bypassAdjustments) {
+      const position = getPhysicalToolboxPosition(this.toolbox.getWorkspace());
+      switch (position) {
+        case Position.TOP:
+          return this.getInNode(node, true);
+        case Position.BOTTOM:
+          return this.getOutNode(node, true);
+      }
+    }
+
+    return super.getNextNode(node);
+  }
+
+  /**
+   * Returns the flyout's first item (if any) or previous toolbox item when
+   * navigating previous (up arrow) from a toolbox.
+   *
+   * @param node The toolbox item to navigate relative to.
+   * @param bypassAdjustments True to skip adjusting navigation based on the
+   *     toolbox's layout; false (default) to take it into account.
+   */
+  override getPreviousNode(
+    node?: IFocusableNode | null,
+    bypassAdjustments = false,
+  ): IFocusableNode | null {
+    if (!bypassAdjustments) {
+      const position = getPhysicalToolboxPosition(this.toolbox.getWorkspace());
+      switch (position) {
+        case Position.TOP:
+          return this.getOutNode(node, true);
+        case Position.BOTTOM:
+          return this.getInNode(node, true);
+      }
+    }
+
+    return super.getPreviousNode(node);
   }
 
   /**
@@ -44,3 +144,26 @@ export class ToolboxNavigator extends Navigator {
     return this.toolbox.getToolboxItems();
   }
 }
+
+/**
+ * Although developers specify the toolbox position as "start" or "end", this
+ * gets normalized by the injection options parser based on RTL, such that "end"
+ * in RTL means the left. When dealing with arrow keys, we want the actual/
+ * physical position on screen, not the logical position. This function converts
+ * the stored logical position to the physical position.
+ *
+ * @internal
+ * @param workspace The workspace to use injection options from.
+ * @returns The physical location of the toolbox/flyout on screen.
+ */
+export function getPhysicalToolboxPosition(workspace: WorkspaceSvg): Position {
+  const logicalPosition = workspace.options.toolboxPosition;
+  if (
+    workspace.options.RTL &&
+    !(logicalPosition === Position.TOP || logicalPosition === Position.BOTTOM)
+  ) {
+    return logicalPosition === Position.LEFT ? Position.RIGHT : Position.LEFT;
+  }
+
+  return logicalPosition;
+}

packages/blockly/core/shortcut_items.ts

@@ -570,7 +570,8 @@ export function registerArrowNavigation() {
     right: {
       name: names.NAVIGATE_RIGHT,
       preconditionFn: (workspace) => !workspace.isDragging(),
-      callback: (workspace) => {
+      callback: (workspace, e) => {
+        e.preventDefault();
         keyboardNavigationController.setIsActive(true);
         const node = workspace.RTL
           ? getFocusManager().getFocusedTree()?.getNavigator().getOutNode()
@@ -587,7 +588,8 @@ export function registerArrowNavigation() {
     left: {
       name: names.NAVIGATE_LEFT,
       preconditionFn: (workspace) => !workspace.isDragging(),
-      callback: (workspace) => {
+      callback: (workspace, e) => {
+        e.preventDefault();
         keyboardNavigationController.setIsActive(true);
         const node = workspace.RTL
           ? getFocusManager().getFocusedTree()?.getNavigator().getInNode()
@@ -604,7 +606,8 @@ export function registerArrowNavigation() {
     down: {
       name: names.NAVIGATE_DOWN,
       preconditionFn: (workspace) => !workspace.isDragging(),
-      callback: () => {
+      callback: (_workspace, e) => {
+        e.preventDefault();
         keyboardNavigationController.setIsActive(true);
         const node = getFocusManager()
           .getFocusedTree()
@@ -621,7 +624,8 @@ export function registerArrowNavigation() {
     up: {
       name: names.NAVIGATE_UP,
       preconditionFn: (workspace) => !workspace.isDragging(),
-      callback: () => {
+      callback: (_workspace, e) => {
+        e.preventDefault();
         keyboardNavigationController.setIsActive(true);
         const node = getFocusManager()
           .getFocusedTree()

packages/blockly/tests/mocha/keyboard_navigation_test.js

@@ -401,3 +401,259 @@ suite('Workspace comment navigation', function () {
     assert.equal(getFocusNodeId(), this.commentId1);
   });
 });
+
+const leftColumnNav = {
+  in: Blockly.utils.KeyCodes.RIGHT,
+  out: Blockly.utils.KeyCodes.LEFT,
+  nextItem: Blockly.utils.KeyCodes.DOWN,
+  previousItem: Blockly.utils.KeyCodes.UP,
+};
+
+const rightColumnNav = {
+  in: Blockly.utils.KeyCodes.LEFT,
+  out: Blockly.utils.KeyCodes.RIGHT,
+  nextItem: Blockly.utils.KeyCodes.DOWN,
+  previousItem: Blockly.utils.KeyCodes.UP,
+};
+
+/**
+ * All possible combinations of horizontal/vertical layout, LTR/RTL, and start/
+ * end toolbox/flyout positioning, along with the keycodes that should navigate
+ * in, out, and to the previous/next item in that layout configuration.
+ */
+const TOOLBOX_FLYOUT_LAYOUTS = [
+  {
+    id: 'Vertical Start LTR',
+    rtl: false,
+    horizontalLayout: false,
+    toolboxPosition: 'start',
+    ...leftColumnNav,
+  },
+  {
+    id: 'Vertical Start RTL',
+    rtl: true,
+    horizontalLayout: false,
+    toolboxPosition: 'start',
+    ...rightColumnNav,
+  },
+  {
+    id: 'Vertical End LTR',
+    rtl: false,
+    horizontalLayout: false,
+    toolboxPosition: 'end',
+    ...rightColumnNav,
+  },
+  {
+    id: 'Vertical End RTL',
+    rtl: true,
+    horizontalLayout: false,
+    toolboxPosition: 'end',
+    ...leftColumnNav,
+  },
+  {
+    id: 'Horizontal Start LTR',
+    rtl: false,
+    horizontalLayout: true,
+    toolboxPosition: 'start',
+    in: Blockly.utils.KeyCodes.DOWN,
+    out: Blockly.utils.KeyCodes.UP,
+    nextItem: Blockly.utils.KeyCodes.RIGHT,
+    previousItem: Blockly.utils.KeyCodes.LEFT,
+  },
+  {
+    id: 'Horizontal Start RTL',
+    rtl: true,
+    horizontalLayout: true,
+    toolboxPosition: 'start',
+    in: Blockly.utils.KeyCodes.DOWN,
+    out: Blockly.utils.KeyCodes.UP,
+    nextItem: Blockly.utils.KeyCodes.LEFT,
+    previousItem: Blockly.utils.KeyCodes.RIGHT,
+  },
+  {
+    id: 'Horizontal End LTR',
+    rtl: false,
+    horizontalLayout: true,
+    toolboxPosition: 'end',
+    in: Blockly.utils.KeyCodes.UP,
+    out: Blockly.utils.KeyCodes.DOWN,
+    nextItem: Blockly.utils.KeyCodes.RIGHT,
+    previousItem: Blockly.utils.KeyCodes.LEFT,
+  },
+  {
+    id: 'Horizontal End RTL',
+    rtl: true,
+    horizontalLayout: true,
+    toolboxPosition: 'end',
+    in: Blockly.utils.KeyCodes.UP,
+    out: Blockly.utils.KeyCodes.DOWN,
+    nextItem: Blockly.utils.KeyCodes.LEFT,
+    previousItem: Blockly.utils.KeyCodes.RIGHT,
+  },
+];
+
+suite('Toolbox and flyout arrow navigation by layout', function () {
+  for (const layout of TOOLBOX_FLYOUT_LAYOUTS) {
+    suite(layout.id, function () {
+      setup(function () {
+        sharedTestSetup.call(this);
+        Blockly.defineBlocksWithJsonArray([
+          {
+            type: 'basic_block',
+            message0: '%1',
+            args0: [
+              {
+                type: 'field_input',
+                name: 'TEXT',
+                text: 'default',
+              },
+            ],
+          },
+        ]);
+        const toolbox = document.getElementById('toolbox-categories');
+        this.workspace = Blockly.inject('blocklyDiv', {
+          toolbox,
+          rtl: layout.rtl,
+          horizontalLayout: layout.horizontalLayout,
+          toolboxPosition: layout.toolboxPosition,
+          renderer: 'zelos',
+        });
+        this.keys = layout;
+        this.firstToolboxItem = this.workspace
+          .getToolbox()
+          .getToolboxItems()[0];
+        this.lastToolboxItem = this.workspace.getToolbox().getToolboxItems()[1];
+      });
+
+      teardown(function () {
+        sharedTestTeardown.call(this);
+      });
+
+      test('Previous toolbox item from first is no-op', function () {
+        Blockly.getFocusManager().focusNode(this.firstToolboxItem);
+        pressKey(this.workspace, this.keys.previousItem);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.firstToolboxItem,
+        );
+      });
+
+      test('Previous toolbox item', function () {
+        Blockly.getFocusManager().focusNode(this.lastToolboxItem);
+        pressKey(this.workspace, this.keys.previousItem);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.firstToolboxItem,
+        );
+      });
+
+      test('Next toolbox item from last is no-op', function () {
+        Blockly.getFocusManager().focusNode(this.lastToolboxItem);
+        pressKey(this.workspace, this.keys.nextItem);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.lastToolboxItem,
+        );
+      });
+
+      test('Next toolbox item', function () {
+        Blockly.getFocusManager().focusNode(this.firstToolboxItem);
+        pressKey(this.workspace, this.keys.nextItem);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.lastToolboxItem,
+        );
+      });
+
+      test('Out from toolbox item is no-op', function () {
+        Blockly.getFocusManager().focusNode(this.firstToolboxItem);
+        pressKey(this.workspace, this.keys.out);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.firstToolboxItem,
+        );
+      });
+
+      test('In from toolbox item focuses first flyout item', function () {
+        Blockly.getFocusManager().focusNode(this.firstToolboxItem);
+        pressKey(this.workspace, this.keys.in);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[0],
+        );
+      });
+
+      test('Previous flyout item from first is no-op', function () {
+        pressKey(this.workspace, Blockly.utils.KeyCodes.T);
+        Blockly.getFocusManager().focusNode(
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[0],
+        );
+        pressKey(this.workspace, this.keys.previousItem);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[0],
+        );
+      });
+
+      test('Previous flyout item', function () {
+        pressKey(this.workspace, Blockly.utils.KeyCodes.T);
+        Blockly.getFocusManager().focusNode(
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[1],
+        );
+        pressKey(this.workspace, this.keys.previousItem);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[0],
+        );
+      });
+
+      test('Next flyout item from last is no-op', function () {
+        pressKey(this.workspace, Blockly.utils.KeyCodes.T);
+        Blockly.getFocusManager().focusNode(
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[1],
+        );
+        pressKey(this.workspace, this.keys.nextItem);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[1],
+        );
+      });
+
+      test('Next flyout item', function () {
+        pressKey(this.workspace, Blockly.utils.KeyCodes.T);
+        Blockly.getFocusManager().focusNode(
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[0],
+        );
+        pressKey(this.workspace, this.keys.nextItem);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[1],
+        );
+      });
+
+      test('Out from flyout item focuses toolbox item', function () {
+        pressKey(this.workspace, Blockly.utils.KeyCodes.T);
+        Blockly.getFocusManager().focusNode(
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[0],
+        );
+        pressKey(this.workspace, this.keys.out);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.firstToolboxItem,
+        );
+      });
+
+      test('In from flyout item is no-op', function () {
+        pressKey(this.workspace, Blockly.utils.KeyCodes.T);
+        Blockly.getFocusManager().focusNode(
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[0],
+        );
+        pressKey(this.workspace, this.keys.in);
+        assert.equal(
+          Blockly.getFocusManager().getFocusedNode(),
+          this.workspace.getFlyout().getWorkspace().getTopBlocks()[0],
+        );
+      });
+    });
+  }
+});