fix: Display dragged comments and bubbles atop the toolbox (#9552)

* fix: Display dragged comments and bubbles atop the toolbox

* test: Add tests for drag layer behavior

Author: gonfunko

core/comments/rendered_workspace_comment.ts

@@ -23,7 +23,6 @@ import {IFocusableNode} from '../interfaces/i_focusable_node.js';
 import type {IFocusableTree} from '../interfaces/i_focusable_tree.js';
 import {IRenderedElement} from '../interfaces/i_rendered_element.js';
 import {ISelectable} from '../interfaces/i_selectable.js';
-import * as layers from '../layers.js';
 import * as commentSerialization from '../serialization/workspace_comments.js';
 import {Coordinate} from '../utils/coordinate.js';
 import * as dom from '../utils/dom.js';
@@ -346,7 +345,7 @@ export class RenderedWorkspaceComment
   onNodeFocus(): void {
     this.select();
     // Ensure that the comment is always at the top when focused.
-    this.workspace.getLayerManager()?.append(this, layers.BLOCK);
+    this.getSvgRoot().parentElement?.appendChild(this.getSvgRoot());
     this.workspace.scrollBoundsIntoView(this.getBoundingRectangle());
   }
 

core/dragging/block_drag_strategy.ts

@@ -14,8 +14,10 @@ import {ConnectionType} from '../connection_type.js';
 import type {BlockMove} from '../events/events_block_move.js';
 import {EventType} from '../events/type.js';
 import * as eventUtils from '../events/utils.js';
+import type {IBubble} from '../interfaces/i_bubble.js';
 import {IConnectionPreviewer} from '../interfaces/i_connection_previewer.js';
 import {IDragStrategy} from '../interfaces/i_draggable.js';
+import {IHasBubble, hasBubble} from '../interfaces/i_has_bubble.js';
 import * as layers from '../layers.js';
 import * as registry from '../registry.js';
 import {finishQueuedRenders} from '../render_management.js';
@@ -120,6 +122,34 @@ export class BlockDragStrategy implements IDragStrategy {
     }
     this.block.setDragging(true);
     this.workspace.getLayerManager()?.moveToDragLayer(this.block);
+    this.getVisibleBubbles(this.block).forEach((bubble) => {
+      this.workspace.getLayerManager()?.moveToDragLayer(bubble, false);
+    });
+  }
+
+  /**
+   * Returns an array of visible bubbles attached to the given block or its
+   * descendants.
+   *
+   * @param block The block to identify open bubbles on.
+   * @returns An array of all currently visible bubbles on the given block or
+   *    its descendants.
+   */
+  private getVisibleBubbles(block: BlockSvg): IBubble[] {
+    return block
+      .getDescendants(false)
+      .flatMap((block) => block.getIcons())
+      .filter((icon) => hasBubble(icon) && icon.bubbleIsVisible())
+      .map((icon) => (icon as unknown as IHasBubble).getBubble())
+      .filter((bubble) => !!bubble) // Convince TS they're non-null.
+      .sort((a, b) => {
+        // Sort the bubbles by their position in the DOM in order to maintain
+        // their relative z-ordering when moving between layers.
+        const position = a.getSvgRoot().compareDocumentPosition(b.getSvgRoot());
+        if (position & Node.DOCUMENT_POSITION_PRECEDING) return 1;
+        if (position & Node.DOCUMENT_POSITION_FOLLOWING) return -1;
+        return 0;
+      });
   }
 
   /**
@@ -393,6 +423,13 @@ export class BlockDragStrategy implements IDragStrategy {
       this.workspace
         .getLayerManager()
         ?.moveOffDragLayer(this.block, layers.BLOCK);
+
+      this.getVisibleBubbles(this.block).forEach((bubble) =>
+        this.workspace
+          .getLayerManager()
+          ?.moveOffDragLayer(bubble, layers.BUBBLE, false),
+      );
+
       this.block.setDragging(false);
     }
 
@@ -462,6 +499,12 @@ export class BlockDragStrategy implements IDragStrategy {
       this.workspace
         .getLayerManager()
         ?.moveOffDragLayer(this.block, layers.BLOCK);
+      this.getVisibleBubbles(this.block).forEach((bubble) =>
+        this.workspace
+          .getLayerManager()
+          ?.moveOffDragLayer(bubble, layers.BUBBLE, false),
+      );
+
       // Blocks dragged directly from a flyout may need to be bumped into
       // bounds.
       bumpObjects.bumpIntoBounds(

core/layer_manager.ts

@@ -99,12 +99,15 @@ export class LayerManager {
    * Moves the given element to the drag layer, which exists on top of all other
    * layers, and the drag surface.
    *
+   * @param elem The element to move onto the drag layer.
+   * @param focus Whether or not to focus the element post-move.
+   *
    * @internal
    */
-  moveToDragLayer(elem: IRenderedElement & IFocusableNode) {
+  moveToDragLayer(elem: IRenderedElement & IFocusableNode, focus = true) {
     this.dragLayer?.appendChild(elem.getSvgRoot());
 
-    if (elem.canBeFocused()) {
+    if (focus && elem.canBeFocused()) {
       // Since moving the element to the drag layer will cause it to lose focus,
       // ensure it regains focus (to ensure proper highlights & sent events).
       getFocusManager().focusNode(elem);
@@ -114,12 +117,22 @@ export class LayerManager {
   /**
    * Moves the given element off of the drag layer.
    *
+   * @param elem The element to move off of the drag layer.
+   * @param layerNum The identifier of the layer to move the element onto.
+   *     Should be a constant from layers.ts.
+   * @param focus Whether or not the element should be focused once moved onto
+   *     the destination layer.
+   *
    * @internal
    */
-  moveOffDragLayer(elem: IRenderedElement & IFocusableNode, layerNum: number) {
+  moveOffDragLayer(
+    elem: IRenderedElement & IFocusableNode,
+    layerNum: number,
+    focus = true,
+  ) {
     this.append(elem, layerNum);
 
-    if (elem.canBeFocused()) {
+    if (focus && elem.canBeFocused()) {
       // Since moving the element off the drag layer will cause it to lose focus,
       // ensure it regains focus (to ensure proper highlights & sent events).
       getFocusManager().focusNode(elem);
@@ -202,4 +215,13 @@ export class LayerManager {
   getBubbleLayer(): SVGGElement {
     return this.layers.get(layerNums.BUBBLE)!;
   }
+
+  /**
+   * Returns the drag layer.
+   *
+   * @internal
+   */
+  getDragLayer(): SVGGElement | undefined {
+    return this.dragLayer;
+  }
 }

tests/mocha/block_test.js

@@ -2883,4 +2883,50 @@ suite('Blocks', function () {
       assert.equal(block.inputList[1].fieldRow[0].getValue(), 'Row2');
     });
   });
+
+  suite('Dragging', function () {
+    setup(function () {
+      this.workspace = Blockly.inject('blocklyDiv');
+      this.blocks = createTestBlocks(this.workspace, false);
+      for (const block of Object.values(this.blocks)) {
+        block.initSvg();
+        block.render();
+      }
+    });
+    test('Bubbles are moved to drag layer along with their blocks', async function () {
+      this.blocks.A.setCommentText('a');
+      this.blocks.B.setCommentText('b');
+      this.blocks.C.setCommentText('c');
+      const v1 = this.blocks.A.getIcon(
+        Blockly.icons.IconType.COMMENT,
+      ).setBubbleVisible(true);
+      const v2 = this.blocks.B.getIcon(
+        Blockly.icons.IconType.COMMENT,
+      ).setBubbleVisible(true);
+      const v3 = this.blocks.C.getIcon(
+        Blockly.icons.IconType.COMMENT,
+      ).setBubbleVisible(true);
+
+      this.clock.tick(1000);
+      await Promise.all([v1, v2, v3]);
+
+      this.blocks.B.startDrag();
+
+      // Block A stays put and should have its comment stay on the bubble layer.
+      assert.equal(
+        this.blocks.A.getIcon(Blockly.icons.IconType.COMMENT)
+          .getBubble()
+          .getSvgRoot().parentElement,
+        this.blocks.A.workspace.getLayerManager()?.getBubbleLayer(),
+      );
+
+      // Block B moves to the drag layer and its comment should follow.
+      assert.equal(
+        this.blocks.B.getIcon(Blockly.icons.IconType.COMMENT)
+          .getBubble()
+          .getSvgRoot().parentElement,
+        this.blocks.B.workspace.getLayerManager()?.getDragLayer(),
+      );
+    });
+  });
 });

tests/mocha/workspace_comment_test.js

@@ -168,8 +168,10 @@ suite('Workspace comment', function () {
         this.workspace.id,
       );
     });
+  });
 
-    test('focuses the workspace when deleted', function () {
+  suite('Focus', function () {
+    test('moves to the workspace when deleted', function () {
       const comment = new Blockly.comments.RenderedWorkspaceComment(
         this.workspace,
       );
@@ -178,5 +180,18 @@ suite('Workspace comment', function () {
       comment.view.getCommentBarButtons()[1].performAction();
       assert.equal(Blockly.getFocusManager().getFocusedNode(), this.workspace);
     });
+
+    test('does not change the layer', function () {
+      const comment = new Blockly.comments.RenderedWorkspaceComment(
+        this.workspace,
+      );
+
+      this.workspace.getLayerManager()?.moveToDragLayer(comment);
+      Blockly.getFocusManager().focusNode(comment);
+      assert.equal(
+        comment.getSvgRoot().parentElement,
+        this.workspace.getLayerManager()?.getDragLayer(),
+      );
+    });
   });
 });