From 38df7c87765acfeef8efd208059649f1af339cb7 Mon Sep 17 00:00:00 2001 From: Aaron Dodson Date: Wed, 28 May 2025 20:43:16 -0700 Subject: [PATCH] feat: Allow visiting empty input connections. (#9104) * feat: Update navigation policies to allow visiting empty input connections. * fix: Fix tests. * chore: Add JSDoc. * fix: Add missing import. * fix: Fix JSDoc. * chore: Remove double comments. --- core/keyboard_nav/block_navigation_policy.ts | 180 ++++++++++-------- .../connection_navigation_policy.ts | 42 +--- core/keyboard_nav/field_navigation_policy.ts | 44 +---- core/keyboard_nav/icon_navigation_policy.ts | 26 +-- tests/mocha/cursor_test.js | 4 +- tests/mocha/navigation_test.js | 78 ++++---- 6 files changed, 150 insertions(+), 224 deletions(-) diff --git a/core/keyboard_nav/block_navigation_policy.ts b/core/keyboard_nav/block_navigation_policy.ts index af7eadc09..570b06fe3 100644 --- a/core/keyboard_nav/block_navigation_policy.ts +++ b/core/keyboard_nav/block_navigation_policy.ts @@ -5,9 +5,12 @@ */ import {BlockSvg} from '../block_svg.js'; +import {ConnectionType} from '../connection_type.js'; import type {Field} from '../field.js'; +import type {Icon} from '../icons/icon.js'; import type {IFocusableNode} from '../interfaces/i_focusable_node.js'; import type {INavigationPolicy} from '../interfaces/i_navigation_policy.js'; +import {RenderedConnection} from '../rendered_connection.js'; import {WorkspaceSvg} from '../workspace_svg.js'; /** @@ -21,21 +24,8 @@ export class BlockNavigationPolicy implements INavigationPolicy { * @returns The first field or input of the given block, if any. */ getFirstChild(current: BlockSvg): IFocusableNode | null { - const icons = current.getIcons(); - if (icons.length) return icons[0]; - - for (const input of current.inputList) { - if (!input.isVisible()) { - continue; - } - for (const field of input.fieldRow) { - return field; - } - if (input.connection?.targetBlock()) - return input.connection.targetBlock() as BlockSvg; - } - - return null; + const candidates = getBlockNavigationCandidates(current); + return candidates[0]; } /** @@ -66,36 +56,10 @@ export class BlockNavigationPolicy implements INavigationPolicy { getNextSibling(current: BlockSvg): IFocusableNode | null { if (current.nextConnection?.targetBlock()) { return current.nextConnection?.targetBlock(); - } - - const parent = this.getParent(current); - let navigatingCrossStacks = false; - let siblings: (BlockSvg | Field)[] = []; - if (parent instanceof BlockSvg) { - for (let i = 0, input; (input = parent.inputList[i]); i++) { - if (!input.isVisible()) { - continue; - } - siblings.push(...input.fieldRow); - const child = input.connection?.targetBlock(); - if (child) { - siblings.push(child as BlockSvg); - } - } - } else if (parent instanceof WorkspaceSvg) { - siblings = parent.getTopBlocks(true); - navigatingCrossStacks = true; - } else { - return null; - } - - const currentIndex = siblings.indexOf( - navigatingCrossStacks ? current.getRootBlock() : current, - ); - if (currentIndex >= 0 && currentIndex < siblings.length - 1) { - return siblings[currentIndex + 1]; - } else if (currentIndex === siblings.length - 1 && navigatingCrossStacks) { - return siblings[0]; + } else if (current.outputConnection?.targetBlock()) { + return navigateBlock(current, 1); + } else if (this.getParent(current) instanceof WorkspaceSvg) { + return navigateStacks(current, 1); } return null; @@ -111,44 +75,13 @@ export class BlockNavigationPolicy implements INavigationPolicy { getPreviousSibling(current: BlockSvg): IFocusableNode | null { if (current.previousConnection?.targetBlock()) { return current.previousConnection?.targetBlock(); + } else if (current.outputConnection?.targetBlock()) { + return navigateBlock(current, -1); + } else if (this.getParent(current) instanceof WorkspaceSvg) { + return navigateStacks(current, -1); } - const parent = this.getParent(current); - let navigatingCrossStacks = false; - let siblings: (BlockSvg | Field)[] = []; - if (parent instanceof BlockSvg) { - for (let i = 0, input; (input = parent.inputList[i]); i++) { - if (!input.isVisible()) { - continue; - } - siblings.push(...input.fieldRow); - const child = input.connection?.targetBlock(); - if (child) { - siblings.push(child as BlockSvg); - } - } - } else if (parent instanceof WorkspaceSvg) { - siblings = parent.getTopBlocks(true); - navigatingCrossStacks = true; - } else { - return null; - } - - const currentIndex = siblings.indexOf(current); - let result: IFocusableNode | null = null; - if (currentIndex >= 1) { - result = siblings[currentIndex - 1]; - } else if (currentIndex === 0 && navigatingCrossStacks) { - result = siblings[siblings.length - 1]; - } - - // If navigating to a previous stack, our previous sibling is the last - // block in it. - if (navigatingCrossStacks && result instanceof BlockSvg) { - return result.lastConnectionInStack(false)?.getSourceBlock() ?? result; - } - - return result; + return null; } /** @@ -171,3 +104,88 @@ export class BlockNavigationPolicy implements INavigationPolicy { return current instanceof BlockSvg; } } + +/** + * Returns a list of the navigable children of the given block. + * + * @param block The block to retrieve the navigable children of. + * @returns A list of navigable/focusable children of the given block. + */ +function getBlockNavigationCandidates(block: BlockSvg): IFocusableNode[] { + const candidates: IFocusableNode[] = block.getIcons(); + + for (const input of block.inputList) { + if (!input.isVisible()) continue; + candidates.push(...input.fieldRow); + if (input.connection?.targetBlock()) { + candidates.push(input.connection.targetBlock() as BlockSvg); + } else if (input.connection?.type === ConnectionType.INPUT_VALUE) { + candidates.push(input.connection as RenderedConnection); + } + } + + return candidates; +} + +/** + * Returns the next/previous stack relative to the given block's stack. + * + * @param current The block whose stack will be navigated relative to. + * @param delta The difference in index to navigate; positive values navigate + * to the nth next stack, while negative values navigate to the nth previous + * stack. + * @returns The first block in the stack offset by `delta` relative to the + * current block's stack, or the last block in the stack offset by `delta` + * relative to the current block's stack when navigating backwards. + */ +export function navigateStacks(current: BlockSvg, delta: number) { + const stacks = current.workspace.getTopBlocks(true); + const currentIndex = stacks.indexOf(current.getRootBlock()); + const targetIndex = currentIndex + delta; + let result: BlockSvg | null = null; + if (targetIndex >= 0 && targetIndex < stacks.length) { + result = stacks[targetIndex]; + } else if (targetIndex < 0) { + result = stacks[stacks.length - 1]; + } else if (targetIndex >= stacks.length) { + result = stacks[0]; + } + + // When navigating to a previous stack, our previous sibling is the last + // block in it. + if (delta < 0 && result) { + return result.lastConnectionInStack(false)?.getSourceBlock() ?? result; + } + + return result; +} + +/** + * Returns the next navigable item relative to the provided block child. + * + * @param current The navigable block child item to navigate relative to. + * @param delta The difference in index to navigate; positive values navigate + * forward by n, while negative values navigate backwards by n. + * @returns The navigable block child offset by `delta` relative to `current`. + */ +export function navigateBlock( + current: Icon | Field | RenderedConnection | BlockSvg, + delta: number, +): IFocusableNode | null { + const block = + current instanceof BlockSvg + ? current.outputConnection.targetBlock() + : current.getSourceBlock(); + if (!(block instanceof BlockSvg)) return null; + + const candidates = getBlockNavigationCandidates(block); + const currentIndex = candidates.indexOf(current); + if (currentIndex === -1) return null; + + const targetIndex = currentIndex + delta; + if (targetIndex >= 0 && targetIndex < candidates.length) { + return candidates[targetIndex]; + } + + return null; +} diff --git a/core/keyboard_nav/connection_navigation_policy.ts b/core/keyboard_nav/connection_navigation_policy.ts index 9c3eafc56..bf685d063 100644 --- a/core/keyboard_nav/connection_navigation_policy.ts +++ b/core/keyboard_nav/connection_navigation_policy.ts @@ -9,6 +9,7 @@ import {ConnectionType} from '../connection_type.js'; import type {IFocusableNode} from '../interfaces/i_focusable_node.js'; import type {INavigationPolicy} from '../interfaces/i_navigation_policy.js'; import {RenderedConnection} from '../rendered_connection.js'; +import {navigateBlock} from './block_navigation_policy.js'; /** * Set of rules controlling keyboard navigation from a connection. @@ -37,17 +38,7 @@ export class ConnectionNavigationPolicy * @returns The given connection's parent connection or block. */ getParent(current: RenderedConnection): IFocusableNode | null { - if (current.type === ConnectionType.OUTPUT_VALUE) { - return current.targetConnection ?? current.getSourceBlock(); - } else if (current.getParentInput()) { - return current.getSourceBlock(); - } - - const topBlock = current.getSourceBlock().getTopStackBlock(); - return ( - (this.getParentConnection(topBlock)?.targetConnection?.getParentInput() - ?.connection as RenderedConnection) ?? topBlock - ); + return current.getSourceBlock(); } /** @@ -58,19 +49,7 @@ export class ConnectionNavigationPolicy */ getNextSibling(current: RenderedConnection): IFocusableNode | null { if (current.getParentInput()) { - const parentInput = current.getParentInput(); - const block = parentInput?.getSourceBlock(); - if (!block || !parentInput) return null; - - const curIdx = block.inputList.indexOf(parentInput); - for (let i = curIdx + 1; i < block.inputList.length; i++) { - const input = block.inputList[i]; - const fieldRow = input.fieldRow; - if (fieldRow.length) return fieldRow[0]; - if (input.connection) return input.connection as RenderedConnection; - } - - return null; + return navigateBlock(current, 1); } else if (current.type === ConnectionType.NEXT_STATEMENT) { const nextBlock = current.targetConnection; // If this connection is the last one in the stack, our next sibling is @@ -103,20 +82,7 @@ export class ConnectionNavigationPolicy */ getPreviousSibling(current: RenderedConnection): IFocusableNode | null { if (current.getParentInput()) { - const parentInput = current.getParentInput(); - const block = parentInput?.getSourceBlock(); - if (!block || !parentInput) return null; - - const curIdx = block.inputList.indexOf(parentInput); - for (let i = curIdx; i >= 0; i--) { - const input = block.inputList[i]; - if (input.connection && input !== parentInput) { - return input.connection as RenderedConnection; - } - const fieldRow = input.fieldRow; - if (fieldRow.length) return fieldRow[fieldRow.length - 1]; - } - return null; + return navigateBlock(current, -1); } else if ( current.type === ConnectionType.PREVIOUS_STATEMENT || current.type === ConnectionType.OUTPUT_VALUE diff --git a/core/keyboard_nav/field_navigation_policy.ts b/core/keyboard_nav/field_navigation_policy.ts index a1fa35723..f9df406c2 100644 --- a/core/keyboard_nav/field_navigation_policy.ts +++ b/core/keyboard_nav/field_navigation_policy.ts @@ -8,6 +8,7 @@ import type {BlockSvg} from '../block_svg.js'; import {Field} from '../field.js'; import type {IFocusableNode} from '../interfaces/i_focusable_node.js'; import type {INavigationPolicy} from '../interfaces/i_navigation_policy.js'; +import {navigateBlock} from './block_navigation_policy.js'; /** * Set of rules controlling keyboard navigation from a field. @@ -40,24 +41,7 @@ export class FieldNavigationPolicy implements INavigationPolicy> { * @returns The next field or input in the given field's block. */ getNextSibling(current: Field): IFocusableNode | null { - const input = current.getParentInput(); - const block = current.getSourceBlock(); - if (!block) return null; - - const curIdx = block.inputList.indexOf(input); - let fieldIdx = input.fieldRow.indexOf(current) + 1; - for (let i = curIdx; i < block.inputList.length; i++) { - const newInput = block.inputList[i]; - if (newInput.isVisible()) { - const fieldRow = newInput.fieldRow; - if (fieldIdx < fieldRow.length) return fieldRow[fieldIdx]; - if (newInput.connection?.targetBlock()) { - return newInput.connection.targetBlock() as BlockSvg; - } - } - fieldIdx = 0; - } - return null; + return navigateBlock(current, 1); } /** @@ -67,29 +51,7 @@ export class FieldNavigationPolicy implements INavigationPolicy> { * @returns The preceding field or input in the given field's block. */ getPreviousSibling(current: Field): IFocusableNode | null { - const parentInput = current.getParentInput(); - const block = current.getSourceBlock(); - if (!block) return null; - - const curIdx = block.inputList.indexOf(parentInput); - let fieldIdx = parentInput.fieldRow.indexOf(current) - 1; - for (let i = curIdx; i >= 0; i--) { - const input = block.inputList[i]; - if (input.isVisible()) { - if (input.connection?.targetBlock() && input !== parentInput) { - return input.connection.targetBlock() as BlockSvg; - } - const fieldRow = input.fieldRow; - if (fieldIdx > -1) return fieldRow[fieldIdx]; - } - // Reset the fieldIdx to the length of the field row of the previous - // input. - if (i - 1 >= 0) { - fieldIdx = block.inputList[i - 1].fieldRow.length - 1; - } - } - - return block.getIcons().pop() ?? null; + return navigateBlock(current, -1); } /** diff --git a/core/keyboard_nav/icon_navigation_policy.ts b/core/keyboard_nav/icon_navigation_policy.ts index 19d5a9c4d..96908cbbd 100644 --- a/core/keyboard_nav/icon_navigation_policy.ts +++ b/core/keyboard_nav/icon_navigation_policy.ts @@ -8,6 +8,7 @@ import {BlockSvg} from '../block_svg.js'; import {Icon} from '../icons/icon.js'; import type {IFocusableNode} from '../interfaces/i_focusable_node.js'; import type {INavigationPolicy} from '../interfaces/i_navigation_policy.js'; +import {navigateBlock} from './block_navigation_policy.js'; /** * Set of rules controlling keyboard navigation from an icon. @@ -40,21 +41,7 @@ export class IconNavigationPolicy implements INavigationPolicy { * @returns The next icon, field or input following this icon, if any. */ getNextSibling(current: Icon): IFocusableNode | null { - const block = current.getSourceBlock() as BlockSvg; - const icons = block.getIcons(); - const currentIndex = icons.indexOf(current); - if (currentIndex >= 0 && currentIndex + 1 < icons.length) { - return icons[currentIndex + 1]; - } - - for (const input of block.inputList) { - if (input.fieldRow.length) return input.fieldRow[0]; - - if (input.connection?.targetBlock()) - return input.connection.targetBlock() as BlockSvg; - } - - return null; + return navigateBlock(current, 1); } /** @@ -64,14 +51,7 @@ export class IconNavigationPolicy implements INavigationPolicy { * @returns The icon's previous icon, if any. */ getPreviousSibling(current: Icon): IFocusableNode | null { - const block = current.getSourceBlock() as BlockSvg; - const icons = block.getIcons(); - const currentIndex = icons.indexOf(current); - if (currentIndex >= 1) { - return icons[currentIndex - 1]; - } - - return null; + return navigateBlock(current, -1); } /** diff --git a/tests/mocha/cursor_test.js b/tests/mocha/cursor_test.js index aa4f56184..1d283f331 100644 --- a/tests/mocha/cursor_test.js +++ b/tests/mocha/cursor_test.js @@ -246,7 +246,7 @@ suite('Cursor', function () { }); test('getLastNode', function () { const node = this.cursor.getLastNode(); - assert.equal(node, this.blockA); + assert.equal(node, this.blockA.inputList[0].connection); }); }); suite('one c-hat block', function () { @@ -340,7 +340,7 @@ suite('Cursor', function () { test('getLastNode', function () { const node = this.cursor.getLastNode(); const blockB = this.workspace.getBlockById('B'); - assert.equal(node, blockB); + assert.equal(node, blockB.inputList[0].connection); }); }); diff --git a/tests/mocha/navigation_test.js b/tests/mocha/navigation_test.js index 14a9c8f63..5bed2aaab 100644 --- a/tests/mocha/navigation_test.js +++ b/tests/mocha/navigation_test.js @@ -72,6 +72,20 @@ suite('Navigation', function () { 'tooltip': '', 'helpUrl': '', }, + { + 'type': 'double_value_input', + 'message0': '%1 %2', + 'args0': [ + { + 'type': 'input_value', + 'name': 'NAME1', + }, + { + 'type': 'input_value', + 'name': 'NAME2', + }, + ], + }, ]); this.workspace = Blockly.inject('blocklyDiv', {}); this.navigator = this.workspace.getNavigator(); @@ -80,6 +94,7 @@ suite('Navigation', function () { const statementInput3 = this.workspace.newBlock('input_statement'); const statementInput4 = this.workspace.newBlock('input_statement'); const fieldWithOutput = this.workspace.newBlock('field_input'); + const doubleValueInput = this.workspace.newBlock('double_value_input'); const valueInput = this.workspace.newBlock('value_input'); statementInput1.nextConnection.connect(statementInput2.previousConnection); @@ -97,6 +112,7 @@ suite('Navigation', function () { statementInput4: statementInput4, fieldWithOutput: fieldWithOutput, valueInput: valueInput, + doubleValueInput, }; }); teardown(function () { @@ -431,16 +447,9 @@ suite('Navigation', function () { assert.equal(nextNode, prevConnection); }); test('fromInputToInput', function () { - const input = this.blocks.statementInput1.inputList[0]; + const input = this.blocks.doubleValueInput.inputList[0]; const inputConnection = - this.blocks.statementInput1.inputList[1].connection; - const nextNode = this.navigator.getNextSibling(input.connection); - assert.equal(nextNode, inputConnection); - }); - test('fromInputToStatementInput', function () { - const input = this.blocks.fieldAndInputs2.inputList[1]; - const inputConnection = - this.blocks.fieldAndInputs2.inputList[2].connection; + this.blocks.doubleValueInput.inputList[1].connection; const nextNode = this.navigator.getNextSibling(input.connection); assert.equal(nextNode, inputConnection); }); @@ -575,6 +584,11 @@ suite('Navigation', function () { assert.equal(prevNode, this.blocks.statementInput1); }); test('fromInputToField', function () { + // Disconnect the block that was connected to the input we're testing, + // because we only navigate to/from empty input connections (if they're + // connected navigation targets the connected block, bypassing the + // connection). + this.blocks.fieldWithOutput.outputConnection.disconnect(); const input = this.blocks.statementInput1.inputList[0]; const prevNode = this.navigator.getPreviousSibling(input.connection); assert.equal(prevNode, input.fieldRow[1]); @@ -585,9 +599,9 @@ suite('Navigation', function () { assert.isNull(prevNode); }); test('fromInputToInput', function () { - const input = this.blocks.fieldAndInputs2.inputList[2]; + const input = this.blocks.doubleValueInput.inputList[1]; const inputConnection = - this.blocks.fieldAndInputs2.inputList[1].connection; + this.blocks.doubleValueInput.inputList[0].connection; const prevNode = this.navigator.getPreviousSibling(input.connection); assert.equal(prevNode, inputConnection); }); @@ -711,10 +725,10 @@ suite('Navigation', function () { const inNode = this.navigator.getFirstChild(input.connection); assert.equal(inNode, previousConnection); }); - test('fromBlockToField', function () { - const field = this.blocks.valueInput.getField('NAME'); + test('fromBlockToInput', function () { + const connection = this.blocks.valueInput.inputList[0].connection; const inNode = this.navigator.getFirstChild(this.blocks.valueInput); - assert.equal(inNode, field); + assert.equal(inNode, connection); }); test('fromBlockToField', function () { const inNode = this.navigator.getFirstChild( @@ -731,7 +745,10 @@ suite('Navigation', function () { const inNode = this.navigator.getFirstChild( this.blocks.dummyInputValue, ); - assert.equal(inNode, null); + assert.equal( + inNode, + this.blocks.dummyInputValue.inputList[1].connection, + ); }); test('fromOuputToNull', function () { const output = this.blocks.fieldWithOutput.outputConnection; @@ -787,13 +804,10 @@ suite('Navigation', function () { const outNode = this.navigator.getParent(input.connection); assert.equal(outNode, this.blocks.statementInput1); }); - test('fromOutputToInput', function () { + test('fromOutputToBlock', function () { const output = this.blocks.fieldWithOutput.outputConnection; const outNode = this.navigator.getParent(output); - assert.equal( - outNode, - this.blocks.statementInput1.inputList[0].connection, - ); + assert.equal(outNode, this.blocks.fieldWithOutput); }); test('fromOutputToBlock', function () { const output = this.blocks.fieldWithOutput2.outputConnection; @@ -805,43 +819,29 @@ suite('Navigation', function () { const outNode = this.navigator.getParent(field); assert.equal(outNode, this.blocks.statementInput1); }); - test('fromPreviousToInput', function () { - const previous = this.blocks.statementInput3.previousConnection; - const inputConnection = - this.blocks.statementInput2.inputList[1].connection; - const outNode = this.navigator.getParent(previous); - assert.equal(outNode, inputConnection); - }); test('fromPreviousToBlock', function () { const previous = this.blocks.statementInput2.previousConnection; const outNode = this.navigator.getParent(previous); - assert.equal(outNode, this.blocks.statementInput1); - }); - test('fromNextToInput', function () { - const next = this.blocks.statementInput3.nextConnection; - const inputConnection = - this.blocks.statementInput2.inputList[1].connection; - const outNode = this.navigator.getParent(next); - assert.equal(outNode, inputConnection); + assert.equal(outNode, this.blocks.statementInput2); }); test('fromNextToBlock', function () { const next = this.blocks.statementInput2.nextConnection; const outNode = this.navigator.getParent(next); - assert.equal(outNode, this.blocks.statementInput1); + assert.equal(outNode, this.blocks.statementInput2); }); test('fromNextToBlock_NoPreviousConnection', function () { const next = this.blocks.secondBlock.nextConnection; const outNode = this.navigator.getParent(next); - assert.equal(outNode, this.blocks.noPrevConnection); + assert.equal(outNode, this.blocks.secondBlock); }); /** * This is where there is a block with both an output connection and a * next connection attached to an input. */ - test('fromNextToInput_OutputAndPreviousConnection', function () { + test('fromNextToBlock_OutputAndPreviousConnection', function () { const next = this.blocks.outputNextBlock.nextConnection; const outNode = this.navigator.getParent(next); - assert.equal(outNode, this.blocks.secondBlock.inputList[0].connection); + assert.equal(outNode, this.blocks.outputNextBlock); }); test('fromBlockToWorkspace', function () { const outNode = this.navigator.getParent(this.blocks.statementInput2);