feat!: change gestures to look at selected when dragging (#7991)

* feat: change gestures to look at selected when dragging

* chore: fix tests

* chore: format

* chore: PR comments

blocks/procedures.ts

@@ -38,6 +38,7 @@ import {config} from '../core/config.js';
 import {defineBlocks} from '../core/common.js';
 import '../core/icons/comment_icon.js';
 import '../core/icons/warning_icon.js';
+import * as common from '../core/common.js';
 
 /** A dictionary of the block definitions provided by this module. */
 export const blocks: {[key: string]: BlockDefinition} = {};
@@ -1157,7 +1158,7 @@ const PROCEDURE_CALL_COMMON = {
         const def = Procedures.getDefinition(name, workspace);
         if (def) {
           (workspace as WorkspaceSvg).centerOnBlock(def.id);
-          (def as BlockSvg).select();
+          common.setSelected(def as BlockSvg);
         }
       },
     });

core/block_svg.ts

@@ -241,56 +241,18 @@ export class BlockSvg
     return this.style.colourTertiary;
   }
 
-  /**
-   * Selects this block. Highlights the block visually and fires a select event
-   * if the block is not already selected.
-   */
+  /** Selects this block. Highlights the block visually. */
   select() {
     if (this.isShadow() && this.getParent()) {
       // Shadow blocks should not be selected.
       this.getParent()!.select();
       return;
     }
-    if (common.getSelected() === this) {
-      return;
-    }
-    let oldId = null;
-    if (common.getSelected()) {
-      oldId = common.getSelected()!.id;
-      // Unselect any previously selected block.
-      eventUtils.disable();
-      try {
-        common.getSelected()!.unselect();
-      } finally {
-        eventUtils.enable();
-      }
-    }
-    const event = new (eventUtils.get(eventUtils.SELECTED))(
-      oldId,
-      this.id,
-      this.workspace.id,
-    );
-    eventUtils.fire(event);
-    common.setSelected(this);
     this.addSelect();
   }
 
-  /**
-   * Unselects this block. Unhighlights the block and fires a select (false)
-   * event if the block is currently selected.
-   */
+  /** Unselects this block. Unhighlights the blockv visually.   */
   unselect() {
-    if (common.getSelected() !== this) {
-      return;
-    }
-    const event = new (eventUtils.get(eventUtils.SELECTED))(
-      this.id,
-      null,
-      this.workspace.id,
-    );
-    event.workspaceId = this.workspace.id;
-    eventUtils.fire(event);
-    common.setSelected(null);
     this.removeSelect();
   }
 

core/bubbles/bubble.ts

@@ -16,13 +16,16 @@ import {Rect} from '../utils/rect.js';
 import {Size} from '../utils/size.js';
 import {Svg} from '../utils/svg.js';
 import {WorkspaceSvg} from '../workspace_svg.js';
+import * as common from '../common.js';
+import {ISelectable} from '../blockly.js';
+import * as idGenerator from '../utils/idgenerator.js';
 
 /**
  * The abstract pop-up bubble class. This creates a UI that looks like a speech
  * bubble, where it has a "tail" that points to the block, and a "head" that
  * displays arbitrary svg elements.
  */
-export abstract class Bubble implements IBubble {
+export abstract class Bubble implements IBubble, ISelectable {
   /** The width of the border around the bubble. */
   static readonly BORDER_WIDTH = 6;
 
@@ -50,6 +53,8 @@ export abstract class Bubble implements IBubble {
   /** Distance between arrow point and anchor point. */
   static readonly ANCHOR_RADIUS = 8;
 
+  public id: string;
+
   /** The SVG group containing all parts of the bubble. */
   protected svgRoot: SVGGElement;
 
@@ -89,10 +94,11 @@ export abstract class Bubble implements IBubble {
    *     when automatically positioning.
    */
   constructor(
-    protected readonly workspace: WorkspaceSvg,
+    public readonly workspace: WorkspaceSvg,
     protected anchor: Coordinate,
     protected ownerRect?: Rect,
   ) {
+    this.id = idGenerator.getNextUniqueId();
     this.svgRoot = dom.createSvgElement(
       Svg.G,
       {'class': 'blocklyBubble'},
@@ -209,6 +215,7 @@ export abstract class Bubble implements IBubble {
   /** Passes the pointer event off to the gesture system. */
   private onMouseDown(e: PointerEvent) {
     this.workspace.getGesture(e)?.handleBubbleStart(e, this);
+    common.setSelected(this);
   }
 
   /** Positions the bubble relative to its anchor. Does not render its tail. */
@@ -640,4 +647,12 @@ export abstract class Bubble implements IBubble {
   revertDrag(): void {
     this.dragStrategy.revertDrag();
   }
+
+  select(): void {
+    // Bubbles don't have any visual for being selected.
+  }
+
+  unselect(): void {
+    // Bubbles don't have any visual for being selected.
+  }
 }

core/bubbles/mini_workspace_bubble.ts

@@ -47,7 +47,7 @@ export class MiniWorkspaceBubble extends Bubble {
   /** @internal */
   constructor(
     workspaceOptions: BlocklyOptions,
-    protected readonly workspace: WorkspaceSvg,
+    public readonly workspace: WorkspaceSvg,
     protected anchor: Coordinate,
     protected ownerRect?: Rect,
   ) {

core/bubbles/text_bubble.ts

@@ -20,7 +20,7 @@ export class TextBubble extends Bubble {
 
   constructor(
     private text: string,
-    protected readonly workspace: WorkspaceSvg,
+    public readonly workspace: WorkspaceSvg,
     protected anchor: Coordinate,
     protected ownerRect?: Rect,
   ) {

core/bubbles/textinput_bubble.ts

@@ -70,7 +70,7 @@ export class TextInputBubble extends Bubble {
    *     when automatically positioning.
    */
   constructor(
-    protected readonly workspace: WorkspaceSvg,
+    public readonly workspace: WorkspaceSvg,
     protected anchor: Coordinate,
     protected ownerRect?: Rect,
   ) {

core/clipboard/block_paster.ts

@@ -13,6 +13,7 @@ import {Coordinate} from '../utils/coordinate.js';
 import {WorkspaceSvg} from '../workspace_svg.js';
 import * as eventUtils from '../events/utils.js';
 import {config} from '../config.js';
+import * as common from '../common.js';
 
 export class BlockPaster implements IPaster<BlockCopyData, BlockSvg> {
   static TYPE = 'block';
@@ -43,7 +44,7 @@ export class BlockPaster implements IPaster<BlockCopyData, BlockSvg> {
     if (eventUtils.isEnabled() && !block.isShadow()) {
       eventUtils.fire(new (eventUtils.get(eventUtils.BLOCK_CREATE))(block));
     }
-    block.select();
+    common.setSelected(block);
     return block;
   }
 }

core/comments/rendered_workspace_comment.ts

@@ -16,10 +16,12 @@ import * as dom from '../utils/dom.js';
 import {IDraggable} from '../interfaces/i_draggable.js';
 import {CommentDragStrategy} from '../dragging/comment_drag_strategy.js';
 import * as browserEvents from '../browser_events.js';
+import * as common from '../common.js';
+import {ISelectable} from '../interfaces/i_selectable.js';
 
 export class RenderedWorkspaceComment
   extends WorkspaceComment
-  implements IBoundedElement, IRenderedElement, IDraggable
+  implements IBoundedElement, IRenderedElement, IDraggable, ISelectable
 {
   /** The class encompassing the svg elements making up the workspace comment. */
   private view: CommentView;
@@ -159,6 +161,7 @@ export class RenderedWorkspaceComment
     const gesture = this.workspace.getGesture(e);
     if (gesture) {
       gesture.handleCommentStart(e, this);
+      common.setSelected(this);
     }
   }
 
@@ -186,4 +189,8 @@ export class RenderedWorkspaceComment
   revertDrag(): void {
     this.dragStrategy.revertDrag();
   }
+
+  select(): void {}
+
+  unselect(): void {}
 }

core/common.ts

@@ -13,6 +13,7 @@ import {BlockDefinition, Blocks} from './blocks.js';
 import type {Connection} from './connection.js';
 import type {Workspace} from './workspace.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
+import * as eventUtils from './events/utils.js';
 
 /** Database of all workspaces. */
 const WorkspaceDB_ = Object.create(null);
@@ -105,7 +106,18 @@ export function getSelected(): ISelectable | null {
  * @internal
  */
 export function setSelected(newSelection: ISelectable | null) {
+  if (selected === newSelection) return;
+
+  const event = new (eventUtils.get(eventUtils.SELECTED))(
+    selected?.id ?? null,
+    newSelection?.id ?? null,
+    newSelection?.workspace.id ?? selected?.workspace.id ?? '',
+  );
+  eventUtils.fire(event);
+
+  selected?.unselect();
   selected = newSelection;
+  selected?.select();
 }
 
 /**

core/contextmenu.ts

@@ -29,6 +29,7 @@ import * as WidgetDiv from './widgetdiv.js';
 import {WorkspaceCommentSvg} from './workspace_comment_svg.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
 import * as Xml from './xml.js';
+import * as common from './common.js';
 
 /**
  * Which block is the context menu attached to?
@@ -261,7 +262,7 @@ export function callbackFactory(
     if (eventUtils.isEnabled() && !newBlock.isShadow()) {
       eventUtils.fire(new (eventUtils.get(eventUtils.BLOCK_CREATE))(newBlock));
     }
-    newBlock.select();
+    common.setSelected(newBlock);
     return newBlock;
   };
 }
@@ -374,7 +375,7 @@ export function workspaceCommentOption(
     if (ws.rendered) {
       comment.initSvg();
       comment.render();
-      comment.select();
+      common.setSelected(comment);
     }
   }
 

core/dragging/block_drag_strategy.ts

@@ -65,7 +65,10 @@ export class BlockDragStrategy implements IDragStrategy {
       this.block.isOwnMovable() &&
       !this.block.isShadow() &&
       !this.block.isDeadOrDying() &&
-      !this.workspace.options.readOnly
+      !this.workspace.options.readOnly &&
+      // We never drag blocks in the flyout, only create new blocks that are
+      // dragged.
+      !this.block.isInFlyout
     );
   }
 

core/gesture.ts

@@ -30,13 +30,12 @@ import * as internalConstants from './internal_constants.js';
 import * as Tooltip from './tooltip.js';
 import * as Touch from './touch.js';
 import {Coordinate} from './utils/coordinate.js';
-import {WorkspaceCommentSvg} from './workspace_comment_svg.js';
 import {WorkspaceDragger} from './workspace_dragger.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
 import type {IIcon} from './interfaces/i_icon.js';
 import {IDragger} from './interfaces/i_dragger.js';
 import * as registry from './registry.js';
-import {IDraggable} from './interfaces/i_draggable.js';
+import {IDraggable, isDraggable} from './interfaces/i_draggable.js';
 import {RenderedWorkspaceComment} from './comments.js';
 
 /**
@@ -298,71 +297,7 @@ export class Gesture {
       // The start block is no longer relevant, because this is a drag.
       this.startBlock = null;
       this.targetBlock = this.flyout.createBlock(this.targetBlock);
-      this.targetBlock.select();
-      return true;
-    }
-    return false;
-  }
-
-  /**
-   * Update this gesture to record whether a bubble is being dragged.
-   * This function should be called on a pointermove event the first time
-   * the drag radius is exceeded.  It should be called no more than once per
-   * gesture. If a bubble should be dragged this function creates the necessary
-   * BubbleDragger and starts the drag.
-   *
-   * @returns True if a bubble is being dragged.
-   */
-  private updateIsDraggingBubble(e: PointerEvent): boolean {
-    if (!this.startBubble) {
-      return false;
-    }
-
-    this.startDraggingBubble(e);
-    return true;
-  }
-
-  /**
-   * Update this gesture to record whether a comment is being dragged.
-   * This function should be called on a pointermove event the first time
-   * the drag radius is exceeded.  It should be called no more than once per
-   * gesture.
-   *
-   * @returns True if a comment is being dragged.
-   */
-  private updateIsDraggingComment(e: PointerEvent): boolean {
-    if (!this.startComment) {
-      return false;
-    }
-
-    this.startDraggingComment(e);
-    return true;
-  }
-
-  /**
-   * Check whether to start a block drag. If a block should be dragged, either
-   * from the flyout or in the workspace, create the necessary BlockDragger and
-   * start the drag.
-   *
-   * This function should be called on a pointermove event the first time
-   * the drag radius is exceeded.  It should be called no more than once per
-   * gesture. If a block should be dragged, either from the flyout or in the
-   * workspace, this function creates the necessary BlockDragger and starts the
-   * drag.
-   *
-   * @returns True if a block is being dragged.
-   */
-  private updateIsDraggingBlock(e: PointerEvent): boolean {
-    if (!this.targetBlock) {
-      return false;
-    }
-    if (this.flyout) {
-      if (this.updateIsDraggingFromFlyout()) {
-        this.startDraggingBlock(e);
-        return true;
-      }
-    } else if (this.targetBlock.isMovable()) {
-      this.startDraggingBlock(e);
+      common.setSelected(this.targetBlock);
       return true;
     }
     return false;
@@ -403,76 +338,30 @@ export class Gesture {
    * gesture.
    */
   private updateIsDragging(e: PointerEvent) {
-    // Sanity check.
+    if (!this.startWorkspace_) {
+      throw new Error(
+        'Cannot update dragging because the start workspace is undefined',
+      );
+    }
+
     if (this.calledUpdateIsDragging) {
       throw Error('updateIsDragging_ should only be called once per gesture.');
     }
     this.calledUpdateIsDragging = true;
 
-    // First check if it was a bubble drag.  Bubbles always sit on top of
-    // blocks.
-    if (this.updateIsDraggingBubble(e)) {
-      return;
-    }
-    // Then check if it was a block drag.
-    if (this.updateIsDraggingBlock(e)) {
-      return;
-    }
-    if (this.updateIsDraggingComment(e)) {
-      return;
-    }
-    // Then check if it's a workspace drag.
+    // If we drag a block out of the flyout, it updates `common.getSelected`
+    // to return the new block.
+    if (this.flyout) this.updateIsDraggingFromFlyout();
+
+    const selected = common.getSelected();
+    if (selected && isDraggable(selected) && selected.isMovable()) {
+      this.dragging = true;
+      this.dragger = this.createDragger(selected, this.startWorkspace_);
+      this.dragger.onDragStart(e);
+      this.dragger.onDrag(e, this.currentDragDeltaXY);
+    } else {
       this.updateIsDraggingWorkspace();
     }
-
-  /** Start dragging the selected block. */
-  private startDraggingBlock(e: PointerEvent) {
-    this.dragging = true;
-    this.dragger = this.createDragger(this.targetBlock!, this.startWorkspace_!);
-    this.dragger.onDragStart(e);
-    this.dragger.onDrag(e, this.currentDragDeltaXY);
-  }
-
-  /** Start dragging the selected bubble. */
-  private startDraggingBubble(e: PointerEvent) {
-    if (!this.startBubble) {
-      throw new Error(
-        'Cannot update dragging the bubble because the start ' +
-          'bubble is undefined',
-      );
-    }
-    if (!this.startWorkspace_) {
-      throw new Error(
-        'Cannot update dragging the bubble because the start ' +
-          'workspace is undefined',
-      );
-    }
-
-    this.dragging = true;
-    this.dragger = this.createDragger(this.startBubble, this.startWorkspace_);
-    this.dragger.onDragStart(e);
-    this.dragger.onDrag(e, this.currentDragDeltaXY);
-  }
-
-  /** Start dragging the selected comment. */
-  private startDraggingComment(e: PointerEvent) {
-    if (!this.startComment) {
-      throw new Error(
-        'Cannot update dragging the comment because the start ' +
-          'comment is undefined',
-      );
-    }
-    if (!this.startWorkspace_) {
-      throw new Error(
-        'Cannot update dragging the comment because the start ' +
-          'workspace is undefined',
-      );
-    }
-
-    this.dragging = true;
-    this.dragger = this.createDragger(this.startComment, this.startWorkspace_);
-    this.dragger.onDragStart(e);
-    this.dragger.onDrag(e, this.currentDragDeltaXY);
   }
 
   private createDragger(
@@ -532,10 +421,6 @@ export class Gesture {
 
     Tooltip.block();
 
-    if (this.targetBlock) {
-      this.targetBlock.select();
-    }
-
     if (browserEvents.isRightButton(e)) {
       this.handleRightClick(e);
       return;
@@ -668,8 +553,7 @@ export class Gesture {
       } else if (this.workspaceDragger) {
         this.workspaceDragger.endDrag(this.currentDragDeltaXY);
       } else if (this.isBubbleClick()) {
-        // Bubbles are in front of all fields and blocks.
-        this.doBubbleClick();
+        // Do nothing, bubbles don't currently respond to clicks.
       } else if (this.isFieldClick()) {
         this.doFieldClick();
       } else if (this.isIconClick()) {
@@ -873,6 +757,13 @@ export class Gesture {
     }
     this.setStartWorkspace(ws);
     this.mostRecentEvent = e;
+
+    if (!this.startBlock && !this.startBubble && !this.startComment) {
+      // Selection determines what things start drags. So to drag the workspace,
+      // we need to deselect anything that was previously selected.
+      common.setSelected(null);
+    }
+
     this.doStart(e);
   }
 
@@ -963,15 +854,6 @@ export class Gesture {
    * type of target.  Any developer wanting to add behaviour on clicks should
    * modify only this code. */
 
-  /** Execute a bubble click. */
-  private doBubbleClick() {
-    // TODO (#1673): Consistent handling of single clicks.
-    if (this.startBubble instanceof WorkspaceCommentSvg) {
-      this.startBubble.setFocus();
-      this.startBubble.select();
-    }
-  }
-
   /** Execute a field click. */
   private doFieldClick() {
     if (!this.startField) {
@@ -1138,6 +1020,7 @@ export class Gesture {
     // If the gesture already went through a bubble, don't set the start block.
     if (!this.startBlock && !this.startBubble) {
       this.startBlock = block;
+      common.setSelected(this.startBlock);
       if (block.isInFlyout && block !== block.getRootBlock()) {
         this.setTargetBlock(block.getRootBlock());
       } else {

core/interfaces/i_selectable.ts

@@ -6,12 +6,16 @@
 
 // Former goog.module ID: Blockly.ISelectable
 
+import type {Workspace} from '../workspace.js';
+
 /**
  * The interface for an object that is selectable.
  */
 export interface ISelectable {
   id: string;
 
+  workspace: Workspace;
+
   /** Select this.  Highlight it visually. */
   select(): void;
 
@@ -23,6 +27,7 @@ export interface ISelectable {
 export function isSelectable(obj: Object): obj is ISelectable {
   return (
     typeof (obj as any).id === 'string' &&
+    (obj as any).workspace !== undefined &&
     (obj as any).select !== undefined &&
     (obj as any).unselect !== undefined
   );

tests/mocha/comment_deserialization_test.js

@@ -74,9 +74,7 @@ suite('Comment Deserialization', function () {
       simulateClick(this.workspace.trashcan.svgGroup);
       // Place from trashcan.
       simulateClick(
-        this.workspace.trashcan.flyout.svgGroup_.querySelector(
-          '.blocklyDraggable',
-        ),
+        this.workspace.trashcan.flyout.svgGroup_.querySelector('.blocklyPath'),
       );
       chai.assert.equal(this.workspace.getAllBlocks().length, 1);
       // Check comment.
@@ -113,7 +111,7 @@ suite('Comment Deserialization', function () {
       const toolbox = this.workspace.getToolbox();
       simulateClick(toolbox.HtmlDiv.querySelector('.blocklyTreeRow'));
       simulateClick(
-        toolbox.getFlyout().svgGroup_.querySelector('.blocklyDraggable'),
+        toolbox.getFlyout().svgGroup_.querySelector('.blocklyPath'),
       );
       chai.assert.equal(this.workspace.getAllBlocks().length, 1);
       // Check comment.