controls_if: Mutation actions do not undo correctly

[AnmAtAnm]

Problem statement

Undo does not work correctly when controls_if mutator edit actions are interleaved with normal actions.

Expected Behavior

One undo step should undo to the previous step, maintaining connections unaffected by the edit.

Actual Behavior

A block connected to an if block where an undo reverts a mutation may also disconnect the child, even when the connected input is not (supposed to be) affected by the mutation.

Steps to Reproduce

1. Open the playground and create the following structure:

2. Next, make the following edits: a. Add an "else if" in the mutator. b. Connect the loop block to the newly created second statement connection c. Add an "else" to the end, within the mutator.

The results should look like this:

3. Undo one step, removing the last connection.

Note that making what was supposed to be a single step, editing the last connection via the mutator workspace, it has also disconnected the loop block.

[AnmAtAnm]

Comparable behavior appears to work correctly with text_join (aka, "create text") and lists_create_with. Thus, I'm assuming the bug it specific to the controls_if block, and I've rewritten and retitled the issue to reflect this.

That said, something is bothering me. How is the undo/redo event stream not capturing the necessary changes when capturing the before and after <mutation> changes? What assumption is the controls_if mutator breaking, and why is that an assumption at all?

[cpcallen]

This issue exists in again in develop (noted at a6361fb).

This issue was originally fixed in #2167 by introducing the method Blockly.Constants.Logic.CONTROLS_IF_MUTATOR_MIXIN.rebuildShape_, which calls this.updateShape_() and then reconnects any blocks that were disconnected, and then having .domToMutation call this.rebuildShape_() instead of this.updateShape_().

Using git bisect suggests that the issue was reintroduced in PR #5378. The proximate cause appears to be the change to Blockly.Events.BlockChange.prototype.run to prefer using .loadExtraState rather than .domToMutation when replaying BlockChange events. The reason this makes a difference is that the implementation of Blockly.Constants.Logic.CONTROLS_IF_MUTATOR_MIXIN.loadExtraState introduced in PR #5329 calls this.updateShape_() rather than this.rebuildShape_().

[cpcallen]

The root cause of this issue appears to be the same as issue #8225: BlockMove events being filtered out due to events being fired in the wrong order:

In the reproduction steps given above, at the point the else clause block is added to the stack in the mutator flyout, the following events are fired (in the given order) (edited for readability):

BlockMove {
  blockId: "%RcdpdplAuHZK~LiL}9+",
  newCoordinate: undefined,
  newInputName: "DO1",
  newParentId: "RM=6@108I~w2-=F!NzF[",
  oldCoordinate: undefined,
  oldInputName: "DO1",
  oldParentId: "RM=6@108I~w2-=F!NzF[",
  recordUndo: true,
}
BlockMove {
  blockId: "%RcdpdplAuHZK~LiL}9+",
  newCoordinate: undefined,
  newInputName: "DO1",
  newParentId: "RM=6@108I~w2-=F!NzF[",
  oldCoordinate: Coordinate {x: 200.29159545898438, y: 189.7638931274414},
  oldInputName: undefined,
  oldParentId: undefined,
  recordUndo: true,
}
BlockChange {
  blockId: "RM=6@108I~w2-=F!NzF[",
  element: "mutation",
  name: null,
  newValue: "{\"elseIfCount\":1,\"hasElse\":true}",
  oldValue: "{\"elseIfCount\":1}",
  recordUndo: true,
}

There is one small wrinkle: unlike the similar event queue reported in the root cause analysis for #8225, the first BlockMove here does not appear to be a disconnect event. Nevertheless, neither BlockMove event survives filtering, with only the BlockChange event actually being effected (and, it seems, therefore available to be undone).

[cpcallen]

In the process of investigating #7950 I have realised that the events listed in my previous comment may be wrong because although they were logged at the time that they were fire()ed, I was using console inspector after they had been subject to having been modified by filter() between when they were initially logged and when I inspected their values.

[cpcallen]

This has been adequately patched for the time being by the (new, higher quality) bandaid in #8539, but the underlying root cause remains and is tracked in #1036.