fix: Ensure immovable blocks are considered during workspace tidying

core/utils/rect.ts

@@ -13,6 +13,8 @@
  */
 // Former goog.module ID: Blockly.utils.Rect
 
+import {Coordinate} from './coordinate.js';
+
 /**
  * Class for representing rectangular regions.
  */
@@ -30,10 +32,21 @@ export class Rect {
     public right: number,
   ) {}
 
+  /**
+   * Creates a new copy of this rectangle.
+   *
+   * @returns A copy of this Rect.
+   */
+  clone(): Rect {
+    return new Rect(this.top, this.bottom, this.left, this.right);
+  }
+
+  /** Returns the height of this rectangle. */
   getHeight(): number {
     return this.bottom - this.top;
   }
 
+  /** Returns the width of this rectangle. */
   getWidth(): number {
     return this.right - this.left;
   }
@@ -59,11 +72,56 @@ export class Rect {
    * @returns Whether this rectangle intersects the provided rectangle.
    */
   intersects(other: Rect): boolean {
-    return !(
-      this.left > other.right ||
-      this.right < other.left ||
-      this.top > other.bottom ||
-      this.bottom < other.top
+    // The following logic can be derived and then simplified from a longer form symmetrical check
+    // of verifying each rectangle's borders with the other rectangle by checking if either end of
+    // the border's line segment is contained within the other rectangle. The simplified version
+    // used here can be logically interpreted as ensuring that each line segment of 'this' rectangle
+    // is not outside the bounds of the 'other' rectangle (proving there's an intersection).
+    return (
+      this.left <= other.right &&
+      this.right >= other.left &&
+      this.bottom >= other.top &&
+      this.top <= other.bottom
     );
   }
+
+  /**
+   * Compares bounding rectangles for equality.
+   *
+   * @param a A Rect.
+   * @param b A Rect.
+   * @returns True iff the bounding rectangles are equal, or if both are null.
+   */
+  static equals(a?: Rect | null, b?: Rect | null): boolean {
+    if (a === b) {
+      return true;
+    }
+    if (!a || !b) {
+      return false;
+    }
+    return (
+      a.top === b.top &&
+      a.bottom === b.bottom &&
+      a.left === b.left &&
+      a.right === b.right
+    );
+  }
+
+  /**
+   * Creates a new Rect using a position and supplied dimensions.
+   *
+   * @param position The upper left coordinate of the new rectangle.
+   * @param width The width of the rectangle, in pixels.
+   * @param height The height of the rectangle, in pixels.
+   * @returns A newly created Rect using the provided Coordinate and dimensions.
+   */
+  static createFromPoint(
+    position: Coordinate,
+    width: number,
+    height: number,
+  ): Rect {
+    const left = position.x;
+    const top = position.y;
+    return new Rect(top, top + height, left, left + width);
+  }
 }

core/workspace_svg.ts

@@ -1645,23 +1645,56 @@ export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg {
     return boundary;
   }
 
-  /** Clean up the workspace by ordering all the blocks in a column. */
+  /** Clean up the workspace by ordering all the blocks in a column such that none overlap. */
   cleanUp() {
     this.setResizesEnabled(false);
     eventUtils.setGroup(true);
+
     const topBlocks = this.getTopBlocks(true);
-    let cursorY = 0;
-    for (let i = 0, block; (block = topBlocks[i]); i++) {
-      if (!block.isMovable()) {
-        continue;
+    const movableBlocks = topBlocks.filter((block) => block.isMovable());
+    const immovableBlocks = topBlocks.filter((block) => !block.isMovable());
+
+    const immovableBlockBounds = immovableBlocks.map((block) =>
+      block.getBoundingRectangle(),
+    );
+
+    const getNextIntersectingImmovableBlock = function (
+      rect: Rect,
+    ): Rect | null {
+      for (const immovableRect of immovableBlockBounds) {
+        if (rect.intersects(immovableRect)) {
+          return immovableRect;
+        }
       }
-      const xy = block.getRelativeToSurfaceXY();
-      block.moveBy(-xy.x, cursorY - xy.y, ['cleanup']);
+      return null;
+    };
+
+    let cursorY = 0;
+    const minBlockHeight = this.renderer.getConstants().MIN_BLOCK_HEIGHT;
+    for (const block of movableBlocks) {
+      // Make the initial movement of shifting the block to its best possible position.
+      let boundingRect = block.getBoundingRectangle();
+      block.moveBy(-boundingRect.left, cursorY - boundingRect.top, ['cleanup']);
       block.snapToGrid();
+
+      boundingRect = block.getBoundingRectangle();
+      let conflictingRect = getNextIntersectingImmovableBlock(boundingRect);
+      while (conflictingRect != null) {
+        // If the block intersects with an immovable block, move it down past that immovable block.
+        cursorY =
+          conflictingRect.top + conflictingRect.getHeight() + minBlockHeight;
+        block.moveBy(0, cursorY - boundingRect.top, ['cleanup']);
+        block.snapToGrid();
+        boundingRect = block.getBoundingRectangle();
+        conflictingRect = getNextIntersectingImmovableBlock(boundingRect);
+      }
+
+      // Ensure all next blocks start past the most recent (which will also put them past all
+      // previously intersecting immovable blocks).
       cursorY =
         block.getRelativeToSurfaceXY().y +
         block.getHeightWidth().height +
-        this.renderer.getConstants().MIN_BLOCK_HEIGHT;
+        minBlockHeight;
     }
     eventUtils.setGroup(false);
     this.setResizesEnabled(true);

tests/mocha/contextmenu_items_test.js

@@ -123,7 +123,7 @@ suite('Context Menu Items', function () {
     suite('Cleanup', function () {
       setup(function () {
         this.cleanupOption = this.registry.getItem('cleanWorkspace');
-        this.cleanupStub = sinon.stub(this.workspace, 'cleanUp');
+        this.cleanUpStub = sinon.stub(this.workspace, 'cleanUp');
       });
 
       test('Enabled when multiple blocks', function () {
@@ -153,9 +153,9 @@ suite('Context Menu Items', function () {
         );
       });
 
-      test('Calls workspace cleanup', function () {
+      test('Calls workspace cleanUp', function () {
         this.cleanupOption.callback(this.scope);
-        sinon.assert.calledOnce(this.cleanupStub);
+        sinon.assert.calledOnce(this.cleanUpStub);
       });
 
       test('Has correct label', function () {

tests/mocha/index.html

@@ -110,6 +110,7 @@
       import './old_workspace_comment_test.js';
       import './procedure_map_test.js';
       import './blocks/procedures_test.js';
+      import './rect_test.js';
       import './registry_test.js';
       import './render_management_test.js';
       import './serializer_test.js';