diff --git a/core/block.ts b/core/block.ts index 4d22b1886..c8932277f 100644 --- a/core/block.ts +++ b/core/block.ts @@ -312,44 +312,45 @@ export class Block implements IASTNodeLocation, IDeletable { * @suppress {checkTypes} */ dispose(healStack: boolean) { - if (this.isDeadOrDying()) { - return; - } - // Terminate onchange event calls. + if (this.isDeadOrDying()) return; + + // Dispose of this change listener before unplugging. + // Technically not necessary due to the event firing delay. + // But future-proofing. if (this.onchangeWrapper_) { this.workspace.removeChangeListener(this.onchangeWrapper_); } this.unplug(healStack); if (eventUtils.isEnabled()) { + // Constructing the delete event is costly. Only perform if necessary. eventUtils.fire(new (eventUtils.get(eventUtils.BLOCK_DELETE))(this)); } - eventUtils.disable(); + this.workspace.removeTopBlock(this); + this.disposeInternal(); + } + /** + * Disposes of this block without doing things required by the top block. + * E.g. does not fire events, unplug the block, etc. + */ + protected disposeInternal() { + if (this.isDeadOrDying()) return; + + if (this.onchangeWrapper_) { + this.workspace.removeChangeListener(this.onchangeWrapper_); + } + + eventUtils.disable(); try { - // This block is now at the top of the workspace. - // Remove this block from the workspace's list of top-most blocks. - this.workspace.removeTopBlock(this); this.workspace.removeTypedBlock(this); - // Remove from block database. this.workspace.removeBlockById(this.id); this.disposing = true; - // First, dispose of all my children. - for (let i = this.childBlocks_.length - 1; i >= 0; i--) { - this.childBlocks_[i].dispose(false); - } - // Then dispose of myself. - // Dispose of all inputs and their fields. - for (let i = 0, input; input = this.inputList[i]; i++) { - input.dispose(); - } + this.childBlocks_.forEach((c) => c.disposeInternal()); + this.inputList.forEach((i) => i.dispose()); this.inputList.length = 0; - // Dispose of any remaining connections (next/previous/output). - const connections = this.getConnections_(true); - for (let i = 0, connection; connection = connections[i]; i++) { - connection.dispose(); - } + this.getConnections_(true).forEach((c) => c.dispose()); } finally { eventUtils.enable(); if (typeof this.destroy === 'function') { diff --git a/core/block_animations.ts b/core/block_animations.ts index 9378498a9..065b4ee51 100644 --- a/core/block_animations.ts +++ b/core/block_animations.ts @@ -39,6 +39,9 @@ let wobblingBlock: BlockSvg|null = null; * @internal */ export function disposeUiEffect(block: BlockSvg) { + // Disposing is going to take so long the animation won't play anyway. + if (block.getDescendants(false).length > 100) return; + const workspace = block.workspace; const svgGroup = block.getSvgRoot(); workspace.getAudioManager().play('delete'); diff --git a/core/block_svg.ts b/core/block_svg.ts index 4f3ec2844..b463c64c6 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -841,55 +841,39 @@ export class BlockSvg extends Block implements IASTNodeLocationSvg, * @suppress {checkTypes} */ override dispose(healStack?: boolean, animate?: boolean) { - if (this.isDeadOrDying()) { - return; - } + if (this.isDeadOrDying()) return; + Tooltip.dispose(); - Tooltip.unbindMouseEvents(this.pathObject.svgPath); - dom.startTextWidthCache(); - // Save the block's workspace temporarily so we can resize the - // contents once the block is disposed. - const blockWorkspace = this.workspace; - // If this block is being dragged, unlink the mouse events. - if (common.getSelected() === this) { - this.unselect(); - this.workspace.cancelCurrentGesture(); - } - // If this block has a context menu open, close it. - if (ContextMenu.getCurrentBlock() === this) { - ContextMenu.hide(); - } + ContextMenu.hide(); if (animate && this.rendered) { this.unplug(healStack); blockAnimations.disposeUiEffect(this); } - // Stop rerendering. - this.rendered = false; - - // Clear pending warnings. - for (const n of this.warningTextDb.values()) { - clearTimeout(n); - } - this.warningTextDb.clear(); - - const icons = this.getIcons(); - for (let i = 0; i < icons.length; i++) { - icons[i].dispose(); - } - - // Just deleting this block from the DOM would result in a memory leak as - // well as corruption of the connection database. Therefore we must - // methodically step through the blocks and carefully disassemble them. - if (common.getSelected() === this) { - common.setSelected(null); - } super.dispose(!!healStack); - dom.removeNode(this.svgGroup_); - blockWorkspace.resizeContents(); - dom.stopTextWidthCache(); + } + + /** + * Disposes of this block without doing things required by the top block. + * E.g. does trigger UI effects, remove nodes, etc. + */ + override disposeInternal() { + if (this.isDeadOrDying()) return; + super.disposeInternal(); + + this.rendered = false; + + if (common.getSelected() === this) { + this.unselect(); + this.workspace.cancelCurrentGesture(); + } + + [...this.warningTextDb.values()].forEach((n) => clearTimeout(n)); + this.warningTextDb.clear(); + + this.getIcons().forEach((i) => i.dispose()); } /** diff --git a/core/connection.ts b/core/connection.ts index 485f3895e..d6e046b3b 100644 --- a/core/connection.ts +++ b/core/connection.ts @@ -150,7 +150,7 @@ export class Connection implements IASTNodeLocationWithBlock { this.setShadowStateInternal_(); const targetBlock = this.targetBlock(); - if (targetBlock) { + if (targetBlock && !targetBlock.isDeadOrDying()) { // Disconnect the attached normal block. targetBlock.unplug(); } @@ -552,6 +552,7 @@ export class Connection implements IASTNodeLocationWithBlock { } } else if (target.isShadow()) { target.dispose(false); + if (this.getSourceBlock().isDeadOrDying()) return; this.respawnShadow_(); if (this.targetBlock() && this.targetBlock()!.isShadow()) { this.serializeShadow_(this.targetBlock()); diff --git a/core/field.ts b/core/field.ts index 0c163e038..ff5663599 100644 --- a/core/field.ts +++ b/core/field.ts @@ -489,14 +489,11 @@ export abstract class Field implements IASTNodeLocationSvg, dispose() { dropDownDiv.hideIfOwner(this); WidgetDiv.hideIfOwner(this); - Tooltip.unbindMouseEvents(this.getClickTarget_()); - if (this.mouseDownWrapper_) { - browserEvents.unbind(this.mouseDownWrapper_); + if (!this.getSourceBlock()?.isDeadOrDying()) { + dom.removeNode(this.fieldGroup_); } - dom.removeNode(this.fieldGroup_); - this.disposed = true; } diff --git a/core/icon.ts b/core/icon.ts index 791f835d2..5d8002165 100644 --- a/core/icon.ts +++ b/core/icon.ts @@ -79,8 +79,10 @@ export abstract class Icon { /** Dispose of this icon. */ dispose() { - dom.removeNode(this.iconGroup_); // Dispose of and unlink the icon. - this.setVisible(false); // Dispose of and unlink the bubble. + if (!this.getBlock().isDeadOrDying()) { + dom.removeNode(this.iconGroup_); + } + this.setVisible(false); // Dispose of and unlink the bubble. } /** Add or remove the UI indicating if this icon may be clicked or not. */ diff --git a/core/tooltip.ts b/core/tooltip.ts index 50a2a1e2a..e907561a4 100644 --- a/core/tooltip.ts +++ b/core/tooltip.ts @@ -276,6 +276,7 @@ function onMouseOut(_e: PointerEvent) { hide(); }, 1); clearTimeout(showPid); + showPid = 0; } /** @@ -342,6 +343,7 @@ export function hide() { } if (showPid) { clearTimeout(showPid); + showPid = 0; } } diff --git a/tests/mocha/connection_checker_test.js b/tests/mocha/connection_checker_test.js index 7636d7af3..11b9545c1 100644 --- a/tests/mocha/connection_checker_test.js +++ b/tests/mocha/connection_checker_test.js @@ -386,7 +386,7 @@ suite('Connection checker', function() { }); test('Connect to unconnected unmovable block', function() { - // Delete blockA. + this.blockB.previousConnection.disconnect(); this.blockA.dispose(); // Try to connect blockC above blockB. @@ -436,7 +436,7 @@ suite('Connection checker', function() { }); test('Connect to unconnected unmovable block', function() { - // Delete blockA + this.blockB.outputConnection.disconnect(); this.blockA.dispose(); // Try to connect C's input to B's output. Allowed because B is now unconnected. diff --git a/tests/mocha/event_block_delete_test.js b/tests/mocha/event_block_delete_test.js index e5aed21c5..0a9c9c894 100644 --- a/tests/mocha/event_block_delete_test.js +++ b/tests/mocha/event_block_delete_test.js @@ -6,13 +6,12 @@ goog.declareModuleId('Blockly.test.eventBlockDelete'); -import * as eventUtils from '../../build/src/core/events/utils.js'; import {defineRowBlock} from './test_helpers/block_definitions.js'; import {sharedTestSetup, sharedTestTeardown} from './test_helpers/setup_teardown.js'; suite('Block Delete Event', function() { setup(function() { - sharedTestSetup.call(this); + this.clock = sharedTestSetup.call(this, {fireEventsNow: false}).clock; defineRowBlock(); this.workspace = new Blockly.Workspace(); }); @@ -21,6 +20,23 @@ suite('Block Delete Event', function() { sharedTestTeardown.call(this); }); + suite('Receiving', function() { + test('blocks do not receive their own delete events', function() { + Blockly.Blocks['test'] = { + onchange: function(e) { }, + }; + // Need to stub the definition, because the property on the definition is + // what gets registered as an event listener. + const spy = sinon.spy(Blockly.Blocks['test'], 'onchange'); + const testBlock = this.workspace.newBlock('test'); + + testBlock.dispose(); + this.clock.runAll(); + + chai.assert.isFalse(spy.called); + }); + }); + suite('Serialization', function() { test('events round-trip through JSON', function() { const block = this.workspace.newBlock('row_block', 'block_id'); diff --git a/tests/mocha/serializer_test.js b/tests/mocha/serializer_test.js index 0a09b1725..d45cd40c3 100644 --- a/tests/mocha/serializer_test.js +++ b/tests/mocha/serializer_test.js @@ -1866,7 +1866,7 @@ const runSerializerTestSuite = (serializer, deserializer, testSuite) => { suiteCall(testSuite.title, function() { setup(function() { - sharedTestSetup.call(this); + sharedTestSetup.call(this, {fireEventsNow: false}); this.workspace = new Blockly.Workspace(); });