codemirror/codemirror5

vim use of keydown and keypress preventing codemirrorIgnore?

dalejung opened this issue · 14 comments

As far as I can tell, the move to on('keydown') and on('keypress')` is circumventing the normal way of preventing codemirror from doing things with events.

What's the proper way to register a keyevent handler that can prevent the keymap from firing?

diff --git i/keymap/vim.js w/keymap/vim.js
index 558e4c8..f884f7b 100644
--- i/keymap/vim.js
+++ w/keymap/vim.js
@@ -251,6 +251,7 @@
     // Keys with modifiers are handled using keydown due to limitations of
     // keypress event.
     function handleKeyDown(cm, e) {
+      if(e.codemirrorIgnore) { return; }
       var name = lookupKey(e);
       if (!name) { return; }

@@ -262,6 +263,7 @@
     // Keys without modifiers are handled using keypress to work best with
     // non-standard keyboard layouts.
     function handleKeyPress(cm, e) {
+      if(e.codemirrorIgnore) { return; }
       var code = e.charCode || e.keyCode;
       if (e.ctrlKey || e.metaKey || e.altKey ||
           e.shiftKey && code < 32) { return; }

this helped give me the behavior that I wanted. The other issue is that I have to modify the _handlers array directly since on() doesn't give me the ability to insert a handler before the vim keymap handlers.

You could register your handler before turning on the vim mode, i.e.

var editor = CodeMirror.fromTextArea(...);
// register handlers
editor.setOption('vimMode', true);

and that should put your handlers before vim gets it.

How would I cancel vim handling for certain events?

I suppose vim mode could check for preventDefault and codemirrorIgnore values on the event and ignore it if either is set. Would that solve your problem?

Yeah, that how I patched my local CodeMirror. I think it's the wrong place to handle it, but we'd need to split handlers into app and internal in order to remove it.

Tricky problem. @marijnh do you have any suggestions on a good way for clients to suppress keypress and keydown handlers registered on CodeMirror instances?

I think it would be preferable to move the vim mode's key handling down into the keymap layer again, which has at least some concept of priorities and fine-grained handling. Would it work for you to be able to add a function to a keymap that is simply called for each key, and which can return a value indicating whether it chose to handle that key? You'd have to translate from CodeMirror-style key names to Vim-style names, but that's probably not very hard.

Yes, that's probably a good idea. I didn't realize until after I finished the insert mode key handling work that I didn't actually have to move the key handling out of the keymap layer.

A function on the keymap that is called for each key would make the vim code cleaner if we take this approach.

Attached patch should allow you to give the vim keymap(s) a call method (or just make them functions). This method will be called with the name of a key, and should return null if there is no binding, and a function when there is one. That function will then be called with a CodeMirror instance as argument to actually execute the binding's action.

You might also want to take a look at the new functionality for multi-stroke key bindings. Basically, if you return "..." from a keymap, the key will be stored, and combined with the next key, so that if you press Ctrl-X and that returns "...", and then you press F, the string "Ctrl-X F" will be dispatched to the keymap. This might be too primitive for the vim map's use, I'm not sure.

By the way, if you need access to the editor state during dispatch, I am also open to passing the editor instance as second argument to the keymap function. It's less clean, but I could live with it.

I took a crack at this today, and realized I do need the editor instance. Vim mode's key mappings can change depending on its current state

Attached patch causes the editor instance to be passed as second argument to the keymap's call method.

I believe this issue is preventing Light Table's Vim from upgrading - see LightTable/Vim#45

@cldwalker This should be resolved on Monday when I sync up with @heppe