From 2b22237560b2da5e8aa48a5956eba1a872f4997f Mon Sep 17 00:00:00 2001 From: joeclinton1 Date: Sat, 25 Apr 2026 00:58:55 +0100 Subject: [PATCH 1/4] fix: preserve stack below when deleting a top stack block --- src/context_menu_items.ts | 26 +++++++++++++-- src/index.ts | 3 ++ tests/unit/context_menu_items.test.ts | 46 ++++++++++++++++++++++++++- 3 files changed, 71 insertions(+), 4 deletions(-) diff --git a/src/context_menu_items.ts b/src/context_menu_items.ts index 847666c22c..2297865575 100644 --- a/src/context_menu_items.ts +++ b/src/context_menu_items.ts @@ -34,9 +34,7 @@ export function registerDeleteBlock() { if (!scope.block) { return } - Blockly.Events.setGroup(true) - scope.block.dispose(true, true) - Blockly.Events.setGroup(false) + deleteBlock(scope.block) }, scopeType: Blockly.ContextMenuRegistry.ScopeType.BLOCK, id: 'blockDelete', @@ -45,6 +43,28 @@ export function registerDeleteBlock() { Blockly.ContextMenuRegistry.registry.register(deleteOption) } +export function deleteBlock(block: Blockly.Block) { + if (block.workspace.isFlyout) return + + const priorGroup = Blockly.Events.getGroup() + Blockly.Events.setGroup(true) + try { + if (!block.outputConnection && !block.previousConnection?.isConnected()) { + block.nextConnection?.disconnect() + } + if (block.workspace instanceof Blockly.WorkspaceSvg) { + block.workspace.hideChaff() + } + if (block instanceof Blockly.BlockSvg) { + block.dispose(!block.outputConnection, true) + } else { + block.dispose(!block.outputConnection) + } + } finally { + Blockly.Events.setGroup(priorGroup) + } +} + function getDeletableBlocksInStack(block: Blockly.Block): Blockly.Block[] { let descendants = block.getDescendants(false).filter(isDeletable) const nextBlock = block.getNextBlock() diff --git a/src/index.ts b/src/index.ts index d88a8f34d1..d5ffda53e6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -189,6 +189,9 @@ if (!blockCommentMenuItem) { } Blockly.ContextMenuRegistry.registry.unregister('blockDelete') contextMenuItems.registerDeleteBlock() +Blockly.BlockSvg.prototype.checkAndDelete = function () { + contextMenuItems.deleteBlock(this) +} contextMenuItems.registerDuplicateBlock() contextMenuItems.registerCopyShortcut() contextMenuItems.registerCutShortcut() diff --git a/tests/unit/context_menu_items.test.ts b/tests/unit/context_menu_items.test.ts index 1961ff769d..9bbf24325c 100644 --- a/tests/unit/context_menu_items.test.ts +++ b/tests/unit/context_menu_items.test.ts @@ -4,7 +4,7 @@ */ import * as Blockly from 'blockly/core' import { afterAll, afterEach, assert, beforeAll, beforeEach, describe, expect, it } from 'vitest' -import { registerDeleteBlock } from '../../src/context_menu_items' +import { deleteBlock, registerDeleteBlock } from '../../src/context_menu_items' // Tests for the scratch-specific delete context menu override (registerDeleteBlock). // The copy/cut/paste override (registerDuplicateBlock, issue #3470) is tested @@ -171,4 +171,48 @@ describe('registerDeleteBlock', () => { delete Blockly.Blocks.test_output_block } }) + + it('callback deletes only a top stack block and preserves its next block', () => { + const first = workspace.newBlock('test_stack_block') + const second = workspace.newBlock('test_stack_block') + const nextConn = first.nextConnection + const prevConn = second.previousConnection + assert(nextConn, 'Expected next connection') + assert(prevConn, 'Expected previous connection') + nextConn.connect(prevConn) + + const item = Blockly.ContextMenuRegistry.registry.getItem('blockDelete') + assert(item, 'Expected blockDelete item to be registered') + const callback = item.callback as (scope: Blockly.ContextMenuRegistry.Scope) => void + callback({ block: asBlockSvg(first) }) + + expect(workspace.getAllBlocks(false)).toEqual([second]) + expect(second.getParent()).toBeNull() + }) + + it('checkAndDelete override routes through deleteBlock and preserves next block', () => { + const first = workspace.newBlock('test_stack_block') + const second = workspace.newBlock('test_stack_block') + const nextConn = first.nextConnection + const prevConn = second.previousConnection + assert(nextConn, 'Expected next connection') + assert(prevConn, 'Expected previous connection') + nextConn.connect(prevConn) + + // Mirror the wiring in src/index.ts so we cover the keyboard-delete path. + // Tests use plain Workspace (not WorkspaceSvg), so blocks aren't BlockSvg + // instances — invoke the override via prototype to simulate the call site. + const originalCheckAndDelete = Blockly.BlockSvg.prototype.checkAndDelete + Blockly.BlockSvg.prototype.checkAndDelete = function () { + deleteBlock(this) + } + try { + Blockly.BlockSvg.prototype.checkAndDelete.call(first) + } finally { + Blockly.BlockSvg.prototype.checkAndDelete = originalCheckAndDelete + } + + expect(workspace.getAllBlocks(false)).toEqual([second]) + expect(second.getParent()).toBeNull() + }) }) From d58339aabc56849cc521a62524dd5d69ca99549f Mon Sep 17 00:00:00 2001 From: joeclinton1 Date: Wed, 29 Apr 2026 18:14:16 +0100 Subject: [PATCH 2/4] fix: guard potential lone stack delete edge case --- src/context_menu_items.ts | 4 ++-- tests/unit/context_menu_items.test.ts | 12 +++++++++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/context_menu_items.ts b/src/context_menu_items.ts index 2297865575..32d199df85 100644 --- a/src/context_menu_items.ts +++ b/src/context_menu_items.ts @@ -49,8 +49,8 @@ export function deleteBlock(block: Blockly.Block) { const priorGroup = Blockly.Events.getGroup() Blockly.Events.setGroup(true) try { - if (!block.outputConnection && !block.previousConnection?.isConnected()) { - block.nextConnection?.disconnect() + if (!block.outputConnection && !block.previousConnection?.isConnected() && block.nextConnection?.isConnected()) { + block.nextConnection.disconnect() } if (block.workspace instanceof Blockly.WorkspaceSvg) { block.workspace.hideChaff() diff --git a/tests/unit/context_menu_items.test.ts b/tests/unit/context_menu_items.test.ts index 9bbf24325c..8f2abac4c3 100644 --- a/tests/unit/context_menu_items.test.ts +++ b/tests/unit/context_menu_items.test.ts @@ -190,6 +190,16 @@ describe('registerDeleteBlock', () => { expect(second.getParent()).toBeNull() }) + it('callback deletes a lone stack block', () => { + const block = workspace.newBlock('test_stack_block') + + const item = Blockly.ContextMenuRegistry.registry.getItem('blockDelete') + assert(item, 'Expected blockDelete item to be registered') + ;(item.callback as (scope: Blockly.ContextMenuRegistry.Scope) => void)({ block: asBlockSvg(block) }) + + expect(workspace.getAllBlocks(false)).toEqual([]) + }) + it('checkAndDelete override routes through deleteBlock and preserves next block', () => { const first = workspace.newBlock('test_stack_block') const second = workspace.newBlock('test_stack_block') @@ -202,7 +212,7 @@ describe('registerDeleteBlock', () => { // Mirror the wiring in src/index.ts so we cover the keyboard-delete path. // Tests use plain Workspace (not WorkspaceSvg), so blocks aren't BlockSvg // instances — invoke the override via prototype to simulate the call site. - const originalCheckAndDelete = Blockly.BlockSvg.prototype.checkAndDelete + const originalCheckAndDelete = Reflect.get(Blockly.BlockSvg.prototype, 'checkAndDelete') Blockly.BlockSvg.prototype.checkAndDelete = function () { deleteBlock(this) } From d11d26d738e7ae2ce55ead3f97dd310906ea0480 Mon Sep 17 00:00:00 2001 From: joeclinton1 Date: Thu, 30 Apr 2026 18:36:36 +0100 Subject: [PATCH 3/4] fix: preserve active event group when deleting block --- src/context_menu_items.ts | 9 +++++++-- tests/unit/context_menu_items.test.ts | 23 +++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/context_menu_items.ts b/src/context_menu_items.ts index 32d199df85..bf9fd91645 100644 --- a/src/context_menu_items.ts +++ b/src/context_menu_items.ts @@ -47,7 +47,10 @@ export function deleteBlock(block: Blockly.Block) { if (block.workspace.isFlyout) return const priorGroup = Blockly.Events.getGroup() - Blockly.Events.setGroup(true) + const shouldStartGroup = !priorGroup + if (shouldStartGroup) { + Blockly.Events.setGroup(true) + } try { if (!block.outputConnection && !block.previousConnection?.isConnected() && block.nextConnection?.isConnected()) { block.nextConnection.disconnect() @@ -61,7 +64,9 @@ export function deleteBlock(block: Blockly.Block) { block.dispose(!block.outputConnection) } } finally { - Blockly.Events.setGroup(priorGroup) + if (shouldStartGroup) { + Blockly.Events.setGroup(false) + } } } diff --git a/tests/unit/context_menu_items.test.ts b/tests/unit/context_menu_items.test.ts index 8f2abac4c3..374f193ad4 100644 --- a/tests/unit/context_menu_items.test.ts +++ b/tests/unit/context_menu_items.test.ts @@ -200,6 +200,29 @@ describe('registerDeleteBlock', () => { expect(workspace.getAllBlocks(false)).toEqual([]) }) + it('callback reuses an active event group', () => { + const block = workspace.newBlock('test_stack_block') + const originalSetGroup = Reflect.get(Blockly.Events, 'setGroup') + const setGroupCalls: (boolean | string)[] = [] + + Blockly.Events.setGroup('outerGroup') + Blockly.Events.setGroup = (state: boolean | string) => { + setGroupCalls.push(state) + originalSetGroup(state) + } + try { + const item = Blockly.ContextMenuRegistry.registry.getItem('blockDelete') + assert(item, 'Expected blockDelete item to be registered') + ;(item.callback as (scope: Blockly.ContextMenuRegistry.Scope) => void)({ block: asBlockSvg(block) }) + + expect(setGroupCalls).not.toContain(true) + expect(Blockly.Events.getGroup()).toBe('outerGroup') + } finally { + Blockly.Events.setGroup = originalSetGroup + Blockly.Events.setGroup(false) + } + }) + it('checkAndDelete override routes through deleteBlock and preserves next block', () => { const first = workspace.newBlock('test_stack_block') const second = workspace.newBlock('test_stack_block') From 0cdbd861664ecfbab3d5ac078f48f45f9faaa749 Mon Sep 17 00:00:00 2001 From: joeclinton1 Date: Thu, 30 Apr 2026 18:55:43 +0100 Subject: [PATCH 4/4] fix: respect protected blocks in delete helper Current UI delete paths already check deletability. This is not reproducible through normal manual UI testing. Keep deleteBlock aligned with Blockly deletion semantics. --- src/context_menu_items.ts | 1 + tests/unit/context_menu_items.test.ts | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/src/context_menu_items.ts b/src/context_menu_items.ts index bf9fd91645..dfa7a6249d 100644 --- a/src/context_menu_items.ts +++ b/src/context_menu_items.ts @@ -45,6 +45,7 @@ export function registerDeleteBlock() { export function deleteBlock(block: Blockly.Block) { if (block.workspace.isFlyout) return + if (!block.isDeletable() || block.isShadow()) return const priorGroup = Blockly.Events.getGroup() const shouldStartGroup = !priorGroup diff --git a/tests/unit/context_menu_items.test.ts b/tests/unit/context_menu_items.test.ts index 374f193ad4..c2147713af 100644 --- a/tests/unit/context_menu_items.test.ts +++ b/tests/unit/context_menu_items.test.ts @@ -200,6 +200,24 @@ describe('registerDeleteBlock', () => { expect(workspace.getAllBlocks(false)).toEqual([]) }) + it('deleteBlock ignores non-deletable blocks', () => { + const block = workspace.newBlock('test_stack_block') + block.setDeletable(false) + + deleteBlock(block) + + expect(workspace.getAllBlocks(false)).toEqual([block]) + }) + + it('deleteBlock ignores shadow blocks', () => { + const block = workspace.newBlock('test_stack_block') + block.setShadow(true) + + deleteBlock(block) + + expect(workspace.getAllBlocks(false)).toEqual([block]) + }) + it('callback reuses an active event group', () => { const block = workspace.newBlock('test_stack_block') const originalSetGroup = Reflect.get(Blockly.Events, 'setGroup')