bpmn-io / dmn-js

View and edit DMN diagrams in the browser.
https://bpmn.io/toolkit/dmn-js/
Other
288 stars 137 forks source link

Make inputs commit on blur, and let browser handle undo/redo #859

Closed barmac closed 2 months ago

barmac commented 3 months ago

Make inputs commit on blur, and have a separate command stack (browser native) instead of diagram-js command stack.

Originally posted by @barmac in https://github.com/bpmn-io/dmn-js/issues/852#issuecomment-2084683824

Describe the Bug

Right now the inputs commit to the command stack which results in:

  1. Slow input undo/redo
  2. You can undo changes outside of the input from that input

Steps to Reproduce

Steps to reproduce the behavior:

  1. Type in literal expression
  2. Cmd/Ctrl + Z

If you report a modeling related issue, ensure you can reproduce it on demo.bpmn.io

When reporting a library error, try to build an example that reproduces your problem. You can use our playgrounds for viewer or modeler as a starting point or put a demo up on GitHub for inspection.

Expected Behavior

Browser and code editor library handle inputs while command stack handles all the rest. Single undo outside of the input undoes the entire input change.

Environment

Please complete the following information:

barmac commented 2 months ago

Additional context:

There is a difference between how change events are handled in text inputs and contenteditable elements. For a text input, you can listen to a change event which is dispatched only on blur and if input value was changed. In contrast, contenteditable does not fire the change event, and a common solution is to listen to the input . However, it behaves differently as input event is dispatched after each keystroke. In a naive implementation of contenteditable, we may flood the editor command stack with changes which leads to a problems like in https://github.com/bpmn-io/dmn-js/issues/859 https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/change_event

https://camunda.slack.com/archives/GP70M0J6M/p1715331428555569

barmac commented 2 months ago

The FEEL editor relies on CodeMirror's updateListener to call the change handler. However, such updates behave much more closely to the browser input events, as the handler is called each time the editor view updates. This is way too often, so in the LiteralExpression component we should emulate browser's change event behavior, i.e. call the handler only on blur when the value changed.

nikku commented 2 months ago

We could consider to change the FEEL editor change handler once we settled on unifying the behavior.

barmac commented 2 months ago

Agreed, but that means a breaking change if we modify the onChange callback behavior.