FIx undo/redo for collapsed subprocesses

[jarekdanielak]

Proposed Changes

Related to bpmn-io/bpmn-js#2269

Add some null checks to prevent crashes when undo/redo adds or removes a collapsed subprocess.

Checklist

To ensure you provided everything we need to look at your PR:

• Brief textual description of the changes present
• Visual demo attached
• Steps to try out present, i.e. using the @bpmn-io/sr tool
• Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

[barmac]

Do you have a branch with an integration test for bpmn-js? Let's verify that this PR fixes the issue when linked with bpmn-js.

[barmac]

Please add tests (in diagram-js too) which reproduce the issue and allow us to verify the fix.

[jarekdanielak]

I extended the integration tests in bpmn-js with one that covers this bug. Failing with current version, passes with the change: bpmn-io/bpmn-js@5d8240d

[nikku]

I struggle to understand in which situation this can become an issue, could you elaborate? And would you be able to model this as a test case in diagram-js?

From what I understand:

• GraphicsFactory#updateContainment is being called after any change operation happened (ref)
• At this stage, looking at the model, parents for updated children are collected
• Then the children inside these parents are re-ordered
• BUT some of the children are actually NOT part of the render tree anymore?

The BUT is the big issue for me. How can children exist, but not actually be rendered? Where don't we properly update the model before we render?

[barmac]

It may also be that we messed up the bpmn-js implementation.

[nikku]

@jarekdanielak Let's figure out if we fixed the root cause of the issue here, happy to help with a debug session / code review.

[jarekdanielak]

I spent some time debugging:

• GraphicsFactory#updateContainment crashes because an element inside of a subprocess that is being deleted by undo still has a parent assigned (the subprocess).
• Element's parent is correctly unset by Canvas#_removeElement, but is later brought back by MoveShapeHandler#revert.
• MoveShapeHandler#revert is fired because elements.move is fired when creating a collapsed subprocess.

At this point I need some help. I don't know why we call elements.move when creating a new collapsed subprocess. It doesn't fire when creating other types of elements.

@nikku could you step in?

[barmac]

@jarekdanielak I'm happy to support you with this. We can have a debugging session together to root cause the problem.

[nikku]

Thanks @barmac, will remove this from my TODO list. If you folks cannot figure it out, please ping me again.

[jarekdanielak]

Debugging session conclusions so far:

• When collapsed subprocess is created, we first make sure its children are visible.
• Then children are moved and positioned in the newly created plane.
• When user triggers undo, shape.create action for each element is reverted therefor they are removed.
• Then, shape.move action is reverted for each child. The child element's parent was set to null by Canvas#_removeElement, but move action revert handler sets it back to subprocess.

After the shape was removed, it cannot be moved back to the previous parent which is not present on the canvas as well. Therefore, we established that the root cause of the problem is in the implementation of the collapsed subprocess. We still need to find a way to fix it.

[barmac]

The SubProcessPlaneBehavior is triggered on each shape.create. However, we could trigger this only once for elements.create command. This means that we would move the elements at the end of the action. We are trying this out.

[barmac]

There's more in this issue than we expected. @jarekdanielak will push our solution sketch with the still not working text annotation part. The diagram-js PR can be closed as we established that the root cause is in bpmn-js.

[jarekdanielak]

Closing this one. More discussion and debugging will happen in bpmn-js: bpmn-io/bpmn-js#2275