From cffd4584490f52e31ca1c1d97195ff050817bab1 Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Fri, 25 Apr 2025 11:48:54 +0100 Subject: [PATCH 1/5] 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). --- tests/mocha/block_test.js | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/mocha/block_test.js b/tests/mocha/block_test.js index 56cfba54290..85a9f29181c 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); From 8ac378754a7e9f6f15fb5093c0436330f72f6549 Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Fri, 25 Apr 2025 12:37:49 +0100 Subject: [PATCH 2/5] 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. --- core/block_svg.ts | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index b8712b01914..731213cccc2 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 canvass = this.workspace.getCanvas(); + if (draggingParentElement === canvass) { + canvass.insertBefore(svgRoot, draggingBlockElement); } else { - this.workspace.getCanvas().appendChild(svgRoot); + canvass.appendChild(svgRoot); } this.translate(oldXY.x, oldXY.y); } From e85f6574aaa420f63c8314bfeef35b6ff34ca063 Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Fri, 25 Apr 2025 13:41:30 +0100 Subject: [PATCH 3/5] refactor(BlockDragStrategy): Hide connection preview earlier --- core/dragging/block_drag_strategy.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/dragging/block_drag_strategy.ts b/core/dragging/block_drag_strategy.ts index b53c131653b..32c7d68da4d 100644 --- a/core/dragging/block_drag_strategy.ts +++ b/core/dragging/block_drag_strategy.ts @@ -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; } From cd7f33f6a33dc731dcb798d10aee2373c118390e Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Tue, 29 Apr 2025 16:11:15 +0100 Subject: [PATCH 4/5] chore(BlockDragStrategy): prefer ?. to !. Per nit on PR #8934. --- core/dragging/block_drag_strategy.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/core/dragging/block_drag_strategy.ts b/core/dragging/block_drag_strategy.ts index 32c7d68da4d..76020f90b5b 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,7 +445,7 @@ export class BlockDragStrategy implements IDragStrategy { return; } - this.connectionPreviewer!.hidePreview(); + this.connectionPreviewer?.hidePreview(); this.connectionCandidate = null; this.startChildConn?.connect(this.block.nextConnection); From f73b76d2a2a011ef4d87639ce0443b7b08440f66 Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Tue, 29 Apr 2025 16:11:43 +0100 Subject: [PATCH 5/5] fix(BlockSvg): Spelling: "canvass" -> "canvas" --- core/block_svg.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index 731213cccc2..4cef79df8e6 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -316,11 +316,11 @@ export class BlockSvg | SVGElement | null | undefined; - const canvass = this.workspace.getCanvas(); - if (draggingParentElement === canvass) { - canvass.insertBefore(svgRoot, draggingBlockElement); + const canvas = this.workspace.getCanvas(); + if (draggingParentElement === canvas) { + canvas.insertBefore(svgRoot, draggingBlockElement); } else { - canvass.appendChild(svgRoot); + canvas.appendChild(svgRoot); } this.translate(oldXY.x, oldXY.y); }