Cannot undo element removal: Cannot read properties of undefined (reading 'appendChild')

[nikku]

Describe the Bug

Removing complex diagram and reverting that change via undo triggers the following error:

Cannot read properties of undefined (reading 'appendChild')
    at index.esm.js:28:17
    at it (index.esm.js:44:3)
    at mr._createContainer (GraphicsFactory.js:112:5)
    at mr.create (GraphicsFactory.js:136:15)
    at Vn._addElement (Canvas.js:866:29)
    at Vn.addShape (Canvas.js:888:15)
    at rf.revert (DeleteShapeHandler.js:88:10)
    at CommandStack.js:334:31
    at ih._atomicDo (CommandStack.js:377:5)
    at ih._internalUndo (CommandStack.js:330:8)
    at ih.undo (CommandStack.js:228:12)
    at Object.undo (EditorActions.js:80:20)
    at qp.trigger (EditorActions.js:170:24)
    at KeyboardBindings.js:77:21
    at EventBus.js:519:13
    at sr._invokeListener (EventBus.js:371:19)
    at sr._invokeListeners (EventBus.js:352:24)
    at sr.fire (EventBus.js:313:24)
    at Ia._keyHandler (Keyboard.js:104:35)
    at Ia._keydownHandler (Keyboard.js:86:8)
    at HTMLDocument.r (helpers.ts:87:17)

Steps to Reproduce

1. Open this diagram
2. Select all
3. Remove contents
4. Undo
5. See error in DEV tools

Expected Behavior

Undo works reliably.

Environment

• Browser: Chrome 100
• OS: Linux (Arch)
• Library version: bpmn-js@9.0.3, bpmn-js@9.2

[MaxTru]

@nikku in currents planning we were not able to assign this. Are you able to tackle? If not, plaese move to backlog / repropose next time.

[nikku]

Looking into this today and will move to backlog unless I#m able to crack this.

[nikku]

Root cause of this issue is a buggy DataStoreBehavior. In certain situations, when removing the last participant, we keep the data stores around (when they actually have no partner to attach to). During undo this explodes 💥.

[marstamm]

The Root cause is not the DataStore behavior but our makeProcess behavior and root replacing behavior. This also happens with comments placed outside of the Participant:

This is related to the delete order. If you manually select the Comment first, then the participant, the delete operation can be reverted. If we select+A, the behavior is as follows:

• Delete everything inside the participant
• Delete the participant
  • Replace the Collaboration with a Process
  • Set new root
  • ⚠️ This does NOT remove other root shapes, such as comments and data stores from the element registry. They are only removed from the Canvas, because their parent was removed. This will cause a problem later
• Delete the Comment/DataStore
  • ⚠️ The dataStore still has the participant as parent

On redo, we try to insert the element into the participant, which is not yet present.

Solution Sketches

• Properly delete all remaining content when creating a process. This is what visually happens on the canvas and should be reflected in the element Registry as well
• [Stretch/Follow up] Move root-elements to the newly created process. This might cause confusion, e.g. why comments are kept even when the related shape is gone

Alternatives considered

• Order deletion order to delete participants last. This does solve the issue, but it is fighting symptoms only and we might face similar problems in other parts of the code

[nikku]

@marstamm Could you checkout #1686? Any tests that we want to transfer over? Otherwise let's close the linked PR.

[marstamm]

The tests do not apply to the solution we implemented here. We do not want to remove the Data Store with the last participant but set the new parent correctly. Let's close the PR

[nikku]

@marstamm Could you investigate a follow-up quirk visualized in the following screen capture?

As you can see as we remove the participant the whole viewport seems to jump (test.bpmn.txt).

[marstamm]

This is because we replace the root element, which automatically resets the viewbox. I'll check what we do when creating a collaboration to keep the existing viewbox and apply it here as well