search

codemirror / dev

Development repository for the CodeMirror editor project
https://codemirror.net/
Other
5.94k stars 376 forks source link

Possible bug in the "Undoable Effects" example #1431

Closed andy0130tw closed 2 months ago

andy0130tw commented 2 months ago

Describe the issue

I found a potential bug in the example demonstrating the use of invertedEffects from @codemirror/commands. I can reproduce by the following sequence.

  1. Select a range (for example "something") that is at least 3 characters long.
  2. Press ctrl/cmd-h to highlight the range.
  3. Place the caret right after the second character ("so_mething") and quickly press backspace twice to delete the first two characters highlighted.
  4. Press ctrl-z to undo that change. The two edits in 3. should be undone at once.
  5. The highlighting is only partially restored. In specific, the second character's highlight is lost.

Demo:

https://github.com/user-attachments/assets/e1141966-9786-42bd-9700-4f59f53cb9a3

Apologize that I have not spent time locating the bug. I will update this issue if I gain more insight on this.

Browser and platform

N/A

Reproduction link

https://codemirror.net/examples/inverted-effect/

marijnh commented 2 months ago

This patch should help: https://github.com/codemirror/website/commit/8b30aec3ce78fb62de4021917fcc67d0affbb58c

andy0130tw commented 2 months ago

Thank you again for your help. The patch you applied resolved the issue I initially reported, but I have encountered another case where the inverted effects are still failing. This time, the issue arises when I use delete (fn-delete on Mac) to erase multiple characters before the mark boundary, and then undo the changes in one go. The highlight after the first character is also lost after the undo.

This is reproducible in the example:

https://github.com/user-attachments/assets/69f1ead8-32cf-4cf5-bf3e-9009e30ba352

I replicated the example in a sandbox, looked into each transaction the editor has undergone. I noticed that, while the callback in invertedEffects.of computed the effects needed for inverting the corresponding transaction, the undo manager concatenated multiple effects which should be mapped as if each were applied at the document it was attached to to ensure a correct outcome.

marijnh commented 2 months ago

Very good point. That was an actual bug in the history implementation. Attached patch should help with this.