From b9b40f48abff9d7d4ff352039d982639458c975f Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Tue, 29 Apr 2025 16:15:42 +0100 Subject: [PATCH] fix: Fix bug in BlockSvg.prototype.setParent (#8934) * test(BlockSvg): Add tests for setParent(null) when dragging Add tests for scenarios where block(s) unrelated to the block being disconnected has/have been marked as as being dragged. Due to a bug in BlockSvg.prototype.setParent, one of these fails in the case that the dragging block is not a top block. Also add a check to assertNonParentAndOrphan to check that the orphan block's SVG root's parent is the workspace's canvass (i.e., that orphan is a top-level block in the DOM too). * fix(BlockSvg): Fix bug in setParent re: dragging block Fix an incorrect assumption in setParent: the topmost block whose root SVG element has the blocklyDragging class may not actually be a top-level block. * refactor(BlockDragStrategy): Hide connection preview earlier * chore(BlockDragStrategy): prefer ?. to !. Per nit on PR #8934. * fix(BlockSvg): Spelling: "canvass" -> "canvas" --- core/block_svg.ts | 18 ++++++++++---- core/dragging/block_drag_strategy.ts | 18 +++++++------- tests/mocha/block_test.js | 37 ++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 14 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index b8712b019..4cef79df8 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -305,14 +305,22 @@ export class BlockSvg (newParent as BlockSvg).getSvgRoot().appendChild(svgRoot); } else if (oldParent) { // If we are losing a parent, we want to move our DOM element to the - // root of the workspace. - const draggingBlock = this.workspace + // root of the workspace. Try to insert it before any top-level + // block being dragged, but note that blocks can have the + // blocklyDragging class even if they're not top blocks (especially + // at start and end of a drag). + const draggingBlockElement = this.workspace .getCanvas() .querySelector('.blocklyDragging'); - if (draggingBlock) { - this.workspace.getCanvas().insertBefore(svgRoot, draggingBlock); + const draggingParentElement = draggingBlockElement?.parentElement as + | SVGElement + | null + | undefined; + const canvas = this.workspace.getCanvas(); + if (draggingParentElement === canvas) { + canvas.insertBefore(svgRoot, draggingBlockElement); } else { - this.workspace.getCanvas().appendChild(svgRoot); + canvas.appendChild(svgRoot); } this.translate(oldXY.x, oldXY.y); } diff --git a/core/dragging/block_drag_strategy.ts b/core/dragging/block_drag_strategy.ts index b53c13165..76020f90b 100644 --- a/core/dragging/block_drag_strategy.ts +++ b/core/dragging/block_drag_strategy.ts @@ -238,7 +238,7 @@ export class BlockDragStrategy implements IDragStrategy { const currCandidate = this.connectionCandidate; const newCandidate = this.getConnectionCandidate(draggingBlock, delta); if (!newCandidate) { - this.connectionPreviewer!.hidePreview(); + this.connectionPreviewer?.hidePreview(); this.connectionCandidate = null; return; } @@ -254,7 +254,7 @@ export class BlockDragStrategy implements IDragStrategy { local.type === ConnectionType.OUTPUT_VALUE || local.type === ConnectionType.PREVIOUS_STATEMENT; const neighbourIsConnectedToRealBlock = - neighbour.isConnected() && !neighbour.targetBlock()!.isInsertionMarker(); + neighbour.isConnected() && !neighbour.targetBlock()?.isInsertionMarker(); if ( localIsOutputOrPrevious && neighbourIsConnectedToRealBlock && @@ -264,14 +264,14 @@ export class BlockDragStrategy implements IDragStrategy { local.type, ) ) { - this.connectionPreviewer!.previewReplacement( + this.connectionPreviewer?.previewReplacement( local, neighbour, neighbour.targetBlock()!, ); return; } - this.connectionPreviewer!.previewConnection(local, neighbour); + this.connectionPreviewer?.previewConnection(local, neighbour); } /** @@ -385,7 +385,7 @@ export class BlockDragStrategy implements IDragStrategy { dom.stopTextWidthCache(); blockAnimation.disconnectUiStop(); - this.connectionPreviewer!.hidePreview(); + this.connectionPreviewer?.hidePreview(); if (!this.block.isDeadOrDying() && this.dragging) { // These are expensive and don't need to be done if we're deleting, or @@ -413,7 +413,7 @@ export class BlockDragStrategy implements IDragStrategy { // Must dispose after connections are applied to not break the dynamic // connections plugin. See #7859 - this.connectionPreviewer!.dispose(); + this.connectionPreviewer?.dispose(); this.workspace.setResizesEnabled(true); eventUtils.setGroup(newGroup); } @@ -445,6 +445,9 @@ export class BlockDragStrategy implements IDragStrategy { return; } + this.connectionPreviewer?.hidePreview(); + this.connectionCandidate = null; + this.startChildConn?.connect(this.block.nextConnection); if (this.startParentConn) { switch (this.startParentConn.type) { @@ -471,9 +474,6 @@ export class BlockDragStrategy implements IDragStrategy { this.startChildConn = null; this.startParentConn = null; - this.connectionPreviewer!.hidePreview(); - this.connectionCandidate = null; - this.block.setDragging(false); this.dragging = false; } diff --git a/tests/mocha/block_test.js b/tests/mocha/block_test.js index 56cfba542..85a9f2918 100644 --- a/tests/mocha/block_test.js +++ b/tests/mocha/block_test.js @@ -1105,6 +1105,18 @@ suite('Blocks', function () { ); this.textJoinBlock = this.printBlock.getInputTargetBlock('TEXT'); this.textBlock = this.textJoinBlock.getInputTargetBlock('ADD0'); + this.extraTopBlock = Blockly.Xml.domToBlock( + Blockly.utils.xml.textToDom(` + + + + drag me + + + `), + this.workspace, + ); + this.extraNestedBlock = this.extraTopBlock.getInputTargetBlock('TEXT'); }); function assertBlockIsOnlyChild(parent, child, inputName) { @@ -1116,6 +1128,10 @@ suite('Blocks', function () { assert.equal(nonParent.getChildren().length, 0); assert.isNull(nonParent.getInputTargetBlock('TEXT')); assert.isNull(orphan.getParent()); + assert.equal( + orphan.getSvgRoot().parentElement, + orphan.workspace.getCanvas(), + ); } function assertOriginalSetup() { assertBlockIsOnlyChild(this.printBlock, this.textJoinBlock, 'TEXT'); @@ -1187,6 +1203,27 @@ suite('Blocks', function () { ); assertNonParentAndOrphan(this.textJoinBlock, this.textBlock, 'ADD0'); }); + test('Setting parent to null with dragging block', function () { + this.extraTopBlock.setDragging(true); + this.textBlock.outputConnection.disconnect(); + assert.doesNotThrow( + this.textBlock.setParent.bind(this.textBlock, null), + ); + assertNonParentAndOrphan(this.textJoinBlock, this.textBlock, 'ADD0'); + assert.equal( + this.textBlock.getSvgRoot().nextSibling, + this.extraTopBlock.getSvgRoot(), + ); + }); + test('Setting parent to null with non-top dragging block', function () { + this.extraNestedBlock.setDragging(true); + this.textBlock.outputConnection.disconnect(); + assert.doesNotThrow( + this.textBlock.setParent.bind(this.textBlock, null), + ); + assertNonParentAndOrphan(this.textJoinBlock, this.textBlock, 'ADD0'); + assert.equal(this.textBlock.getSvgRoot().nextSibling, null); + }); test('Setting parent to null without disconnecting', function () { assert.throws(this.textBlock.setParent.bind(this.textBlock, null)); assertOriginalSetup.call(this);