fix: widget positioning (#7507)

* chore: delete dead code

* chore: moves location updating into the block

* chore: change dragging to use update component locations

* fix: field widgets not being moved when blocks are editted

* chore: remove unnecessary resizeEditor_ calls

* chore: format

* chore: fix build

* fix: tests

* chore: PR comments

* chore: format

core/block_dragger.ts

@@ -29,6 +29,7 @@ import {Coordinate} from './utils/coordinate.js';
 import * as dom from './utils/dom.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
 import {hasBubble} from './interfaces/i_has_bubble.js';
+import * as deprecation from './utils/deprecation.js';
 
 /**
  * Class for a block dragger.  It moves blocks around the workspace when they
@@ -48,7 +49,12 @@ export class BlockDragger implements IBlockDragger {
   /** Whether the block would be deleted if dropped immediately. */
   protected wouldDeleteBlock_ = false;
   protected startXY_: Coordinate;
-  protected dragIconData_: IconPositionData[];
+
+  /**
+   * @deprecated To be removed in v11. Updating icons is now handled by the
+   *     block's `moveDuringDrag` method.
+   */
+  protected dragIconData_: IconPositionData[] = [];
 
   /**
    * @param block The block to drag.
@@ -70,11 +76,6 @@ export class BlockDragger implements IBlockDragger {
      */
     this.startXY_ = this.draggingBlock_.getRelativeToSurfaceXY();
 
-    /**
-     * A list of all of the icons (comment, warning, and mutator) that are
-     * on this block and its descendants.  Moving an icon moves the bubble that
-     * extends from it if that bubble is open.
-     */
     this.dragIconData_ = initIconData(block, this.startXY_);
   }
 
@@ -85,7 +86,6 @@ export class BlockDragger implements IBlockDragger {
    */
   dispose() {
     this.dragIconData_.length = 0;
-
     if (this.draggedConnectionManager_) {
       this.draggedConnectionManager_.dispose();
     }
@@ -178,7 +178,6 @@ export class BlockDragger implements IBlockDragger {
     const delta = this.pixelsToWorkspaceUnits_(currentDragDeltaXY);
     const newLoc = Coordinate.sum(this.startXY_, delta);
     this.draggingBlock_.moveDuringDrag(newLoc);
-    this.dragIcons_(delta);
 
     const oldDragTarget = this.dragTarget_;
     this.dragTarget_ = this.workspace_.getDragTarget(e);
@@ -210,7 +209,6 @@ export class BlockDragger implements IBlockDragger {
   endDrag(e: PointerEvent, currentDragDeltaXY: Coordinate) {
     // Make sure internal state is fresh.
     this.drag(e, currentDragDeltaXY);
-    this.dragIconData_ = [];
     this.fireDragEndEvent_();
 
     dom.stopTextWidthCache();
@@ -398,14 +396,11 @@ export class BlockDragger implements IBlockDragger {
   /**
    * Move all of the icons connected to this drag.
    *
-   * @param dxy How far to move the icons from their original positions, in
+   * @deprecated To be removed in v11. This is now handled by the block's
-   *     workspace units.
+   *     `moveDuringDrag` method.
    */
-  protected dragIcons_(dxy: Coordinate) {
+  protected dragIcons_() {
-    // Moving icons moves their associated bubbles.
+    deprecation.warn('Blockly.BlockDragger.prototype.dragIcons_', 'v10', 'v11');
-    for (const data of this.dragIconData_) {
-      data.icon.onLocationChange(Coordinate.sum(data.location, dxy));
-    }
   }
 
   /**

core/block_svg.ts

@@ -154,6 +154,9 @@ export class BlockSvg
    */
   private bumpNeighboursPid = 0;
 
+  /** Whether this block is currently being dragged. */
+  private dragging = false;
+
   /**
    * The location of the top left of this block (in workspace coordinates)
    * relative to either its parent block, or the workspace origin if it has no
@@ -384,9 +387,13 @@ export class BlockSvg
       event = new (eventUtils.get(eventUtils.BLOCK_MOVE)!)(this) as BlockMove;
       reason && event.setReason(reason);
     }
-    const xy = this.getRelativeToSurfaceXY();
+
-    this.translate(xy.x + dx, xy.y + dy);
+    const delta = new Coordinate(dx, dy);
-    this.moveConnections(dx, dy);
+    const currLoc = this.getRelativeToSurfaceXY();
+    const newLoc = Coordinate.sum(currLoc, delta);
+    this.translate(newLoc.x, newLoc.y);
+    this.updateComponentLocations(newLoc);
+
     if (eventsEnabled && event) {
       event!.recordNew();
       eventUtils.fire(event);
@@ -437,6 +444,7 @@ export class BlockSvg
   moveDuringDrag(newLoc: Coordinate) {
     this.translate(newLoc.x, newLoc.y);
     this.getSvgRoot().setAttribute('transform', this.getTranslation());
+    this.updateComponentLocations(newLoc);
   }
 
   /** Snap this block to the nearest grid point. */
@@ -649,32 +657,47 @@ export class BlockSvg
   }
 
   /**
-   * Move the connections for this block and all blocks attached under it.
+   * Updates the locations of any parts of the block that need to know where
-   * Also update any attached bubbles.
+   * they are (e.g. connections, icons).
    *
-   * @param dx Horizontal offset from current location, in workspace units.
+   * @param blockOrigin The top-left of this block in workspace coordinates.
-   * @param dy Vertical offset from current location, in workspace units.
    * @internal
    */
-  moveConnections(dx: number, dy: number) {
+  updateComponentLocations(blockOrigin: Coordinate) {
     if (!this.rendered) {
       // Rendering is required to lay out the blocks.
       // This is probably an invisible block attached to a collapsed block.
       return;
     }
-    const myConnections = this.getConnections_(false);
+
-    for (let i = 0; i < myConnections.length; i++) {
+    if (!this.dragging) this.updateConnectionLocations(blockOrigin);
-      myConnections[i].moveBy(dx, dy);
+    this.updateIconLocations(blockOrigin);
+    this.updateFieldLocations(blockOrigin);
+
+    for (const child of this.getChildren(false)) {
+      child.updateComponentLocations(
+        Coordinate.sum(blockOrigin, child.relativeCoords),
+      );
     }
-    const icons = this.getIcons();
-    const pos = this.getRelativeToSurfaceXY();
-    for (const icon of icons) {
-      icon.onLocationChange(pos);
   }
 
-    // Recurse through all blocks attached under this one.
+  private updateConnectionLocations(blockOrigin: Coordinate) {
-    for (let i = 0; i < this.childBlocks_.length; i++) {
+    for (const conn of this.getConnections_(false)) {
-      (this.childBlocks_[i] as BlockSvg).moveConnections(dx, dy);
+      conn.moveToOffset(blockOrigin);
+    }
+  }
+
+  private updateIconLocations(blockOrigin: Coordinate) {
+    for (const icon of this.getIcons()) {
+      icon.onLocationChange(blockOrigin);
+    }
+  }
+
+  private updateFieldLocations(blockOrigin: Coordinate) {
+    for (const input of this.inputList) {
+      for (const field of input.fieldRow) {
+        field.onLocationChange(blockOrigin);
+      }
     }
   }
 
@@ -686,6 +709,7 @@ export class BlockSvg
    * @internal
    */
   setDragging(adding: boolean) {
+    this.dragging = adding;
     if (adding) {
       this.translation = '';
       common.draggingConnections.push(...this.getConnections_(true));
@@ -1628,46 +1652,6 @@ export class BlockSvg
     }
   }
 
-  /**
-   * Update all of the connections on this block with the new locations
-   * calculated during rendering.  Also move all of the connected blocks based
-   * on the new connection locations.
-   *
-   * @internal
-   */
-  private updateConnectionAndIconLocations() {
-    const blockTL = this.getRelativeToSurfaceXY();
-    // Don't tighten previous or output connections because they are inferior
-    // connections.
-    if (this.previousConnection) {
-      this.previousConnection.moveToOffset(blockTL);
-    }
-    if (this.outputConnection) {
-      this.outputConnection.moveToOffset(blockTL);
-    }
-
-    for (let i = 0; i < this.inputList.length; i++) {
-      const conn = this.inputList[i].connection as RenderedConnection;
-      if (conn) {
-        conn.moveToOffset(blockTL);
-        if (conn.isConnected()) {
-          conn.tighten();
-        }
-      }
-    }
-
-    if (this.nextConnection) {
-      this.nextConnection.moveToOffset(blockTL);
-      if (this.nextConnection.isConnected()) {
-        this.nextConnection.tighten();
-      }
-    }
-
-    for (const icon of this.getIcons()) {
-      icon.onLocationChange(blockTL);
-    }
-  }
-
   /**
    * Add the cursor SVG to this block's SVG group.
    *

core/field.ts

@@ -960,6 +960,14 @@ export abstract class Field<T = any>
     return new Rect(xy.y, xy.y + scaledHeight, xy.x, xy.x + scaledWidth);
   }
 
+  /**
+   * Notifies the field that it has changed locations.
+   *
+   * @param _ The location of this field's block's top-start corner
+   *     in workspace coordinates.
+   */
+  onLocationChange(_: Coordinate) {}
+
   /**
    * Get the text from this field to display on the block. May differ from
    * `getText` due to ellipsis, and other formatting.

core/field_input.ts

@@ -33,7 +33,6 @@ import {Coordinate} from './utils/coordinate.js';
 import * as userAgent from './utils/useragent.js';
 import * as WidgetDiv from './widgetdiv.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
-import * as renderManagement from './render_management.js';
 import {Size} from './utils/size.js';
 
 /**
@@ -251,6 +250,14 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
     return super.getSize();
   }
 
+  /**
+   * Notifies the field that it has changed locations. Moves the widget div to
+   * be in the correct place if it is open.
+   */
+  onLocationChange(): void {
+    if (this.isBeingEdited_) this.resizeEditor_();
+  }
+
   /**
    * Updates the colour of the htmlInput given the current validity of the
    * field's value.
@@ -263,7 +270,6 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
     // This logic is done in render_ rather than doValueInvalid_ or
     // doValueUpdate_ so that the code is more centralized.
     if (this.isBeingEdited_) {
-      this.resizeEditor_();
       const htmlInput = this.htmlInput_ as HTMLElement;
       if (!this.isTextValid_) {
         dom.addClass(htmlInput, 'blocklyInvalidInput');
@@ -576,11 +582,6 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
         ),
       );
     }
-
-    // Resize the widget div after the block has finished rendering.
-    renderManagement.finishQueuedRenders().then(() => {
-      this.resizeEditor_();
-    });
   }
 
   /**
@@ -631,7 +632,7 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
   /**
    * Handles repositioning the WidgetDiv used for input fields when the
    * workspace is resized. Will bump the block into the viewport and update the
-   * position of the field if necessary.
+   * position of the text input if necessary.
    *
    * @returns True for rendered workspaces, as we never want to hide the widget
    *     div.
@@ -642,13 +643,13 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
     // rendered blocks.
     if (!(block instanceof BlockSvg)) return false;
 
-    bumpObjects.bumpIntoBounds(
+    const bumped = bumpObjects.bumpIntoBounds(
       this.workspace_!,
       this.workspace_!.getMetricsManager().getViewMetrics(true),
       block,
     );
 
-    this.resizeEditor_();
+    if (!bumped) this.resizeEditor_();
 
     return true;
   }

core/render_management.ts

@@ -5,7 +5,6 @@
  */
 
 import {BlockSvg} from './block_svg.js';
-import {Coordinate} from './utils/coordinate.js';
 import * as userAgent from './utils/useragent.js';
 
 /** The set of all blocks in need of rendering which don't have parents. */
@@ -114,28 +113,36 @@ function queueBlock(block: BlockSvg) {
  */
 function doRenders() {
   const workspaces = new Set([...rootBlocks].map((block) => block.workspace));
-  for (const block of rootBlocks) {
+  const blocks = [...rootBlocks].filter(shouldRenderRootBlock);
-    // No need to render a dead block.
+  for (const block of blocks) {
-    if (block.isDisposed()) continue;
-    // A render for this block may have been queued, and then the block was
-    // connected to a parent, so it is no longer a root block.
-    // Rendering will be triggered through the real root block.
-    if (block.getParent()) continue;
-
     renderBlock(block);
-    const blockOrigin = block.getRelativeToSurfaceXY();
-    updateConnectionLocations(block, blockOrigin);
-    updateIconLocations(block, blockOrigin);
   }
   for (const workspace of workspaces) {
     workspace.resizeContents();
   }
+  for (const block of blocks) {
+    const blockOrigin = block.getRelativeToSurfaceXY();
+    block.updateComponentLocations(blockOrigin);
+  }
 
   rootBlocks.clear();
   dirtyBlocks = new Set();
   afterRendersPromise = null;
 }
 
+/**
+ * Returns true if the block should be rendered.
+ *
+ * No need to render dead blocks.
+ *
+ * No need to render blocks with parents. A render for the block may have been
+ * queued, and the the block was connected to a parent, so it is no longer a
+ * root block. Rendering will be triggered through the real root block.
+ */
+function shouldRenderRootBlock(block: BlockSvg): boolean {
+  return !block.isDisposed() && !block.getParent();
+}
+
 /**
  * Recursively renders all of the dirty children of the given block, and
  * then renders the block.
@@ -149,44 +156,3 @@ function renderBlock(block: BlockSvg) {
   }
   block.renderEfficiently();
 }
-
-/**
- * Updates the connection database with the new locations of all of the
- * connections that are children of the given block.
- *
- * @param block The block to update the connection locations of.
- * @param blockOrigin The top left of the given block in workspace coordinates.
- */
-function updateConnectionLocations(block: BlockSvg, blockOrigin: Coordinate) {
-  for (const conn of block.getConnections_(false)) {
-    const moved = conn.moveToOffset(blockOrigin);
-    const target = conn.targetBlock();
-    if (!conn.isSuperior()) continue;
-    if (!target) continue;
-    if (moved || dirtyBlocks.has(target)) {
-      updateConnectionLocations(
-        target,
-        Coordinate.sum(blockOrigin, target.relativeCoords),
-      );
-    }
-  }
-}
-
-/**
- * Updates all icons that are children of the given block with their new
- * locations.
- *
- * @param block The block to update the icon locations of.
- */
-function updateIconLocations(block: BlockSvg, blockOrigin: Coordinate) {
-  if (!block.getIcons) return;
-  for (const icon of block.getIcons()) {
-    icon.onLocationChange(blockOrigin);
-  }
-  for (const child of block.getChildren(false)) {
-    updateIconLocations(
-      child,
-      Coordinate.sum(blockOrigin, child.relativeCoords),
-    );
-  }
-}

core/rendered_connection.ts

@@ -24,7 +24,6 @@ import * as internalConstants from './internal_constants.js';
 import {Coordinate} from './utils/coordinate.js';
 import * as dom from './utils/dom.js';
 import {Svg} from './utils/svg.js';
-import * as svgMath from './utils/svg_math.js';
 import * as svgPaths from './utils/svg_paths.js';
 
 /** A shape that has a pathDown property. */
@@ -270,27 +269,6 @@ export class RenderedConnection extends Connection {
     return this.offsetInBlock;
   }
 
-  /**
-   * Move the blocks on either side of this connection right next to each other.
-   *
-   * @internal
-   */
-  tighten() {
-    const dx = this.targetConnection!.x - this.x;
-    const dy = this.targetConnection!.y - this.y;
-    if (dx !== 0 || dy !== 0) {
-      const block = this.targetBlock();
-      const svgRoot = block!.getSvgRoot();
-      if (!svgRoot) {
-        throw Error('block is not rendered.');
-      }
-      // Workspace coordinates.
-      const xy = svgMath.getRelativeXY(svgRoot);
-      block!.translate(xy.x - dx, xy.y - dy);
-      block!.moveConnections(-dx, -dy);
-    }
-  }
-
   /**
    * Moves the blocks on either side of this connection right next to
    * each other, based on their local offsets, not global positions.

tests/mocha/render_management_test.js

@@ -30,8 +30,8 @@ suite('Render Management', function () {
         getParent: () => null,
         getChildren: () => [],
         isDisposed: () => false,
-        getConnections_: () => [],
         getRelativeToSurfaceXY: () => ({x: 0, y: 0}),
+        updateComponentLocations: () => {},
         workspace: {
           resizeContents: () => {},
         },