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

Undo / redo does not fire onBeforeChange() in controlled component #31

Closed
cephirothdy2j opened this issue Oct 30, 2017 · 8 comments
Closed

Comments

@cephirothdy2j
Copy link

When making a change within the editor, and hitting CMD-Z to undo your changes, the onBeforeChange() event does not fire. However, the onChange() event does fire.

I'm willing to use onChange() to get around the issue, but all of the documentation around the controlled component states that onBeforeChange() should be used for managing the value of the component.

Here's a GIF of the problem in action:

onbeforesaveundo

Easiest way to reproduce:

Do as I did in the GIF: Visit the provided example with the React DevTools extension opened in Chrome, and you can see that the value of the input does not update in component state (done via onBeforeChange); though you do see the change event firing in the console, and the text within the component reflects what we see in the console.

@scniro
Copy link
Owner

scniro commented Oct 30, 2017

Hm interesting, thanks for bringing this up. I know codemirror manages some form of state internally in regards to history so the undo updating you're seeing if mostly codemirror itself.

I do think using onBeforeChange is appropriate here. Will keep you posted!

@cephirothdy2j
Copy link
Author

Thanks for the quick response. FWIW it's not included in the GIF above, but once you CMD-Z, not only does it fail to update the component's value immediately; any key input after that point places the key input in the wrong spot within the value and you end up with a completely broken experience.

scniro added a commit that referenced this issue Oct 31, 2017
@scniro
Copy link
Owner

scniro commented Oct 31, 2017

@cephirothdy2j Hey again, this should be fixed with the 3.0.4 release. Working with the codemirror history api proved to be quite difficult, but I think I was able to get through it. Can you please try the new release out?

Also, the side effect you had mentioned should be resolved, I too ran into that and think this fix should squash it.

@cephirothdy2j
Copy link
Author

Thanks for looking into this so quickly. I feel your pain on working with codemirror history; I forked your repo and attempted to fix this morning but could only fix about 60% of the problem. e.g. I'd get the undo stack working but lose the redo stack at the same time, or changes would undo correctly but misplace the cursor.

3.0.4 is MUCH better than 3.0.3 but there is still an issue when "fast-undoing". See the attached GIF, where I'm making some changes in the example and then holding down Cmd-Z to undo them all. One character gets left behind, and the codemirror instance becomes out of sync with your controlled component. The result is I can't even delete the leftover character unless I delete the entire line.

fast-undo-issue

@scniro
Copy link
Owner

scniro commented Nov 1, 2017

@cephirothdy2j Yep,I too can reproduce this. I don't think it has to do with the speed as I can get the same result still by slowly undo/redo-ing all of the inputs as well on your <div> example, but maybe the auto indentation codemirror is doing - specificially on electricInput. Unsure fully yet...

I'll be working through this today and will share my findings with you. Thanks for the collaboration!

scniro added a commit that referenced this issue Nov 1, 2017
@scniro
Copy link
Owner

scniro commented Nov 1, 2017

@cephirothdy2j Wow, this was tough! Thanks again for all the help on this. I believe I finally got this working with the 3.0.5 release. Let me know if better? Please reply back here if you should notice anything else weird again, but I'm feeling confident you'll get the behavior you're expecting this time. The only lingering thought might be an inconsistency in the cursor position so be sure to pay careful attention to it 🤞

@cephirothdy2j
Copy link
Author

It's perfect! I can't recreate any of the previous issues and so far I haven't seen any cursor weirdness.

Looking at the commit, you had to wade pretty deep into Codemirror to get to the root of the issue. Thanks for looking into this and resolving it so quickly.

@scniro
Copy link
Owner

scniro commented Nov 1, 2017

Np 👍 Going to close this out for now. Please re-open should you notice anything odd and thanks for using react-codemirror2!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants