Skip to content

Commit 240d44a

Browse files
authored
Merge pull request #3563 from scratchfoundation/fix/cursed-inputs-duplicate-paste
Fix cursed inputs on duplicate/paste
2 parents c7bb245 + 22f6429 commit 240d44a

4 files changed

Lines changed: 244 additions & 21 deletions

File tree

src/checkable_continuous_flyout.ts

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import { ContinuousFlyout, type LabelFlyoutItem } from '@blockly/continuous-toolbox'
66
import * as Blockly from 'blockly/core'
77
import { CheckboxBubble } from './checkbox_bubble'
8+
import { stripIds } from './scratch_blocks_utils'
89
import { StatusIndicatorLabel } from './status_indicator_label'
910
import { STATUS_INDICATOR_LABEL_TYPE } from './status_indicator_label_flyout_inflater'
1011

@@ -20,27 +21,6 @@ function isCheckboxIcon(icon: Blockly.IIcon | undefined): icon is Blockly.IIcon
2021
return !!icon && typeof (icon as { setChecked?: unknown }).setChecked === 'function'
2122
}
2223

23-
/**
24-
* Recursively strip `id` properties from a serialized block state tree
25-
* so that every block (including shadows and nested inputs) gets a fresh
26-
* ID when deserialized onto the workspace.
27-
* @param state A serialized block state object.
28-
*/
29-
function stripIds(state: Blockly.serialization.blocks.State): void {
30-
delete state.id
31-
if (state.inputs) {
32-
for (const inputName in state.inputs) {
33-
const conn = state.inputs[inputName]
34-
if (conn.shadow) stripIds(conn.shadow)
35-
if (conn.block) stripIds(conn.block)
36-
}
37-
}
38-
if (state.next) {
39-
if (state.next.shadow) stripIds(state.next.shadow)
40-
if (state.next.block) stripIds(state.next.block)
41-
}
42-
}
43-
4424
export class CheckableContinuousFlyout extends ContinuousFlyout {
4525
declare protected tabWidth_: number
4626
declare MARGIN: number

src/scratch_block_paster.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
* SPDX-License-Identifier: Apache-2.0
44
*/
55
import * as Blockly from 'blockly/core'
6+
import { stripIds } from './scratch_blocks_utils'
67
import type { ScratchCommentIcon } from './scratch_comment_icon'
78

89
/**
@@ -22,6 +23,12 @@ class ScratchBlockPaster extends Blockly.clipboard.BlockPaster {
2223
workspace: Blockly.WorkspaceSvg,
2324
coordinate: Blockly.utils.Coordinate,
2425
) {
26+
// Strip all block IDs so that every block in the pasted tree (including
27+
// shadows) gets a fresh ID. Without this, disposed shadows from the
28+
// original block can reuse the same ID, causing the VM to think both
29+
// blocks share the same shadow. Deleting one then destroys the other's
30+
// shadow (forum topic 878291).
31+
stripIds(copyData.blockState)
2532
const block = super.paste(copyData, workspace, coordinate)
2633
if (block?.type === 'argument_reporter_boolean' || block?.type === 'argument_reporter_string_number') {
2734
block.setDragStrategy(new Blockly.dragging.BlockDragStrategy(block))

src/scratch_blocks_utils.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,31 @@
2020
* @file Utility methods for Scratch Blocks but not Blockly.
2121
* @author fenichel@google.com (Rachel Fenichel)
2222
*/
23+
import type * as Blockly from 'blockly/core'
24+
25+
/**
26+
* Recursively strip `id` properties from a serialized block state tree
27+
* so that every block (including shadows and nested inputs) gets a fresh
28+
* ID when deserialized onto the workspace. This prevents two blocks from
29+
* sharing the same shadow block in the VM, which would cause deleting
30+
* one to destroy the other's shadow.
31+
* @param state A serialized block state object.
32+
*/
33+
export function stripIds(state: Blockly.serialization.blocks.State): void {
34+
delete state.id
35+
if (state.inputs) {
36+
for (const inputName in state.inputs) {
37+
const conn = state.inputs[inputName]
38+
if (conn.shadow) stripIds(conn.shadow)
39+
if (conn.block) stripIds(conn.block)
40+
}
41+
}
42+
if (state.next) {
43+
if (state.next.shadow) stripIds(state.next.shadow)
44+
if (state.next.block) stripIds(state.next.block)
45+
}
46+
}
47+
2348
/**
2449
* Compare strings with natural number sorting.
2550
* @param str1 First input.
Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
/**
2+
* Copyright 2026 Scratch Foundation
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
import * as Blockly from 'blockly/core'
6+
import { afterAll, afterEach, assert, beforeAll, beforeEach, describe, expect, it } from 'vitest'
7+
import { CheckableContinuousFlyout } from '../../src/checkable_continuous_flyout'
8+
import { registerRecyclableBlockFlyoutInflater } from '../../src/recyclable_block_flyout_inflater'
9+
import { registerScratchBlockPaster } from '../../src/scratch_block_paster'
10+
11+
// Browser tests for the shared-shadow-ID bug (forum topic 878291).
12+
// Both the duplicate/paste path and the flyout copy path must produce
13+
// blocks with unique shadow IDs. Without stripIds, disposed shadows
14+
// from the original reuse their IDs in the copy, causing the VM to
15+
// think both blocks share the same shadow.
16+
17+
const BLOCK_TYPES = ['test_value_block', 'test_text_shadow', 'test_reporter']
18+
19+
let container: HTMLElement | undefined
20+
let workspace: Blockly.WorkspaceSvg | undefined
21+
22+
beforeEach(() => {
23+
container = document.createElement('div')
24+
container.style.width = '800px'
25+
container.style.height = '600px'
26+
document.body.appendChild(container)
27+
28+
Blockly.defineBlocksWithJsonArray([
29+
{
30+
type: 'test_value_block',
31+
message0: 'set to %1',
32+
args0: [{ type: 'input_value', name: 'VALUE' }],
33+
previousStatement: null,
34+
nextStatement: null,
35+
},
36+
{
37+
type: 'test_text_shadow',
38+
message0: '%1',
39+
args0: [{ type: 'field_input', name: 'TEXT' }],
40+
output: 'String',
41+
},
42+
{
43+
type: 'test_reporter',
44+
message0: 'answer',
45+
output: 'String',
46+
},
47+
])
48+
})
49+
50+
// Save the default Blockly implementations so we can restore them after
51+
// all tests, preventing cross-suite leakage of Scratch-specific overrides.
52+
const DefaultInflater = Blockly.registry.getClass(Blockly.registry.Type.FLYOUT_INFLATER, 'block')
53+
54+
beforeAll(() => {
55+
registerScratchBlockPaster()
56+
registerRecyclableBlockFlyoutInflater()
57+
})
58+
59+
afterAll(() => {
60+
// Restore the default block paster
61+
Blockly.clipboard.registry.unregister(Blockly.clipboard.BlockPaster.TYPE)
62+
Blockly.clipboard.registry.register(Blockly.clipboard.BlockPaster.TYPE, new Blockly.clipboard.BlockPaster())
63+
// Restore the default flyout inflater
64+
if (DefaultInflater) {
65+
Blockly.registry.unregister(Blockly.registry.Type.FLYOUT_INFLATER, 'block')
66+
Blockly.registry.register(Blockly.registry.Type.FLYOUT_INFLATER, 'block', DefaultInflater)
67+
}
68+
})
69+
70+
afterEach(() => {
71+
workspace?.dispose()
72+
container?.remove()
73+
for (const t of BLOCK_TYPES) {
74+
delete Blockly.Blocks[t]
75+
}
76+
})
77+
78+
describe('duplicate block shadow IDs (forum topic 878291)', () => {
79+
it('duplicated block gets unique shadow IDs, not shared with original', () => {
80+
assert(container, 'Expected container from beforeEach')
81+
workspace = Blockly.inject(container, {})
82+
83+
// Create the original block with a shadow on VALUE
84+
let original: Blockly.BlockSvg
85+
let reporter: Blockly.BlockSvg
86+
Blockly.Events.disable()
87+
try {
88+
original = Blockly.serialization.blocks.append(
89+
{
90+
type: 'test_value_block',
91+
inputs: {
92+
VALUE: {
93+
shadow: {
94+
type: 'test_text_shadow',
95+
fields: { TEXT: '0' },
96+
},
97+
},
98+
},
99+
},
100+
workspace,
101+
) as Blockly.BlockSvg
102+
103+
// Connect a reporter to obscure the shadow
104+
reporter = workspace.newBlock('test_reporter')
105+
} finally {
106+
Blockly.Events.enable()
107+
}
108+
109+
const conn = original.getInput('VALUE')?.connection
110+
assert(conn, 'Expected VALUE connection')
111+
const reporterOutput = reporter.outputConnection
112+
assert(reporterOutput, 'Expected reporter output connection')
113+
conn.connect(reporterOutput)
114+
115+
// Capture the original's shadow state before duplication
116+
const originalShadowState = conn.getShadowState()
117+
assert(originalShadowState, 'Expected shadow state on original after connecting reporter')
118+
const originalShadowId = originalShadowState.id
119+
assert(originalShadowId, 'Expected shadow state to have an ID')
120+
121+
// Duplicate via the actual clipboard path (toCopyData + paste)
122+
const copyData = original.toCopyData()
123+
assert(copyData, 'Expected toCopyData to return data')
124+
const copyResult = Blockly.clipboard.paste(copyData, workspace)
125+
assert(copyResult, 'Expected paste to return a block')
126+
const copy = copyResult as unknown as Blockly.BlockSvg
127+
128+
// The copy's shadow should have a DIFFERENT ID than the original's
129+
const copyConn = copy.getInput('VALUE')?.connection
130+
assert(copyConn, 'Expected VALUE connection on copy')
131+
const copyShadowState = copyConn.getShadowState()
132+
assert(copyShadowState, 'Expected shadow state on copy')
133+
134+
expect(copyShadowState.id).not.toBe(originalShadowId)
135+
136+
// Verify: deleting the copy does not affect the original's shadow
137+
copy.dispose()
138+
reporterOutput.disconnect()
139+
const respawned = conn.targetBlock()
140+
assert(respawned, 'Original shadow should respawn after copy is deleted')
141+
expect(respawned.isShadow()).toBe(true)
142+
expect(respawned.type).toBe('test_text_shadow')
143+
})
144+
145+
it('two flyout copies get unique shadow IDs when first copy shadow is obscured', () => {
146+
assert(container, 'Expected container from beforeEach')
147+
// Inject with CheckableContinuousFlyout (which has the stripIds fix)
148+
// and a toolbox that includes a block with a shadow input.
149+
workspace = Blockly.inject(container, {
150+
toolbox: {
151+
kind: 'flyoutToolbox',
152+
contents: [
153+
{
154+
kind: 'block',
155+
type: 'test_value_block',
156+
inputs: {
157+
VALUE: {
158+
shadow: {
159+
type: 'test_text_shadow',
160+
fields: { TEXT: '0' },
161+
},
162+
},
163+
},
164+
},
165+
],
166+
},
167+
plugins: {
168+
flyoutsVerticalToolbox: CheckableContinuousFlyout,
169+
},
170+
})
171+
172+
const flyout = workspace.getFlyout()
173+
assert(flyout, 'Expected workspace to have a flyout')
174+
175+
// Find the template block in the flyout
176+
const flyoutBlocks = flyout.getWorkspace().getAllBlocks(false)
177+
const template = flyoutBlocks.find((b) => b.type === 'test_value_block')
178+
assert(template, 'Expected template block in flyout')
179+
180+
// First copy from flyout
181+
const copy1 = flyout.createBlock(template)
182+
const conn1 = copy1.getInput('VALUE')?.connection
183+
assert(conn1, 'Expected VALUE connection on first copy')
184+
const shadow1 = conn1.targetBlock()
185+
assert(shadow1, 'Expected shadow on first copy')
186+
expect(shadow1.isShadow()).toBe(true)
187+
const shadow1Id = shadow1.id
188+
189+
// Obscure the first copy's shadow by connecting a reporter
190+
let reporter: Blockly.BlockSvg
191+
Blockly.Events.disable()
192+
try {
193+
reporter = workspace.newBlock('test_reporter')
194+
} finally {
195+
Blockly.Events.enable()
196+
}
197+
const reporterOutput = reporter.outputConnection
198+
assert(reporterOutput, 'Expected reporter output connection')
199+
conn1.connect(reporterOutput)
200+
201+
// Second copy from flyout — its shadow must get a different ID
202+
const copy2 = flyout.createBlock(template)
203+
const conn2 = copy2.getInput('VALUE')?.connection
204+
assert(conn2, 'Expected VALUE connection on second copy')
205+
const shadow2 = conn2.targetBlock()
206+
assert(shadow2, 'Expected shadow on second copy')
207+
expect(shadow2.isShadow()).toBe(true)
208+
209+
expect(shadow2.id).not.toBe(shadow1Id)
210+
})
211+
})

0 commit comments

Comments
 (0)