scniro / react-codemirror2

Codemirror integrated components for React
MIT License
1.66k stars 193 forks source link

autoCursor broken starting in v4.0.0 #63

Closed glasserc closed 6 years ago

glasserc commented 6 years ago

Refs https://github.com/mozilla-services/react-jsonschema-form/issues/851

We are using an UnControlled component with autoCursor={false} because this seems the most similar to the v2.X behavior. Without autoCursor={false}, every edit causes the cursor to jump to the end of the document (probably because we're resetting the value on every render). With autoCursor={false} in v3.0.7, the cursor behaves normally (for instance, advancing as the user types). However, in v4.0.0, even with autoCursor={false}, the cursor jumps to the end of the document at the end of every edit, as previously. As far as I can tell, autoCursor is essentially useless now (although I am not an expert in CodeMirror or even React so maybe I'm missing something).

I believe the change happened in https://github.com/scniro/react-codemirror2/commit/934521af9e89b58572d90e48ecdcf96a0760e95e#diff-168726dbe96b3ce427e7fedce31bb0bcL521.

Do you have advice on how best for us to upgrade to v4?

scniro commented 6 years ago

@glasserc Hey thanks for opening this up. I've read through this as well as the referenced issue and am curious as to why you are resetting value on an UnControlled component. The uncontrolled component does't care about the value whatsoever other than a starting one, as it's all managed internally and simply spit back out to the user on a cb (if needed). It's best to just give value a static string starting value and never worry about it again.

Perhaps you are wanting the Controlled variant if you are indeed wanting to manage value (perhaps this is coming down via redux, or similar). A few solutions come to mind...

I'm thinking of doing the last suggestion anyways as it makes sense, but I'd try the first two as well as I suspect you may be using UnControlled just a bit incorrectly. Either way, keep me posted with your findings and If I can reproduce this locally I'll get a fix published this weekend.

glasserc commented 6 years ago

Hi, thanks for your advice. Some background: in our application, the user edits JSON in the editor, but periodically (when the JSON value being edited "changes", i.e. the parsed JSON object is different from the last parsed JSON object) we re-set the value to a pretty-printed version of the current JSON object. When this happens, ideally the cursor would stay "where it is", i.e. if the user has just typed the closing quote after a new property, then the cursor would still be located at this closing quote, even if the closing quote moved to a new location due to pretty-printing. However, this behavior seems hard, and all I really want is to have behavior that is similar enough to our old behavior, where the cursor goes to someplace nearby.

I think you're right that we are actually most like a Controlled variant, but I had some problems with the cursor moving to the end of the value whenever we re-format the JSON value. So I guess my bug should actually have the title "How can I maintain cursor position when I update the value?" As you can see from the linked issue, I tried using autoCursor={false} with the Controlled component, but that caused even stranger behavior -- the cursor remains fixed, even when typing new characters (which appear after the cursor, causing typed text to come out backwards). We weren't tracking the cursor position as a prop previously, but maybe that's the approach I should be following? I tried something like this but didn't see any difference in behavior from just not tracking the cursor state at all.

scniro commented 6 years ago

Hm okay, it seems like option 3 may be the best quick approach here. You can confirm this was not an issue in 2.x? I think I just need to be more strict in the handling of props on the uncontrolled component

scniro commented 6 years ago

@glasserc okay I may have found the issue by being more hands off with the uncontrolled component and letting cursor delegation largely be driven on it's own unless explicitly specified via autoCursor. The behavior should replicate the likes of v2.x

Here's the deal, testing this lib has been tough, and I can't reliably run my suite and assume this safely fixes the issue with the upmost certainty. I'm often left to manually regress this, which is okay for now, but it gets very challenging because there are a lot of different use cases from folks, some I have never even imagined before, lol. Also, even with reading over your use case a few times I couldn't exactly reproduce this on my end so I worked off a more basic scenario.

I pushed up scniro/react-codemirror2-test-pkg so this can be tested in isolation. Can you please install this and let me know if any of the issues on your end are resolved? Thanks

glasserc commented 6 years ago

Yes, the behavior from 2.x is the same as the behavior of 3.x's UnControlled with autoCursor=false, and it's the same behavior I see from your test package (i.e. it fixes my issue, thanks!). I have a lot of sympathy for how difficult it is to maintain a project where people use it in unexpected ways -- if there's a more expected way for us to do what we're doing, I'm totally willing to do that too. I just couldn't figure out what that way was. If you have other branches or tests you'd like me to try, I'm happy to do that as well.

scniro commented 6 years ago

@glasserc Excellent news! I'm happy to hear this. Ideally I'll have feature branching in the project to just pull the source files but I've found folks who I can kindly convince to help me prefer quickly installing the tgz file 😸. Either way I definitely appreciate your help on this. I'm going to regress this a bit more this evening and publish up a new version for you to consume. Will keep you posted here when it's available.

scniro commented 6 years ago

@glasserc You should be all set with the 4.1.0 release. Please let me know if this fully resolves your issue as you tested with the earlier patch. Thanks for the collaboration on this!

glasserc commented 6 years ago

Yes, it works great, thanks again!