Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
barmac opened this issue Apr 30, 2024 · 4 comments · Fixed by #864
Closed

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

barmac opened this issue Apr 30, 2024 · 4 comments · Fixed by #864
Assignees
Labels
bug Something isn't working ux

Comments

@barmac
Copy link
Member

barmac commented Apr 30, 2024

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

Originally posted by @barmac in #852 (comment)

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:

  • Browser: [e.g. IE 11, Chrome 69]
  • OS: [e.g. Windows 7]
  • Library version: [e.g. 2.0.0]
@barmac barmac added bug Something isn't working ready Ready to be worked on ux labels Apr 30, 2024
@barmac barmac self-assigned this Apr 30, 2024
@barmac
Copy link
Member Author

barmac commented May 10, 2024

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 #859
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/change_event

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

@barmac
Copy link
Member Author

barmac commented May 10, 2024

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.

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed ready Ready to be worked on labels May 10, 2024
@nikku
Copy link
Member

nikku commented May 13, 2024

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

@barmac
Copy link
Member Author

barmac commented May 13, 2024

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

barmac added a commit that referenced this issue May 13, 2024
barmac added a commit that referenced this issue May 13, 2024
barmac added a commit that referenced this issue May 13, 2024
barmac added a commit that referenced this issue May 17, 2024
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels May 17, 2024
barmac added a commit that referenced this issue May 24, 2024
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ux
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants