The undo stack inserts deleted content in the wrong flow nodes

[christian-konrad]

Describe the Bug

When you delete a name or id of a flow node in the properties panel, then add a new node and hit undo, the name or id is inserted back again - in the right property, but of the wrong flow node. So, the revert operation is not conducted properly. Also, adding the flow node itself is not reverted. It seems like the undo stacks of the properties panel and bpmn-js clash.

Steps to Reproduce

1. Have a flow node
2. Enter a name or id
3. Remove the name or id (select all + entf/backspace/cut)
4. Add a new flow node
5. Hit undo
6. -> The name or id is inserted back again, but in the currently selected flow node (the new one)

Or just see the videos.

Bildschirmaufnahme.2022-06-28.um.22.01.23.mov

Bildschirmaufnahme.2022-06-28.um.21.59.22.mov

(Not validated if this also applies to other properties than ids and names).

Expected Behavior

Hitting undo, at first the newly added flow node should be removed, then the name or id should be inserted back again in the original node.

Environment

• Desktop Modeler 5.0.0 and Web Modeler (Nightly and our current dev env ultrawombat)

[pinussilvestrus]

Seems like this is related to direct editing visible after creating/appending tasks and text annotations.

I can't reproduce it if I append a flow node that's not immediately activating direct editing.

[pinussilvestrus]

I looked through the commit history to find a point where it is working properly. The bug even occurs in version 1.0.0-alpha.0.

I was not able to find the root cause for this in a time-boxed manner. However, I created a test case via 0dcd373 that tries to reproduce the issue.

Branch: 712-wrong-command-removal

What I found while creating the test case:

• The test case is failing randomly because the name input sometimes has focus. It shouldn't be like that after append, as direct editing is activated. Sometimes it just works as expected.
• When doing the same by firing command via commandStack only, everything works as expected.

This could be an indication that something is going on with handling the keyboard bindings + focusing properly between the modeler and properties panel.

I'd need more time to investigate this, so I'd propose to move this to the backlog as it's not a blocker.

/cc @christian-konrad @CatalinaMoisuc

[marstamm]

Expected Behavior

Hitting undo, at first the newly added flow node should be removed, then the name or id should be inserted back again in the original node.

As we are in the context of direct editing, undo should not be handled by bpmn-js at all but perform the native undo/redo on the contenteditable div

[nikku]

Could this be another one to be fixed by bpmn-io/diagram-js#662?

[marstamm]

I will try it out with your branch (#739), but I am not confident that it will be solved. No actual undo is triggered currently. I suspect it has more to do with focus handling/Event listeners during re-render of the property panel

[marstamm]

I can still reproduce it on #739

[nikku]

You'd need to test it against an end-to-end setup scenario: bpmn-io/diagram-js#662 (comment). There are just too many moving parts at the moment 😄

[marstamm]

I did locally link the diagram and bpmn-js branches as well 😄
It is releavant that we have the Properties panel, I found the root cause and I believe it is not something we can handle in bpmn-js.

Root Cause

• When you have 2 inputs and use the browser-native undo, it will switch the focused element.
  
• Usually, we capture the keyDown events and handle undo/redo ourselves
• DirectEditing does not have the custom key handlers, a historyUndo InputEvent is created. This is a normal input event and is not captured by keyHandlers
• We reuse the input field for names and only change the value. This causes focus to be set and the value to be changed by the input event

Possible Solutions
Still thinking about these ones. As the expected behaviour would be to stay in direct editing, I think we should prevent the browser default there.

[marstamm]

Same case can be created with undo via context pad, so the fix needs to be in the properties panel.

I'll see if we can capture undoHistory events in the root before they reach the target and execute a commandstack undo instead

[nikku]

Given your analysis I believe we should simply make sure that input (task A) is different from input (task B) (via a key attribute).

If we do that though we'll run into the issue that we don't properly remember (and restore) collapse + expand states.

[marstamm]

I was trying to use the beforeinput event to trigger commandstack undo when attempting to undo, but the beforeinput event is triggered on the direct editing container, the following input event in the properties panel.

Given your analysis I believe we should simply make sure that input (task A) is different from input (task B) (via a key attribute).

That makes sense. I'll try it out.
I have not looked at the collapse/expand states yet. From my understanding, if we only replace the inputs, the group should stay the same and could keep the collapse/expand state.

[nikku]

From my understanding, if we only replace the inputs, the group should stay the same and could keep the collapse/expand state.

Yes, however it will have other unwanted side-effects. I.e. caching of state fetched for Task A and making it available for Task B (use-case: custom stateful form elements).

I'm fine though to address / investigate this some day in the future.