fix: collapsing blocks with children with icons (#7111)

* fix: collapsing blocks with icons * chore: add tests for hiding icons of collapsed children
google · May 22, 2023 · 6e3d052 · 6e3d052
1 parent d90d005
commit 6e3d052
Show file tree

Hide file tree

Showing 6 changed files with 138 additions and 133 deletions.
diff --git a/core/interfaces/i_has_bubble.ts b/core/interfaces/i_has_bubble.ts
@@ -6,7 +6,7 @@
 
 export interface IHasBubble {
   /** @return True if the bubble is currently open, false otherwise. */
-  isBubbleVisible(): boolean;
+  bubbleIsVisible(): boolean;
 
   /** Sets whether the bubble is open or not. */
   setBubbleVisible(visible: boolean): void;
@@ -15,6 +15,6 @@ export interface IHasBubble {
 /** Type guard that checks whether the given object is a IHasBubble. */
 export function hasBubble(obj: any): obj is IHasBubble {
   return (
-    obj.isBubbleVisible !== undefined && obj.setBubbleVisible !== undefined
+    obj.bubbleIsVisible !== undefined && obj.setBubbleVisible !== undefined
   );
 }
diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts
@@ -20,6 +20,7 @@ import {Connection} from './connection.js';
 import type {ConnectionDB} from './connection_db.js';
 import {ConnectionType} from './connection_type.js';
 import * as eventUtils from './events/utils.js';
+import {hasBubble} from './interfaces/i_has_bubble.js';
 import * as internalConstants from './internal_constants.js';
 import {Coordinate} from './utils/coordinate.js';
 import * as dom from './utils/dom.js';
@@ -428,9 +429,13 @@ export class RenderedConnection extends Connection {
           connections[j].setTracking(false);
         }
         // Close all bubbles of all children.
-        const icons = block.getIcons();
-        for (let j = 0; j < icons.length; j++) {
-          icons[j].setVisible(false);
+        for (const icon of block.getIcons()) {
+          if (hasBubble(icon)) {
+            icon.setBubbleVisible(false);
+          } else if (icon.setVisible) {
+            // TODO (#7042): Remove old icon handling code.
+            icon.setVisible(false);
+          }
         }
       }
     }

diff --git a/core/renderers/measurables/icon.ts b/core/renderers/measurables/icon.ts
@@ -41,7 +41,7 @@ export class Icon extends Measurable {
     // TODO(#7042): Remove references to old icon API.
     this.isVisible =
       (icon.isVisible && icon.isVisible()) ||
-      (hasBubble(icon) && icon.isBubbleVisible());
+      (hasBubble(icon) && icon.bubbleIsVisible());
     this.type |= Types.ICON;
 
     const size =

diff --git a/tests/mocha/block_test.js b/tests/mocha/block_test.js
@@ -19,6 +19,7 @@ import {
   createChangeListenerSpy,
   createMockEvent,
 } from './test_helpers/events.js';
+import {MockIcon, MockBubbleIcon} from './test_helpers/icon_mocks.js';
 
 suite('Blocks', function () {
   setup(function () {
@@ -1418,38 +1419,6 @@ suite('Blocks', function () {
   });
 
   suite('Icon management', function () {
-    class MockIcon {
-      getType() {
-        return 'mock icon';
-      }
-
-      initView() {}
-
-      dispose() {}
-
-      getWeight() {}
-
-      getSize() {
-        return new Blockly.utils.Size(0, 0);
-      }
-
-      applyColour() {}
-
-      hideForInsertionMarker() {}
-
-      updateEditable() {}
-
-      updateCollapsed() {}
-
-      isShownWhenCollapsed() {}
-
-      setOffsetInBlock() {}
-
-      onLocationChange() {}
-
-      onClick() {}
-    }
-
     class MockIconA extends MockIcon {
       getType() {
         return 'A';
@@ -1624,54 +1593,58 @@ suite('Blocks', function () {
         workspaceTeardown.call(this, this.workspace);
       });
 
-      test('Has Icon', function () {
-        const block = Blockly.Xml.domToBlock(
-          Blockly.utils.xml.textToDom('<block type="statement_block"/>'),
+      test("Collapsing the block closes its contained children's bubbles", function () {
+        const parentBlock = Blockly.serialization.blocks.append(
+          {
+            'type': 'statement_block',
+            'inputs': {
+              'STATEMENT': {
+                'block': {
+                  'type': 'statement_block',
+                },
+              },
+            },
+          },
           this.workspace
         );
-        block.setCommentText('test text');
-        block.comment.setVisible(true);
-        chai.assert.isTrue(block.comment.isVisible());
-        block.setCollapsed(true);
-        chai.assert.isFalse(block.comment.isVisible());
-      });
+        const childBlock = parentBlock.getInputTargetBlock('STATEMENT');
+        const icon = new MockBubbleIcon();
+        childBlock.addIcon(icon);
+        icon.setBubbleVisible(true);
 
-      test('Child Has Icon', function () {
-        const block = Blockly.Xml.domToBlock(
-          Blockly.utils.xml.textToDom(
-            '<block type="statement_block">' +
-              '  <statement name="STATEMENT">' +
-              '    <block type="statement_block"/>' +
-              '  </statement>' +
-              '</block>'
-          ),
-          this.workspace
+        parentBlock.setCollapsed(true);
+
+        chai.assert.isFalse(
+          icon.bubbleIsVisible(),
+          "Expected collapsing the parent block to hide the child block's " +
+            "icon's bubble"
         );
-        const childBlock = block.getInputTargetBlock('STATEMENT');
-        childBlock.setCommentText('test text');
-        childBlock.comment.setVisible(true);
-        chai.assert.isTrue(childBlock.comment.isVisible());
-        block.setCollapsed(true);
-        chai.assert.isFalse(childBlock.comment.isVisible());
       });
 
-      test('Next Block Has Icon', function () {
-        const block = Blockly.Xml.domToBlock(
-          Blockly.utils.xml.textToDom(
-            '<block type="statement_block">' +
-              '  <next>' +
-              '    <block type="statement_block"/>' +
-              '  </next>' +
-              '</block>'
-          ),
+      test("Collapsing a block does not close its following childrens' bubbles", function () {
+        const parentBlock = Blockly.serialization.blocks.append(
+          {
+            'type': 'statement_block',
+            'next': {
+              'block': {
+                'type': 'statement_block',
+              },
+            },
+          },
           this.workspace
         );
-        const nextBlock = block.getNextBlock();
-        nextBlock.setCommentText('test text');
-        nextBlock.comment.setVisible(true);
-        chai.assert.isTrue(nextBlock.comment.isVisible());
-        block.setCollapsed(true);
-        chai.assert.isTrue(nextBlock.comment.isVisible());
+        const nextBlock = parentBlock.getNextBlock();
+        const icon = new MockBubbleIcon();
+        nextBlock.addIcon(icon);
+        icon.setBubbleVisible(true);
+
+        parentBlock.setCollapsed(true);
+
+        chai.assert.isTrue(
+          icon.bubbleIsVisible(),
+          'Expected collapsing the parent block to not hide the next ' +
+            "block's bubble"
+        );
       });
     });
   });

diff --git a/tests/mocha/icon_test.js b/tests/mocha/icon_test.js
@@ -11,6 +11,7 @@ import {
   sharedTestTeardown,
 } from './test_helpers/setup_teardown.js';
 import {defineEmptyBlock} from './test_helpers/block_definitions.js';
+import {MockIcon, MockSerializableIcon} from './test_helpers/icon_mocks.js';
 
 suite('Icon', function () {
   setup(function () {
@@ -22,61 +23,6 @@ suite('Icon', function () {
     sharedTestTeardown.call(this);
   });
 
-  class MockIcon {
-    getType() {
-      return 'mock icon';
-    }
-
-    initView() {}
-
-    dispose() {}
-
-    getWeight() {}
-
-    getSize() {
-      return new Blockly.utils.Size(0, 0);
-    }
-
-    applyColour() {}
-
-    hideForInsertionMarker() {}
-
-    updateEditable() {}
-
-    updateCollapsed() {}
-
-    isShownWhenCollapsed() {}
-
-    setOffsetInBlock() {}
-
-    onLocationChange() {}
-
-    onClick() {}
-  }
-
-  class MockSerializableIcon extends MockIcon {
-    constructor() {
-      super();
-      this.state = '';
-    }
-
-    getType() {
-      return 'serializable icon';
-    }
-
-    getWeight() {
-      return 1;
-    }
-
-    saveState() {
-      return 'some state';
-    }
-
-    loadState(state) {
-      this.state = state;
-    }
-  }
-
   class MockNonSerializableIcon extends MockIcon {
     getType() {
       return 'non-serializable icon';

diff --git a/tests/mocha/test_helpers/icon_mocks.js b/tests/mocha/test_helpers/icon_mocks.js
@@ -0,0 +1,81 @@
+/**
+ * @license
+ * Copyright 2023 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+export class MockIcon {
+  getType() {
+    return 'mock icon';
+  }
+
+  initView() {}
+
+  dispose() {}
+
+  getWeight() {}
+
+  getSize() {
+    return new Blockly.utils.Size(0, 0);
+  }
+
+  applyColour() {}
+
+  hideForInsertionMarker() {}
+
+  updateEditable() {}
+
+  updateCollapsed() {}
+
+  isShownWhenCollapsed() {}
+
+  setOffsetInBlock() {}
+
+  onLocationChange() {}
+
+  onClick() {}
+}
+
+export class MockSerializableIcon extends MockIcon {
+  constructor() {
+    super();
+    this.state = '';
+  }
+
+  getType() {
+    return 'serializable icon';
+  }
+
+  getWeight() {
+    return 1;
+  }
+
+  saveState() {
+    return 'some state';
+  }
+
+  loadState(state) {
+    this.state = state;
+  }
+}
+
+export class MockBubbleIcon extends MockIcon {
+  constructor() {
+    super();
+    this.visible = false;
+  }
+
+  getType() {
+    return 'bubble icon';
+  }
+
+  updateCollapsed() {}
+
+  bubbleIsVisible() {
+    return this.visible;
+  }
+
+  setBubbleVisible(visible) {
+    this.visible = visible;
+  }
+}