scniro / react-codemirror2

Codemirror integrated components for React
MIT License
1.65k stars 192 forks source link

VIM - Strange behavior in VIM mode with Controlled Usage #37

Open edmondxiexie opened 6 years ago

edmondxiexie commented 6 years ago

The VIM mode has strange behavior when working with states. For example, when I press either 'o' or 'i' to enter insert mode, then press Backspace. It doesn't seem to allow me to delete previous characters before the insert position, which seems weird. And also, the cursor in insert mode will change from the line shape to the block shape after one insertion operation. Is it related to the rerendering when state changing? Because it does not happen in UnControlled Usage.

I use the OnClick function to change the state and use states for changing the value in the editor.

Preview:

scniro commented 6 years ago

Interesting, thanks for opening this up, I'll give this a look later today. There is a bit of logic going on for controlled mode to retain/set the cursor in certain cases. I hope VIM isn't going to be a one-off to work around this. I'll keep you posted!

scniro commented 6 years ago

@edmondxiexie Sorry for the delayed response. Okay yep I can definitely reproduce this. Can you use the uncontrolled variant for now? I'm unsure how much work this could entail. I haven't used this mode before nor am I that much of a VIM power user, so when looking for somewhere to get started I came across this on this note over on codemirror

The CodeMirror vim bindings do not have an active maintainer. That means that if you report bugs in it, they are likely to go unanswered. It also means that if you want to help, you are very welcome to look at the open issues and see which ones you can solve.

This has me a little hesitant to dive in as it could lead to an unraveling of who knows what. Either way, I'll start to look and keep you posted.

scniro commented 6 years ago

@edmondxiexie okay so I dug into this a bit and think I may know whats going on here. Basically, for the controlled component there is also a dummy DOMless codemirror instance that mirrors and tracks changes, to which is later applied to the main instance upon state management (new props). However, when using VIM mode (and likely others), there is a notion of a key map that alters the behavior of the editor e.g. for insert => press "i"

The issue is, I don't believe I can emulate this key event to the mirror (dummy), so when I go ahead and extract the mirrored value, it's now incorrect because the keymap altered what would have been applied to the main instance, and is something different entirely. So what I've been digging for is a way to trigger a keystroke change programatically to the dummy, but I have been having trouble to find such a way.

There is a keyHandled callback which is an absolutely perfect hook to apply the emulated event to the dummy, however I'm struggling to find a way to do so through the APIs. I thought maybe I could even grab the native <textarea> and manually dispatchEvent to it - but I can't get _the dummys instance to fire keyHandled.

I might need help with this one, but I'll keep at it when I have time.

xralphack commented 6 years ago

same issue on both Controlled and UnControlled

scniro commented 6 years ago

@xralphack odd, this shouldn't be an issue for uncontrolled

xralphack commented 6 years ago

@scniro you are right, just ignore my comment

scniro commented 6 years ago

@xralphack Cool - no worries. Yea I haven't been able to make a lot of forward progress on this one. It's pretty gnarly on the inside of whats going on as you can see in my investigation above, so hopefully the uncontrolled component is a decent workaround for you, for now.

xralphack commented 6 years ago

@scniro I figured out the problem

I want to wrap the codemirror2 component

// wrapper component

<MarkdownEditor
  markdown={originMarkdown}
  onMarkdownChanged={markdown => this.setState({ markdown })}/>

// codemirror2 component

class MarkdownEditor extends Component {
  ...
  render() {
    return (
      <CodeMirror
         ref="cm"
         value={this.state.initialMarkdown}
         options={{
           keyMap: 'vim'
         }}
         onChange={(editor, data, value) => {
           this.props.onMarkdownChanged();
         }}
    );
  }

use uncontrolled version not work, I think this is due to the parent component rerender so after add the following, it works fine.

shouldComponentUpdate(nextProps, nextState){
  return false;
}

anyway, thanks for your effort.

scniro commented 6 years ago

@xralphack shouldComponentUpdate for UnControlled? Interesting, very. I use this lifecycle hook when responding to new props via the uncontrolled component for resetting values, setting a new theme, etc. Do you think "freezing" the component once it's mounted would solve this via freeze={true}, or something? (Unsure on naming) That prop would essentially do exactly as you shared and prevent any updates. I think that could be valuable if a user actually wanted that behavior, as your use case is showing. Please let me know if that makes sense?

xralphack commented 6 years ago

@scniro I think using freeze makes sense, but I am not sure using boolean prop is ok for all conditions

scniro commented 6 years ago

@xralphack why would it not be okay? shouldComponentUpdate () => false essentially never updates. I am going to add a detachOnMount boolean that does this

xralphack commented 6 years ago

maybe someone want to toggle this boolean on specific condition ( I'm not sure)

scniro commented 6 years ago

@xralphack gotcha. So I'm thinking then of a detach={variable} that can be bound, so whenever it's truthy the component will not update at all, but when false will behave as normal. Does that sound better?

scniro commented 6 years ago

@xralphack okay there is now a detach prop. This should both prevent the component from updating via shouldComponentUpdate lifecycle event and prevent any internal option setting. I may break out the latter option setting to another prop such as detachInternal, but unsure yet as I'd like to gather feedback on how this is used. The new prop also ships with editorDidAttach and editorDidDetach for good measure. These are now available in 4.2.0 and only for the UnControlled component. I hope you find this helpful.

joeldouglass commented 6 years ago

I was able to successfully dispatch the key maps to the mirror like so:

this.editor.on('keyHandled', (...a) => {
  console.log('Editor', a);
  const ke = a[2];
  this.mirror.display.input.textarea.dispatchEvent(new KeyboardEvent(ke.type, ke));
});

(note: I had to use the spread operator to get all three event arguments and avoid a typescript error. I'm not sure what that is all about).

This seems to correctly dispatch to the mirror, because I can listen for keyHandled events on the mirror and watch them fire at the same time. The mirror also correctly fires the vim-mode-change event at the correct time!

However... the behavior is still exactly the same as described in the original bug. So no luck yet.

See below the results of logging the keyHandled and vim-mode-change events on both the master editor and the mirror version.

image

taniarascia commented 4 years ago

Have there been any updates on this? Vim mode is unusable in this component...

scniro commented 4 years ago

@edmondxiexie @taniarascia @joeldouglass @xralphack I am a lot shorter on time these days as when I started this project. Codemirror & React APIs are moving to quickly for me to keep atop of for the day-to-day. I am looking for a co-maintainer of this project. Please contact me directly if you are interested. Thank you for understanding.