handleKeyCommand doesn't honor "not-handled"

[michaelcox]

Do you want to request a feature or report a bug?

bug

What is the current behavior?

Currently if you provide a handleKeyCommand function, it doesn't fall through to the default key binding if you return not-handled. I would expect that if you returned not-handled from that function that it would allow the underlying key press to go through.

From the docs:

The 'myeditor-save' command can be used for our custom behavior, and returning 'handled' instructs the editor that the command has been handled and no more work is required.

By returning 'not-handled' in all other cases, default commands are able to fall through to default handler behavior.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.

Here is an example: https://codepen.io/michael_cox/pen/xrvdRW

The important part is this:

function myKeyBindingFn(e) {
  if (e.keyCode === 65 /* a key */) {
    return 'editor-a';
  } else if (e.keyCode === 66 /* b key */) {
    return 'editor-b';
  }
  return getDefaultKeyBinding(e);
}

handleKeyCommand(cmd) {
  if (cmd === 'editor-a') {
    return 'not-handled'; // Should fall through to default key functionality?
  } else if (cmd === 'editor-b') {
    return 'handled';
  }
  return 'not-handled';
}

What is the expected behavior?

I would expect that in this case if you type the letter a, which returns not-handled, that it would let the underlying keypress to go through and the letter a would be typed to the editor. If you type the letter b, which returns handled, it should not go through and the editor would assume that keypress has been handled some other way. Currently the keypress is stopped in both cases.

I think this may be caused because of this: https://github.com/facebook/draft-js/blob/master/src/component/handlers/edit/editOnKeyDown.js#L150

It says at this point it's safe to stop the keypress, but I think that's probably only true if the next if statement passes where it checks to see if there was a handled event of some kind.

Which versions of Draft.js, and which browser / OS are affected by this issue? Did this work in previous versions of Draft.js?

Chrome 59, 0.10.1 of Draft.

[colinjeanne]

This seems like reasonable behavior to me. By returning editor-a from key binding function you are already saying that this key combination is not a normal key press.

[michaelcox]

My question would be what returning not-handled from handleKeyCommand should do? What was the original use-case that's working?

I interpreted this functionality as "bind to keys you care about within keyBindingFn", but then decide how/when/if to use them as they're pressed in the handleKeyCommand function.

My specific use-case is I want to be able to trap on the backspace key to optionally behave differently if the editor is empty. So most of the time I want it to actually backspace/delete the text, but if it passes a "is the editor empty" check, I want to handle things myself and override the keypress. And it doesn't make sense for me to re-implement the backspace functionality I should get for free.

It turns out though that keyBindingFn is called on each keypress. I would have assumed it would be called only once when the <Editor /> was mounted. Since it calls it every time, the workaround I have is to bring myKeyBindingFn within the container component and check state in that function instead. If that's the recommended approach, then I guess this isn't a bug.

[colinjeanne]

My expectation is that if you return a command in keyBindingFn and never handle it that the editor would do nothing because it similarly doesn't know how to handle your special command.

And yes, for complex key interactions I regularly examine the current editor state within a key binding function. I use draft-js-plugins to help make this cleaner.

[michaelcox]

One last suggestion then, for posterity if nothing else. It would seem that the <Editor /> doesn't need to support both props. Since keyBindingFn is called on every key press, and returning getDefaultKeyBinding(e) works as I'd expect it to, handleKeyCommand doesn't seem to provide anything additional. If you wanted to call a separate function like that you could do so manually without relying on the framework.

But I still stick by my original post that the way things are currently implemented feels incorrect.

[aryraj]

It turns out though that keyBindingFn is called on each keypress. I would have assumed it would be called only once when the <Editor /> was mounted. Since it calls it every time, the workaround I have is to bring myKeyBindingFn within the container component and check state in that function instead. If that's the recommended approach, then I guess this isn't a bug.

Could you please elaborate on your workaround? I have been facing a similar issue. When using the keyBindingFn and handleKeyCommand and on the press of backspace, 'backspace' is being written in the editor.

[andria-dev]

I'm facing a similar issue as well. Currently, I'm trying to apply inline and block styles based on the input using a regular expression, and remove the surrounding characters that denoted that style. For example, *bold me* turns into bold me. With the current handleKeyCommand implementation if I type * it will never show up even if I return 'not-handled';.

I think a possible solution is to define the keyBindingFn function in my component, that way it has access to the current editorState value and can decide whether or not to return getDefaultKeyBinding(event); or handle the command itself.

Another way to do the same thing without cluttering your component is to define a function that takes in the editorState (or whatever you want honestly, like dispatch) and returns a keyBindingFn function that now has access to those variables.

This way, the handleKeyCommand function becomes totally obsolete (it kinda already is) and I have the power to bail out and let the key still be typed.

[cr7cr8]

use return undefined , instead of return "not-handled" https://github.com/draft-js-plugins/draft-js-plugins/issues/1117