Mutated procedures_ifreturn detaches child block, but undoing doesn't reattach it.

[johnnesky]

Check for duplicates

• I have searched for similar issues before opening a new one.

Description

Child connections automatically detach when a mutation removes the connection. This isn't undoable but should be.

Reproduction steps

1. Navigate to https://blockly-demo.appspot.com/static/tests/playground.html
2. Drag a procedures_ifreturn block to the workspace.
3. Drag a procedures_defnoreturn block to the workspace, separately.
4. Add a third block and attach it to the return connection of the procedures_ifreturn block.
5. Attach the procedures_ifreturn as a stack child to the procedures_defnoreturn block. This will mutate the procedures_ifreturn and detach its own child.
6. Undo dragging the procedures_ifreturn block. The block will move, but neither its mutation nor the detached child will get undone.

Stack trace

events_block_move.ts:290 Can't connect to non-existent input: VALUE
    run	@	events_block_move.ts:290
    undo	@	workspace.ts:658
    callback	@	shortcut_items.ts:274
    onKeyDown	@	shortcut_registry.ts:236
    onKeyDown	@	inject.ts:309
    wrapFunc	@	browser_events.ts:65

Screenshots

No response

Browsers

Chrome desktop

[cpcallen]

[Stack trace added to issue description.]

This appears to be superficially similar to #2037 and #8225, but initial investigation suggests that the root cause may be slightly different. In particular, the relevant events that occur at step 5 of the reproduction (i.e., the ones potentially being undone) are (formatted for comparability; apologies to anyone with a narrow monitor):

Selected    {isUiEvent: true,  type: 'selected', group: '',                     recordUndo: false, …}
BlockDrag   {isUiEvent: true,  type: 'drag',     group: 'DOEbh4HgW]OmmN#/u8sl', recordUndo: false, blockId: "$V,?`P=DF{7hJJmxS2$r", blocks: [BlockSvg, BlockSvg], …}
BlockDrag   {isUiEvent: true,  type: 'drag',     group: 'DOEbh4HgW]OmmN#/u8sl', recordUndo: false, blockId: "$V,?`P=DF{7hJJmxS2$r", blocks: [BlockSvg, BlockSvg], …}
BlockMove   {isUiEvent: false, type: 'move',     group: 'DOEbh4HgW]OmmN#/u8sl', recordUndo: true,  blockId: "$V,?`P=DF{7hJJmxS2$r", newCoordinate: undefined,          newInputName: "STACK",   newParentId: "*F7CG)VC6#v#JYvxzo1H", oldCoordinate: {x: 488,   y: 213   }, oldInputName: undefined, oldParentId: undefined,              reason: ['connect', 'drag'], …}
BlockMove   {isUiEvent: false, type: 'move',     group: 'DOEbh4HgW]OmmN#/u8sl', recordUndo: true,  blockId: "$V,?`P=DF{7hJJmxS2$r", newCoordinate: undefined,          newInputName: "STACK",   newParentId: "*F7CG)VC6#v#JYvxzo1H", oldCoordinate: {x: 316.8, y: 604.01}, oldInputName: undefined, oldParentId: undefined,              reason: ['connect'], …}
BlockMove   {isUiEvent: false, type: 'move',     group: 'WO]?=K0U3V1p3.y3Dc^R', recordUndo: true,  blockId: "fesDQ*o|~Jk@X$T`5_G2", newCoordinate: {x: 441.1, y: 596}, newInputName: undefined, newParentId: undefined,              oldCoordinate: undefined,             oldInputName: "VALUE",   oldParentId: "$V,?`P=DF{7hJJmxS2$r", reason: ['disconnect'], …}
BlockChange {isUiEvent: false, type: 'change',   group: '',                     recordUndo: false, blockId: "$V,?`P=DF{7hJJmxS2$r", disabledReason: "UNPARENTED_IFRETURN", element: "disabled", name: undefined, newValue: false, oldValue: true, …}

Notably: although there is a BlockChange event, it is not for a mutation but merely for the disablement.

[cpcallen]

Root cause analysis

I made local changes to fireNow (in core/events/utils.ts) and to Workspace.prototype.undo (in core/workspace.ts) to disable all event filtering and also to add some diagnostic logging for events. Step 5 of the reproduction instructions above causes the following events to be fire()ed in the given order:

Selected
BlockDrag {
    group: "0L#iQ#FM[AL6NbtYVdJv",
}
BlockDrag {
    group: "0L#iQ#FM[AL6NbtYVdJv",
}
BlockMove {
    group: "0L#iQ#FM[AL6NbtYVdJv",
    blockId: "*e{~WSpS$!,Ku,~RBlLh",  // PROCEDURES_IFRETURN block
    oldCoordinate: {"x":38,"y":338},
    oldInputName: undefined,
    oldParentId: undefined,
    newCoordinate: {"x":54.467010498046875,"y":141.36370849609375},
    newInputName: undefined,
    newParentId: undefined,
    reason: ["drag"],
}
BlockMove {
    group: "0L#iQ#FM[AL6NbtYVdJv",
    blockId: "*e{~WSpS$!,Ku,~RBlLh",  // PROCEDURES_IFRETURN block
    oldCoordinate: {"x":54.467010498046875,"y":141.36370849609375},
    oldInputName: undefined,
    oldParentId: undefined,
    newCoordinate: undefined,
    newInputName: "STACK",
    newParentId: "UjAZ(|1*SRzg*4$)etbX",  // PROCEDURES_DEFNORETURN block
    reason: ["connect"],
}
BlockMove {
    group: "3O=_?lF0Y=J2RH1:1.$G",
    blockId: "4$[^0G@VU1S0L*04/$|U",  // Third block
    oldCoordinate: undefined,
    oldInputName: "VALUE",
    oldParentId: "*e{~WSpS$!,Ku,~RBlLh",  // PROCEDURES_IFRETURN block
    newCoordinate: {"x":191.12631225585938,"y":146},
    newInputName: undefined,
    newParentId: undefined,
    reason: ["disconnect"],
}
BlockChange {
    blockId: "*e{~WSpS$!,Ku,~RBlLh",
    element: "disabled",
}

Having run Blockly.getMainWorkspace().clearUndo() before performing step 5, I observed the state of .undoStack_ to contain only the three BlockMove events (in the same order they were fired above).

Observations:

• The final BlockChange event, resulting from the disconnection of the attached third block from the IFRETURN block, has a different group from the other two events.
• The BlockChange event is only has information about the PROCEDURES_IFRETURN block being reenabled, not about its mutation.
• It is not included in the undo stack because of the following code in the PROCEDURES_IFRETURN block definition's onchange handler:

    if (!this.isInFlyout) {
      try {
        // There is no need to record the enable/disable change on the undo/redo
        // list since the change will be automatically recreated when replayed.
        eventUtils.setRecordUndo(false);
        this.setDisabledReason(!legal, UNPARENTED_IFRETURN_DISABLED_REASON);
      } finally {
        eventUtils.setRecordUndo(true);
      }
    }

• There is no BlockChange (or any other) event explicitly reporting the mutation of the block to replace the VALUE value input with a dummy input; instead this is implicit in the BlockMove event that connected the IFRETURN block to the DEFNORETURN block.
• This sequence of events cannot be successfully undone, because they will be replayed in revere order, and the disconnect event will fail to replay correctly because the IFRETURN block no longer has (or: does not yet have) a ValueInput to connect the third block to—and indeed the first undo call results in the following warning appearing in the console:

  Can't connect to non-existent input: VALUE		events_block_move.ts:290
  

• Additionally, merely moving a procedures_ifreturn out of a procedures_defnoreturn is not sufficient to restore its VALUE input; it has to be moved into a procedures_defreturn block.

So the root cause of this issue is: [Edit: this turns out not to be the root root cause; see subsequent comment.]

• There is no BlockChange event to undo, and
• The BlockMove connect event which could in principle undo the mutation is earlier in the undo stack and has a different group ID than the BlockMove event disconnecting the third block.
  • N.B. "in principle" because undoing this BlockMove does not in fact result in the VALUE value input being recreated; that only happens if the PROCEDURES_IFRETURN is dragged into a PROCEDURES_DEFRETURN block

In addition, there is a confounding issue is that the block mutation is occurring after the IFRETURN block had been connected to the enclosing DEFNORETURN block, via an onchange handler.

(Im?)Possible Remedies

The favoured proposed fixes for #8225 (currently in local testing) do not resolve this issue, and since there are several underlying and interacting causes (no BlockChange events, events not being part of the same group) there is probably no 'single piece' solution to this issue.

There are a few approaches that would partially address this bug:

• The proposal to have block shape changing methods emit BlockChange events (see items 2 and 3 in the first section of the proposed remedies for #8225) would fix the problem of making sure that a BlockChange event was emitted, but getting it emitted in the correct order and with the correct group seems non-trivial.
• It would be possible to have the onchange handler explicitly fire a BlockChange event, but this would depart from the existing practice (no other block or mixin fires any events).
• It would be possible to modify the onchange handler to restore the VALUE value input when the IFRETURN block is removed from the DEFNORETURN block (whether as a result of a manual drag or undoing a previous drag), but there will still be issues with event ordering with respect to the disconnect event.

[cpcallen]

Tangentially related issue: #6456.

[cpcallen]

Root cause analysis continued

Discussing this and related issues with @NeilFraser, he pointed out that the underlying cause of this issue is that the Blockly program is momentarily in an invalid state after the procedures_ifreturn block is attached to the procedures_defnoreturn, before its onchange handler rectifies this situation. Code generated in that instant would potentially be invalid (attempting to return a value from a procedure/function with no declared return value)—and a quick investigation of some other block onchange handlers revealed that many of them suffer from the same problem.

Filed #8467 to track solving the underlying general issue.