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"

Author: cpcallen

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);
     }

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;
   }

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(`
+            <block type="text_print">
+              <value name="TEXT">
+                <block type="text">
+                  <field name="TEXT">drag me</field>
+                </block>
+              </value>
+            </block>`),
+          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);