refactor!: Move responsibility for block creation out of flyouts (#9610)

* refactor!: Move responsibility for block creation out of flyouts

* chore: Clarify naming and documentation

* fix: Make test less convoluted

* refactor: Use serialization instead of zero-length drag to handle block clicks

* fix: Fix undoing when dragging a block from the flyout

* refactor: Make `getTargetBlock()` always return a value

Author: gonfunko

packages/blockly/core/dragging/block_drag_strategy.ts

@@ -25,8 +25,10 @@ import * as layers from '../layers.js';
 import * as registry from '../registry.js';
 import {finishQueuedRenders} from '../render_management.js';
 import type {RenderedConnection} from '../rendered_connection.js';
+import * as blocks from '../serialization/blocks.js';
 import {Coordinate} from '../utils.js';
 import * as dom from '../utils/dom.js';
+import * as svgMath from '../utils/svg_math.js';
 import type {WorkspaceSvg} from '../workspace_svg.js';
 
 /** Represents a nearby valid connection. */
@@ -95,17 +97,73 @@ export class BlockDragStrategy implements IDragStrategy {
       this.block.isOwnMovable() &&
       !this.block.isDeadOrDying() &&
       !this.workspace.isReadOnly() &&
-      // We never drag blocks in the flyout, only create new blocks that are
-      // dragged.
-      !this.block.isInFlyout
+      (!this.block.isInFlyout ||
+        (this.block.isEnabled() &&
+          !this.block.workspace.targetWorkspace?.isReadOnly()))
     );
   }
 
+  /**
+   * Positions a cloned block on its new workspace.
+   *
+   * @param oldBlock The flyout block that was cloned.
+   * @param newBlock The new block to position.
+   */
+  private positionNewBlock(oldBlock: BlockSvg, newBlock: BlockSvg) {
+    const screenCoordinate = svgMath.wsToScreenCoordinates(
+      oldBlock.workspace,
+      oldBlock.getRelativeToSurfaceXY(),
+    );
+    const workspaceCoordinates = svgMath.screenToWsCoordinates(
+      newBlock.workspace,
+      screenCoordinate,
+    );
+    newBlock.moveTo(workspaceCoordinates);
+  }
+
+  /**
+   * Returns the block to use for the current drag operation. This may create
+   * and return a newly instantiated block when e.g. dragging from a flyout.
+   */
+  protected getTargetBlock() {
+    if (this.block.isShadow()) {
+      const parent = this.block.getParent();
+      if (parent) {
+        return parent;
+      }
+    } else if (this.block.isInFlyout && this.block.workspace.targetWorkspace) {
+      const rootBlock = this.block.getRootBlock();
+
+      const json = blocks.save(rootBlock);
+      if (json) {
+        const newBlock = blocks.appendInternal(
+          json,
+          this.block.workspace.targetWorkspace,
+          {
+            recordUndo: true,
+          },
+        ) as BlockSvg;
+        eventUtils.setRecordUndo(false);
+        this.positionNewBlock(this.block, newBlock);
+        eventUtils.setRecordUndo(true);
+
+        return newBlock;
+      }
+    }
+
+    return this.block;
+  }
+
   /**
    * Handles any setup for starting the drag, including disconnecting the block
    * from any parent blocks.
    */
   startDrag(e?: PointerEvent | KeyboardEvent) {
+    const alternateTarget = this.getTargetBlock();
+    if (alternateTarget !== this.block) {
+      return alternateTarget.startDrag(e);
+    }
+
     this.dragging = true;
     this.fireDragStartEvent();
 

packages/blockly/core/dragging/dragger.ts

@@ -16,17 +16,13 @@ import type {IDragger} from '../interfaces/i_dragger.js';
 import {isFocusableNode} from '../interfaces/i_focusable_node.js';
 import * as registry from '../registry.js';
 import {Coordinate} from '../utils/coordinate.js';
-import type {WorkspaceSvg} from '../workspace_svg.js';
 
 export class Dragger implements IDragger {
   protected startLoc: Coordinate;
 
   protected dragTarget: IDragTarget | null = null;
 
-  constructor(
-    protected draggable: IDraggable,
-    protected workspace: WorkspaceSvg,
-  ) {
+  constructor(protected draggable: IDraggable) {
     this.startLoc = draggable.getRelativeToSurfaceXY();
   }
 
@@ -65,7 +61,7 @@ export class Dragger implements IDragger {
 
   /** Updates the drag target under the pointer (if there is one). */
   protected updateDragTarget(coordinate: Coordinate) {
-    const newDragTarget = this.workspace.getDragTarget(coordinate);
+    const newDragTarget = this.draggable.workspace.getDragTarget(coordinate);
     if (this.dragTarget !== newDragTarget) {
       this.dragTarget?.onDragExit(this.draggable);
       newDragTarget?.onDragEnter(this.draggable);
@@ -95,10 +91,10 @@ export class Dragger implements IDragger {
     coordinate: Coordinate,
     rootDraggable: IDraggable & IDeletable,
   ) {
-    const dragTarget = this.workspace.getDragTarget(coordinate);
+    const dragTarget = this.draggable.workspace.getDragTarget(coordinate);
     if (!dragTarget) return false;
 
-    const componentManager = this.workspace.getComponentManager();
+    const componentManager = this.draggable.workspace.getComponentManager();
     const isDeleteArea = componentManager.hasCapability(
       dragTarget.id,
       ComponentManager.Capability.DELETE_AREA,
@@ -111,7 +107,7 @@ export class Dragger implements IDragger {
   /** Handles any drag cleanup. */
   onDragEnd(e?: PointerEvent | KeyboardEvent) {
     const origGroup = eventUtils.getGroup();
-    const dragTarget = this.workspace.getDragTarget(
+    const dragTarget = this.draggable.workspace.getDragTarget(
       this.draggable.getRelativeToSurfaceXY(),
     );
 
@@ -175,21 +171,21 @@ export class Dragger implements IDragger {
     coordinate: Coordinate,
     rootDraggable: IDraggable,
   ) {
-    const dragTarget = this.workspace.getDragTarget(coordinate);
+    const dragTarget = this.draggable.workspace.getDragTarget(coordinate);
     if (!dragTarget) return false;
     return dragTarget.shouldPreventMove(rootDraggable);
   }
 
   protected pixelsToWorkspaceUnits(pixelCoord: Coordinate): Coordinate {
     const result = new Coordinate(
-      pixelCoord.x / this.workspace.scale,
-      pixelCoord.y / this.workspace.scale,
+      pixelCoord.x / this.draggable.workspace.scale,
+      pixelCoord.y / this.draggable.workspace.scale,
     );
-    if (this.workspace.isMutator) {
+    if (this.draggable.workspace.isMutator) {
       // If we're in a mutator, its scale is always 1, purely because of some
       // oddities in our rendering optimizations.  The actual scale is the same
       // as the scale on the parent workspace. Fix that for dragging.
-      const mainScale = this.workspace.options.parentWorkspace!.scale;
+      const mainScale = this.draggable.workspace.options.parentWorkspace!.scale;
       result.scale(1 / mainScale);
     }
     return result;

packages/blockly/core/flyout_base.ts

@@ -11,7 +11,6 @@
  */
 // Former goog.module ID: Blockly.Flyout
 
-import {BlockSvg} from './block_svg.js';
 import * as browserEvents from './browser_events.js';
 import {ComponentManager} from './component_manager.js';
 import {DeleteArea} from './delete_area.js';
@@ -32,8 +31,6 @@ import * as registry from './registry.js';
 import * as renderManagement from './render_management.js';
 import {ScrollbarPair} from './scrollbar_pair.js';
 import {SEPARATOR_TYPE} from './separator_flyout_inflater.js';
-import * as blocks from './serialization/blocks.js';
-import {Coordinate} from './utils/coordinate.js';
 import * as dom from './utils/dom.js';
 import * as idGenerator from './utils/idgenerator.js';
 import {Svg} from './utils/svg.js';
@@ -52,17 +49,6 @@ export abstract class Flyout
    */
   abstract position(): void;
 
-  /**
-   * Determine if a drag delta is toward the workspace, based on the position
-   * and orientation of the flyout. This is used in determineDragIntention_ to
-   * determine if a new block should be created or if the flyout should scroll.
-   *
-   * @param currentDragDeltaXY How far the pointer has
-   *     moved from the position at mouse down, in pixel units.
-   * @returns True if the drag is toward the workspace.
-   */
-  abstract isDragTowardWorkspace(currentDragDeltaXY: Coordinate): boolean;
-
   /**
    * Sets the translation of the flyout to match the scrollbars.
    *
@@ -191,29 +177,6 @@ export abstract class Flyout
    * Height of flyout.
    */
   protected height_ = 0;
-  // clang-format off
-  /**
-   * Range of a drag angle from a flyout considered "dragging toward
-   * workspace". Drags that are within the bounds of this many degrees from
-   * the orthogonal line to the flyout edge are considered to be "drags toward
-   * the workspace".
-   *
-   * @example
-   *
-   * ```
-   * Flyout                                                 Edge   Workspace
-   * [block] /  <-within this angle, drags "toward workspace" |
-   * [block] ---- orthogonal to flyout boundary ----          |
-   * [block] \                                                |
-   * ```
-   *
-   * The angle is given in degrees from the orthogonal.
-   *
-   * This is used to know when to create a new block and when to scroll the
-   * flyout. Setting it to 360 means that all drags create a new block.
-   */
-  // clang-format on
-  protected dragAngleRange_ = 70;
 
   /**
    * The path around the background of the flyout, which will be filled with a
@@ -790,47 +753,6 @@ export abstract class Flyout
     }
   }
 
-  /**
-   * Does this flyout allow you to create a new instance of the given block?
-   * Used for deciding if a block can be "dragged out of" the flyout.
-   *
-   * @param block The block to copy from the flyout.
-   * @returns True if you can create a new instance of the block, false
-   *    otherwise.
-   * @internal
-   */
-  isBlockCreatable(block: BlockSvg): boolean {
-    return block.isEnabled() && !this.getTargetWorkspace().isReadOnly();
-  }
-
-  /**
-   * Create a copy of this block on the workspace.
-   *
-   * @param originalBlock The block to copy from the flyout.
-   * @returns The newly created block.
-   * @throws {Error} if something went wrong with deserialization.
-   * @internal
-   */
-  createBlock(originalBlock: BlockSvg): BlockSvg {
-    const targetWorkspace = this.targetWorkspace;
-    const svgRootOld = originalBlock.getSvgRoot();
-    if (!svgRootOld) {
-      throw Error('oldBlock is not rendered');
-    }
-
-    // Clone the block.
-    const json = this.serializeBlock(originalBlock);
-    // Normally this resizes leading to weird jumps. Save it for terminateDrag.
-    targetWorkspace.setResizesEnabled(false);
-    const block = blocks.appendInternal(json, targetWorkspace, {
-      recordUndo: true,
-    }) as BlockSvg;
-
-    this.positionNewBlock(originalBlock, block);
-    targetWorkspace.hideChaff();
-    return block;
-  }
-
   /**
    * Reflow flyout contents.
    */
@@ -851,59 +773,6 @@ export abstract class Flyout
       : false;
   }
 
-  /**
-   * Serialize a block to JSON.
-   *
-   * @param block The block to serialize.
-   * @returns A serialized representation of the block.
-   */
-  protected serializeBlock(block: BlockSvg): blocks.State {
-    return blocks.save(block) as blocks.State;
-  }
-
-  /**
-   * Positions a block on the target workspace.
-   *
-   * @param oldBlock The flyout block being copied.
-   * @param block The block to posiiton.
-   */
-  private positionNewBlock(oldBlock: BlockSvg, block: BlockSvg) {
-    const targetWorkspace = this.targetWorkspace;
-
-    // The offset in pixels between the main workspace's origin and the upper
-    // left corner of the injection div.
-    const mainOffsetPixels = targetWorkspace.getOriginOffsetInPixels();
-
-    // The offset in pixels between the flyout workspace's origin and the upper
-    // left corner of the injection div.
-    const flyoutOffsetPixels = this.workspace_.getOriginOffsetInPixels();
-
-    // The position of the old block in flyout workspace coordinates.
-    const oldBlockPos = oldBlock.getRelativeToSurfaceXY();
-    // The position of the old block in pixels relative to the flyout
-    // workspace's origin.
-    oldBlockPos.scale(this.workspace_.scale);
-
-    // The position of the old block in pixels relative to the upper left corner
-    // of the injection div.
-    const oldBlockOffsetPixels = Coordinate.sum(
-      flyoutOffsetPixels,
-      oldBlockPos,
-    );
-
-    // The position of the old block in pixels relative to the origin of the
-    // main workspace.
-    const finalOffset = Coordinate.difference(
-      oldBlockOffsetPixels,
-      mainOffsetPixels,
-    );
-    // The position of the old block in main workspace coordinates.
-    finalOffset.scale(1 / targetWorkspace.scale);
-
-    // No 'reason' provided since events are disabled.
-    block.moveTo(new Coordinate(finalOffset.x, finalOffset.y));
-  }
-
   /**
    * Returns the inflater responsible for constructing items of the given type.
    *

packages/blockly/core/flyout_horizontal.ts

@@ -18,7 +18,6 @@ import type {FlyoutItem} from './flyout_item.js';
 import type {Options} from './options.js';
 import * as registry from './registry.js';
 import {Scrollbar} from './scrollbar.js';
-import type {Coordinate} from './utils/coordinate.js';
 import {Rect} from './utils/rect.js';
 import * as toolbox from './utils/toolbox.js';
 import * as WidgetDiv from './widgetdiv.js';
@@ -271,32 +270,6 @@ export class HorizontalFlyout extends Flyout {
     }
   }
 
-  /**
-   * Determine if a drag delta is toward the workspace, based on the position
-   * and orientation of the flyout. This is used in determineDragIntention_ to
-   * determine if a new block should be created or if the flyout should scroll.
-   *
-   * @param currentDragDeltaXY How far the pointer has moved from the position
-   *     at mouse down, in pixel units.
-   * @returns True if the drag is toward the workspace.
-   */
-  override isDragTowardWorkspace(currentDragDeltaXY: Coordinate): boolean {
-    const dx = currentDragDeltaXY.x;
-    const dy = currentDragDeltaXY.y;
-    // Direction goes from -180 to 180, with 0 toward the right and 90 on top.
-    const dragDirection = (Math.atan2(dy, dx) / Math.PI) * 180;
-
-    const range = this.dragAngleRange_;
-    // Check for up or down dragging.
-    if (
-      (dragDirection < 90 + range && dragDirection > 90 - range) ||
-      (dragDirection > -90 - range && dragDirection < -90 + range)
-    ) {
-      return true;
-    }
-    return false;
-  }
-
   /**
    * Returns the bounding rectangle of the drag target area in pixel units
    * relative to viewport.