From 939bd66f0aaaee04b60f0a0fdbccaf92193e1432 Mon Sep 17 00:00:00 2001 From: Aaron Dodson Date: Mon, 6 Apr 2026 14:50:07 -0700 Subject: [PATCH 1/2] fix: Fix navigation between toolbox and flyout in all layouts --- .../navigators/flyout_navigator.ts | 105 ++++++++++++- .../core/keyboard_nav/navigators/navigator.ts | 12 +- .../navigators/toolbox_navigator.ts | 139 +++++++++++++++++- packages/blockly/core/shortcut_items.ts | 12 +- 4 files changed, 247 insertions(+), 21 deletions(-) diff --git a/packages/blockly/core/keyboard_nav/navigators/flyout_navigator.ts b/packages/blockly/core/keyboard_nav/navigators/flyout_navigator.ts index 9e2cd8bdcbf..a49d18c04b2 100644 --- a/packages/blockly/core/keyboard_nav/navigators/flyout_navigator.ts +++ b/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. */ diff --git a/packages/blockly/core/keyboard_nav/navigators/navigator.ts b/packages/blockly/core/keyboard_nav/navigators/navigator.ts index 1a66a965bb9..8621ed1e290 100644 --- a/packages/blockly/core/keyboard_nav/navigators/navigator.ts +++ b/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]; } /** diff --git a/packages/blockly/core/keyboard_nav/navigators/toolbox_navigator.ts b/packages/blockly/core/keyboard_nav/navigators/toolbox_navigator.ts index 8b7238cf5b9..542ecca0bfa 100644 --- a/packages/blockly/core/keyboard_nav/navigators/toolbox_navigator.ts +++ b/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; +} diff --git a/packages/blockly/core/shortcut_items.ts b/packages/blockly/core/shortcut_items.ts index 43d1a76521d..29120c0dcac 100644 --- a/packages/blockly/core/shortcut_items.ts +++ b/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() From 5bf61b439b0885731f22b0439a6271f91992d574 Mon Sep 17 00:00:00 2001 From: Aaron Dodson Date: Mon, 6 Apr 2026 14:50:16 -0700 Subject: [PATCH 2/2] test: Add tests --- .../tests/mocha/keyboard_navigation_test.js | 256 ++++++++++++++++++ 1 file changed, 256 insertions(+) diff --git a/packages/blockly/tests/mocha/keyboard_navigation_test.js b/packages/blockly/tests/mocha/keyboard_navigation_test.js index e3491dca54a..9d5614ca9c6 100644 --- a/packages/blockly/tests/mocha/keyboard_navigation_test.js +++ b/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], + ); + }); + }); + } +});