Add onLeftArrow and onRightArrow props

[noahlemen]

Summary

While implementing some custom selection logic, I found it somewhat unintuitive that onUpArrow and onDownArrow are exposed as props, but onLeftArrow and onRightArrow are not.

Given the added complexity due to the workaround needed by firefox for handling CMD+LEFT/RIGHT, it seems to me that it may make sense to have these handlers return DraftHandleValue (like handleReturn, etc) so that they can still fall through to getDefaultKeyBinding.

Is this appropriate? If so, would these props be better named handleLeftArrow and handleRightArrow? Should any other on* props be refactored to use this logic?

Test Plan

As far as I can tell there are no existing tests for the modified files. Did I miss any? Should any be added?

⚠️ This is my first OSS PR, so I apologize in advance if I've missed any PR etiquette and such. Love draft-js and hope I'm able to help contribute to it 😄

Thanks for taking the time to check out this PR!

[flarnie]

:) Thanks for the great commit message and for contributing @kedromelon! I'm curious what your Draft use case is, and how you would use onLeftArrow and onRightArrow.

You are right that it seems silly to expose onUpArrow and onDownArrow but not onLeftArrow and onRightArrow. You're also right that, following the pattern of handleReturn we would want to return a DraftHandledValue from the callback in order to allow falling back to the default behavior in getDefaultKeyBinding that fixes that FF bug.

I am still thinking about the question of how to be consistent once we have made that change - it looks like handleReturn has been treated as a special case and the other key handlers are not treated as cancellable.

Some options we may consider;

• Update all on<Key> props to be cancellable, meaning they must return a 'DraftHandledValue' or true in order to actually override the default behavior of the editor. But for now we don't have any special behavior for any of those.
  • +++ This leaves room for adding more custom behaviors within getDefaultKeyBinding.js in the future, if more browser specific bugs were to arise, like in the case of left and right in older FF versions.
  • --- We would need to consider how to do this in a way that doesn't break behavior for folks using on<Key> props currently.
• Deprecate the on<Key> props and replace with handle<Key> props that return a 'DraftHandledValue'.
  • +++ The naming of props would be consistent between handleReturn and the other key handler props.
  • +++ This also leaves room for adding more custom behaviors within getDefaultKeyBinding.js in the future, if more browser specific bugs were to arise, like in the case of left and right in older FF versions.
  • --- This does break the API and would require folks to migrate their changes. We could probably publish a codemod to make it easier though, since it's just a rename and possibly wrapping the callback in something that returns 'handled'. I would delay releasing that type of change until after v0.11.0 is released though, due to other API changes happening.

Either way it would be nice to add an example, or at least know some possible use cases for these props.

Sidenote:
I found some notes about the FF bug that was being fixed by adding the left and right key handlers for Firefox, and will probably add a bit more detail to the inline comments about that, for posterity. Mainly a link to the bug report, and more detail in getDefaultKeyBinding.js.

[tobiasandersen]

Just thought I'd chime in hear. We've created our own version of atomic blocks, that are selectable. In order for them to be that, and still play nicely with the "normal selection", we need to tell Draft what to do with the different arrow key events. We also have to see if shift is held down, and in that case expand the selection rather than moving it. We're actually intercepting a lot of different key events to make it work well, and it's a little bit annoying that they're handled using different naming conventions and with different return types.

Some more thoughts about the arrow events: the recommendation seems to be to use the keyBindingFn to create commands for handleKeyCommand to take care of. Using the current API, this is not possible to do with the up and down arrow keys, but it is for left and right. While this isn't preventing us from creating what we want, the solutions for seemingly similar operations look quite different.

Having said that, I think @flarnie's second option sounds best, since all key events would be handled, and named, in a similar way. For someone unfamiliar with the code base, I don't think it's clear how and why handleReturn is different from onArrowUp (except for the actual keys).

[noahlemen]

@tobiasandersen Yep! I have a similar use case. Essentially trying to create Medium-like selection of Atomic Blocks, not unlike what was discussed in #203.

I would agree that @flarnie's second option sounds better. As mentioned above, having all of these "handlers" (for lack of a better term) named handle<Key/Action> and returning DraftHandleValue would be much clearer. It certainly took me a bit of research to understand the difference between on<Key> and handle<Key>.

I'll try to see if I can get an isolated example put together soon.

Thanks for the input, @flarnie and @tobiasandersen!

[noahlemen]

Okay, got a chance to put together an example.

Essentially, what I'm trying to illustrate is the inconsistent API between up/down and left/right arrows. In the example, I want to handle key commands myself when the cursor is adjacent to an atomic block.

[flarnie]

Thanks for your patience on this PR - I have been following the conversation, and need to fix some other issues with Draft before giving this my full attention. It is still on my radar.

[noahlemen]

👌 no rush! was able to work around it just fine for my use case, but i do think this API is worth improving.

[octatone]

Deprecate the on props and replace with handle props that return a 'DraftHandledValue'.

Just wanted to put in a vote in favor of this solution. I have been using draft-js for the past months for a project and have found the API for custom key handling inconsistent. I would prefer a one-way-to-rule-them all API.

[noahlemen]

🎉 saw this functionality was merged in #1384! gonna close this issue.