diff --git a/packages/blockly/core/dragging/block_drag_strategy.ts b/packages/blockly/core/dragging/block_drag_strategy.ts index 39dff646ec2..59245737271 100644 --- a/packages/blockly/core/dragging/block_drag_strategy.ts +++ b/packages/blockly/core/dragging/block_drag_strategy.ts @@ -31,14 +31,16 @@ import * as dom from '../utils/dom.js'; import * as svgMath from '../utils/svg_math.js'; import type {WorkspaceSvg} from '../workspace_svg.js'; -/** Represents a nearby valid connection. */ -interface ConnectionCandidate { +/** Represents a valid pair of connections between the dragging block and a block on the workspace. */ +interface ConnectionPair { /** A connection on the dragging stack that is compatible with neighbour. */ local: RenderedConnection; - /** A nearby connection that is compatible with local. */ neighbour: RenderedConnection; +} +/** Represents a nearby valid connection. */ +interface ConnectionCandidate extends ConnectionPair { /** The distance between the local connection and the neighbour connection. */ distance: number; } @@ -73,11 +75,8 @@ export class BlockDragStrategy implements IDragStrategy { private dragging = false; - /** Where a constrained movement should start when traversing the tree. */ - private searchNode: RenderedConnection | null = null; - /** List of all connections available on the workspace. */ - private allConnections: RenderedConnection[] = []; + private allConnectionPairs: ConnectionPair[] = []; /** The current movement mode. */ private moveMode = MoveMode.UNCONSTRAINED; @@ -187,6 +186,7 @@ export class BlockDragStrategy implements IDragStrategy { if (this.shouldDisconnect(healStack)) { this.disconnectBlock(healStack); } + this.block.setDragging(true); this.workspace.getLayerManager()?.moveToDragLayer(this.block); this.getVisibleBubbles(this.block).forEach((bubble) => { @@ -196,13 +196,10 @@ export class BlockDragStrategy implements IDragStrategy { // For keyboard-driven moves, cache a list of valid connection points for // use in constrained moved mode. if (e instanceof KeyboardEvent) { - for (const topBlock of this.block.workspace.getTopBlocks(true)) { - this.allConnections.push(...this.getAllConnections(topBlock)); - } + this.cacheAllConnectionPairs(); // Scooch the block to be offset from the connection preview indicator. this.block.moveDuringDrag(this.startLoc); - this.connectionCandidate = this.createInitialCandidate(); const neighbour = this.updateConnectionPreview( this.block, new Coordinate(0, 0), @@ -228,6 +225,33 @@ export class BlockDragStrategy implements IDragStrategy { return this.block; } + /** + * Handles any setup for starting the drag, including disconnecting the block + * from any parent blocks. + */ + private cacheAllConnectionPairs() { + const connectionChecker = this.block.workspace.connectionChecker; + const workspaceConns = []; + this.allConnectionPairs = []; + const localConns = this.getLocalConnections(this.block); + for (const topBlock of this.block.workspace.getTopBlocks(true)) { + workspaceConns.push(...this.getAllConnections(topBlock)); + } + for (const neighbour of workspaceConns) { + for (const local of localConns) { + if ( + connectionChecker.canConnect(local, neighbour, true, Infinity) && + !neighbour.targetBlock()?.isInsertionMarker() + ) { + this.allConnectionPairs.push({ + local, + neighbour, + }); + } + } + } + } + /** * Returns an array of visible bubbles attached to the given block or its * descendants. @@ -297,17 +321,56 @@ export class BlockDragStrategy implements IDragStrategy { * @param healStack Whether or not to heal the stack after disconnecting. */ private disconnectBlock(healStack: boolean) { - this.startParentConn = - this.block.outputConnection?.targetConnection ?? - this.block.previousConnection?.targetConnection; - if (healStack) { - this.startChildConn = this.block.nextConnection?.targetConnection; - } - + this.storeInitialConnections(healStack); this.block.unplug(healStack); blockAnimation.disconnectUiEffect(this.block); } + /** + * Stores the dragging block's current parent or child connection before + * unplugging. This allows us to revert the drag cleanly. In keyboard move mode, + * the initial connection pair is also used as the first connection candidate. + */ + private storeInitialConnections(healStack: boolean) { + // Prioritze the block's parent connection (output or previous) if one exists. + let localParentConn: RenderedConnection | null = null; + let parentTargetConn: RenderedConnection | null = null; + + if (this.block.outputConnection?.isConnected()) { + localParentConn = this.block.outputConnection; + parentTargetConn = this.block.outputConnection.targetConnection; + } else if (this.block.previousConnection?.isConnected()) { + localParentConn = this.block.previousConnection; + parentTargetConn = this.block.previousConnection.targetConnection; + } + + this.startParentConn = parentTargetConn; + if (localParentConn && parentTargetConn) { + this.connectionCandidate = { + local: localParentConn, + neighbour: parentTargetConn, + distance: 0, + }; + } else { + // If there is no parent connection and we are moving a single block, + // use the next connection. + if (healStack) { + const localNextConn = this.block.nextConnection; + const nextTargetConn = localNextConn?.targetConnection; + + if (localNextConn && nextTargetConn) { + this.connectionCandidate = { + local: localNextConn, + neighbour: nextTargetConn, + distance: 0, + }; + } + + this.startChildConn = nextTargetConn; + } + } + } + /** Fire a UI event at the start of a block drag. */ private fireDragStartEvent() { const event = new (eventUtils.get(EventType.BLOCK_DRAG))( @@ -357,27 +420,41 @@ export class BlockDragStrategy implements IDragStrategy { // Handle the case where the drag has reached a possible connection. if (this.connectionCandidate) { - const neighbour = this.connectionCandidate.neighbour; - // The next constrained move will resume the search from the current - // candidate location. - this.searchNode = neighbour; if (this.moveMode === MoveMode.CONSTRAINED) { - // Position the moving block down and slightly to the right of the - // target connection. - this.block.moveDuringDrag( - new Coordinate( - neighbour.x + this.BLOCK_CONNECTION_OFFSET, - neighbour.y + this.BLOCK_CONNECTION_OFFSET, - ), - ); + const {local, neighbour} = this.connectionCandidate; + + const dx = neighbour.x - local.x; + const dy = neighbour.y - local.y; + + // Base aligned position + let x = this.startLoc!.x + dx; + let y = this.startLoc!.y + dy; + + // Decide offset direction + const becomingChild = + local.type === ConnectionType.PREVIOUS_STATEMENT || + local.type === ConnectionType.OUTPUT_VALUE; + + const offset = this.BLOCK_CONNECTION_OFFSET; + + // An offset is used to distinguish the block from insertion marker, + // while keeping the connection point visible. The offset direction + // changes based on the parent/child relationship of the blocks + // being connected. + if (becomingChild) { + x += offset; + y += offset; + } else { + x -= offset; + y -= offset; + } + + this.block.moveDuringDrag(new Coordinate(x, y)); } } else { // No connection was available or adequately close to the dragged block; - // clear out the search node since we have nowhere to search from, and // suggest using unconstrained mode to arbitrarily position the block if // we're in keyboard-driven constrained mode. - this.searchNode = null; - if (this.moveMode === MoveMode.CONSTRAINED) { showUnconstrainedMoveHint(this.workspace, true); } @@ -398,7 +475,8 @@ export class BlockDragStrategy implements IDragStrategy { delta: Coordinate, ): RenderedConnection | undefined { const currCandidate = this.connectionCandidate; - const newCandidate = this.getConnectionCandidate(draggingBlock, delta); + const newCandidate = this.getConnectionCandidate(delta); + if (!newCandidate) { this.connectionPreviewer?.hidePreview(); this.connectionCandidate = null; @@ -488,21 +566,13 @@ export class BlockDragStrategy implements IDragStrategy { * compatible type (input, output, etc) and connection check. */ private getConnectionCandidate( - draggingBlock: BlockSvg, delta: Coordinate, ): ConnectionCandidate | null { - const localConns = this.getLocalConnections(draggingBlock); - let candidate = null; - if (this.moveMode === MoveMode.CONSTRAINED) { const direction = this.getDirectionToNewLocation( Coordinate.sum(this.startLoc!, delta), ); - candidate = this.findTraversalCandidate( - draggingBlock, - localConns, - direction, - ); + const candidate = this.findTraversalCandidate(direction); if (candidate) { return candidate; } @@ -510,7 +580,10 @@ export class BlockDragStrategy implements IDragStrategy { delta = new Coordinate(0, 0); } + // If we do not have a candidate yet, we fallback to the closest one nearby. let radius = this.getSearchRadius(); + const localConns = this.getLocalConnections(this.block); + let candidate = null; for (const conn of localConns) { const {connection: neighbour, radius: rad} = conn.closest(radius, delta); @@ -550,7 +623,25 @@ export class BlockDragStrategy implements IDragStrategy { if (lastOnStack && lastOnStack !== draggingBlock.nextConnection) { available.push(lastOnStack); } - return available; + + // Reversing the order of input connections provides a more natural traversal + // experience. With each move right/down, the dragging block should move in + // one of those directions (except when wrapping to the other end of the list). + const nonInputConnections = [ + draggingBlock.outputConnection, + draggingBlock.previousConnection, + draggingBlock.nextConnection, + ].filter(Boolean); // Removes falsy (null) values. + const inputConnections: RenderedConnection[] = []; + + for (const conn of available) { + if (!nonInputConnections.includes(conn)) { + inputConnections.push(conn); + } + } + inputConnections.reverse(); + + return [...nonInputConnections, ...inputConnections]; } /** @@ -599,7 +690,7 @@ export class BlockDragStrategy implements IDragStrategy { this.block.queueRender().then(() => this.disposeStep()); } - this.allConnections = []; + this.allConnectionPairs = []; } /** Disposes of any state at the end of the drag. */ @@ -679,97 +770,31 @@ export class BlockDragStrategy implements IDragStrategy { /** * Get the nearest valid candidate connection in traversal order. * - * @param draggingBlock The root block being dragged. - * @param localConns The list of connections on the dragging block(s) that are - * available to connect to. * @param direction The cardinal direction in which the block is being moved. * @returns A candidate connection and radius, or null if none was found. */ - findTraversalCandidate( - draggingBlock: BlockSvg, - localConns: RenderedConnection[], - direction: Direction, - ): ConnectionCandidate | null { - const connectionChecker = draggingBlock.workspace.connectionChecker; - let candidateConnection: ConnectionCandidate | null = null; - let potential: RenderedConnection | null = this.searchNode; - - while (potential && !candidateConnection) { - const potentialIndex = this.allConnections.indexOf(potential); + findTraversalCandidate(direction: Direction): ConnectionCandidate | null { + const currentPairIndex = this.allConnectionPairs.findIndex( + (pair) => + this.connectionCandidate?.local === pair.local && + this.connectionCandidate?.neighbour === pair.neighbour, + ); + if (currentPairIndex !== -1) { if (direction === Direction.UP || direction === Direction.LEFT) { - potential = - this.allConnections[potentialIndex - 1] ?? - this.allConnections[this.allConnections.length - 1]; + const nextPair = + this.allConnectionPairs[currentPairIndex - 1] ?? + this.allConnectionPairs[this.allConnectionPairs.length - 1]; + return {...nextPair, distance: 0}; } else if ( direction === Direction.DOWN || direction === Direction.RIGHT ) { - potential = - this.allConnections[potentialIndex + 1] ?? this.allConnections[0]; + const nextPair = + this.allConnectionPairs[currentPairIndex + 1] ?? + this.allConnectionPairs[0]; + return {...nextPair, distance: 0}; } - - localConns.forEach((conn: RenderedConnection) => { - if ( - potential && - connectionChecker.canConnect(conn, potential, true, Infinity) && - !potential.targetBlock()?.isInsertionMarker() - ) { - candidateConnection = { - local: conn, - neighbour: potential, - distance: 0, - }; - } - }); - if (potential == this.searchNode) break; } - return candidateConnection; - } - - /** - * Create a candidate representing where the block was previously connected. - * Used to render the block position after picking up the block but before - * moving during a drag. - * - * @returns A connection candidate representing where the block was at the - * start of the drag. - */ - private createInitialCandidate(): ConnectionCandidate | null { - this.searchNode = this.startParentConn ?? this.startChildConn; - - switch (this.searchNode?.type) { - case ConnectionType.INPUT_VALUE: { - if (this.block.outputConnection) { - return { - neighbour: this.searchNode, - local: this.block.outputConnection, - distance: 0, - }; - } - break; - } - case ConnectionType.NEXT_STATEMENT: { - if (this.block.previousConnection) { - return { - neighbour: this.searchNode, - local: this.block.previousConnection, - distance: 0, - }; - } - break; - } - case ConnectionType.PREVIOUS_STATEMENT: { - if (this.block.nextConnection) { - return { - neighbour: this.searchNode, - local: this.block.nextConnection, - distance: 0, - }; - } - break; - } - } - return null; } diff --git a/packages/blockly/tests/mocha/keyboard_movement_test.js b/packages/blockly/tests/mocha/keyboard_movement_test.js index 9d6ac0cdddf..cd9ffc3f1cc 100644 --- a/packages/blockly/tests/mocha/keyboard_movement_test.js +++ b/packages/blockly/tests/mocha/keyboard_movement_test.js @@ -638,14 +638,10 @@ suite('Keyboard-driven movement', function () { * pressing right (or down) arrow n times. */ const EXPECTED_COMPLEX_RIGHT = [ - // TODO(#702): With the current behavior, certain connection - // candidates that can be found using the mouse are not visited when - // doing a keyboard move. They appear in the list below, but commented - // out for now. They should be uncommented if the behavior is changed. {id: 'p5_canvas', index: 1, ownIndex: 0}, // Next; starting location again. - // {id: 'text_print', index: 0, ownIndex: 1}, // Previous to own next. + {id: 'text_print', index: 0, ownIndex: 1}, // Previous to own next. {id: 'text_print', index: 0, ownIndex: 4}, // Previous to own else input. - // {id: 'text_print', index: 0, ownIndex: 3}, // Previous to own if input. + {id: 'text_print', index: 0, ownIndex: 3}, // Previous to own if input. {id: 'text_print', index: 1, ownIndex: 0}, // Next. {id: 'controls_if', index: 3, ownIndex: 0}, // "If" statement input. {id: 'controls_repeat_ext', index: 3, ownIndex: 0}, // Statement input. @@ -817,12 +813,8 @@ suite('Keyboard-driven movement', function () { * pressing ArrowRight n times. */ const EXPECTED_COMPLEX_RIGHT = EXPECTED_SIMPLE_RIGHT.concat([ - // TODO(#702): With the current behavior, certain connection - // candidates that can be found using the mouse are not visited when - // doing a keyboard move. They appear in the list below, but commented - // out for now. They should be uncommented if the behavior is changed. {id: 'join0', index: 0, ownIndex: 2}, // Unattached block to own input. - // {id: 'join0', index: 0, ownIndex: 1}, // Unattached block to own input. + {id: 'join0', index: 0, ownIndex: 1}, // Unattached block to own input. ]); /** * Expected connection candidates when moving row consisting of