FIx undo/redo for collapsed subprocesses #957
Conversation
|
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. |
bpmn-io-tasks
bot
added
in progress
|
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 |
bpmn-io-tasks
bot
added
needs review
| @@ -192,6 +192,11 @@ GraphicsFactory.prototype.updateContainments = function(elements) { | |||
| forEach(children.slice().reverse(), function(child) { | |||
| var childGfx = elementRegistry.getGraphics(child); | |||
There was a problem hiding this comment.
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#updateContainmentis 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?
There was a problem hiding this comment.
It may also be that we messed up the bpmn-js implementation.
There was a problem hiding this comment.
@jarekdanielak Let's figure out if we fixed the root cause of the issue here, happy to help with a debug session / code review.
bpmn-io-tasks
bot
added
in progress
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:
@bpmn-io/srtoolCloses {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}