Fix dead key deletion

[sophiebits]

If you type Option-E, Backspace, then the in-progress accented character should be deleted. Similarly, in 2-Set Korean, if you press A, Backspace, the character should be deleted. Both of these don't work correctly yet, but this commit fixes it.

Currently, when you press Backspace, we are in composition mode and we receive a compositionend event then a keydown event for the backspace. Our composition handler doesn't exist composition mode immediately but rather waits 20ms to see if it gets a compositionstart event immediately (and if so, does not exit). It seems like if we receive a compositionend event then a keydown, we always want to exit composition mode immediately and re-interpret the keydown in edit mode. Here I do so.

This change is made trickier by the fact that each event handler reads from this.props.editorState, which doesn't get updated until the next rerender (that is, not synchronously). To combat this, we store the most current editor state in an instance variable and refer to it from all of the event handlers. (If we don't do this then the backspace isn't interpreted correctly by editOnKeyDown because the character data from compositionend hasn't been inserted into the editor state, so we'd end up effectively dropping the entire composition as well as deleting the preceding character.) In this commit, only editOnKeyDown needs the latest state but I changed the others for consistency. In cases where the parent always updates editorState in the same tick (as we recommend in the docs), there should be no behavior change.

Note: This also brings us closer to being able to handle cases where we don't always receive editorState synchronously -- instead we could store our own copy of the editorState locally but overwrite it with props when we receive new ones. It doesn't work yet though.

Test Plan: In plaintext example, type Option-E, Backspace (with US keyboard) or A, O, Backspace, Backspace (with 2-Set Korean keyboard). In both cases, the entire composition is deleted correctly.

cc @hellendag

[sophiebits]

Note to self: need to at least fix the private props here because they're broken by the class transform internally.

[sophiebits]

Looks like this also doesn't quite work in Firefox yet, though it seems no worse than the current code.

[flarnie]

About half done reviewing this, and wondering about your comment "Note to self: need to at least fix the private props here because they're broken by the class transform internally." What is that about, and is more work needed on this before it's ready to merge?

Notes so far:

• Thanks for removing the 'renderGuard' dead code!
• The switch to use this._latestEditorState makes sense; the only way that would cause issues is if users were somehow delaying passing in the updated 'editorState' after their 'onChange' callback was called, and relying on Draft.Editor using the old 'editorState' value. The docs say "For any edits or selection changes that occur in the editor DOM, your onChange handler will execute with the latest EditorState object based on those changes." Am I missing anything there?
• The way the handlers are determined by the current 'mode' using 'buildHandler' in DraftEditor is neat, and I wonder if there is a name for that pattern. cc @hellendag

[sophiebits]

About half done reviewing this, and wondering about your comment "Note to self: need to at least fix the private props here because they're broken by the class transform internally." What is that about, and is more work needed on this before it's ready to merge?

In the Facebook web codebase, we transform class Foo { ... this._bar ... } to class Foo { ... this.$Foo_bar ... } which prevents you from accessing private props from another file (specifically, from outside the class). In particular, both _latestEditorState and _onKeyDown need to be renamed here (or we can add @preventMunge to DraftEditor.react, but I was already nervous about using the private props cross-file so I'll just change it). A little more info at https://fburl.com/441040622 for FB employees.

if users were somehow delaying passing in the updated 'editorState' after their 'onChange' callback was called, and relying on Draft.Editor using the old 'editorState' value

We actually already don't support that: https://facebook.github.io/draft-js/docs/advanced-topics-issues-and-pitfalls.html#delayed-state-updates. This change probably brings us closer to being able to support that if we really wanted, actually.

The way the handlers are determined by the current 'mode' using 'buildHandler' in DraftEditor is neat, and I wonder if there is a name for that pattern.

Hmm, maybe related to delegation? https://en.wikipedia.org/wiki/Delegation_pattern

[sophiebits]

Most of these changes are just mechanical .props.editorState -> ._latestEditorState changes. Just adding ._latestEditorState in DraftEditor and the changes in the composition handler are the real changes here.

[sophiebits]

This was necessary so stillComposing in the composition handler gets set to true when first entering composition mode.

[sophiebits]

I decided to go with just adding @preventMunge because I don't want clients of DraftEditor thinking that it's okay to use these properties so I want to keep the underscore. Going to test in www.

[flarnie]

Thanks - I'll try to read the rest of this later today.

[flarnie]

Cool - so it's more like this:

It used to work like this:

                                 +--DraftEditor---------------------------+
                                 |                                        |
               event+----------->+ triggers '_update'                     |
                                 | ---> which calls 'this.props.onChange' |
                                 |      with the updated editorState      |
                                 |                                        |
                                 |                                        |
                                 |                                        |
                                 +----------------------------------------+

                                                      +
                                                      |
                                                      v

                                 +--DraftEditor---------------------------+
                                 |                                        |
           updates  <------------+ calls 'this.props.onChange'            |
           state.editorState     |                                        |
              +                  |                                        |
ANY DELAY     |                  |                                        |
HERE   XXXXXXX|                  |                                        |
CAUSES        v                  |                                        |
PROBLEMS   re-renders &          |                                        |
           passes in             |                                        |
           state.editorState +--------> We use updated                    |
           via props             |      this.props.editorState            |
                                 |                                        |
                                 |                                        |
                                 +----------------------------------------+


But now it works like this:

                                               +--DraftEditor---------------------------+
                                               |                                        |
                             event+----------->+ triggers '_update'                     |
                                               | ---> which calls 'this.props.onChange' |
                                               |      with the updated editorState      |
                                               |                                        |
                                               |                                        |
                                               |                                        |
                                               +----------------------------------------+

                                                                    +
                                                                    |
                                                                    v

                                               +--DraftEditor---------------------------+
Delays here could                              |                                        |
be OK because            updates  <------------+ calls 'this.props.onChange'            |
DraftEditor is           state.editorState     |    AND also updates                    |
*not* using                 +                  |    this.latestEditorState              |
props.editorState           |                  |                                        |
and it has      +---------> |                  |                                        |
already updated             v                  |                                        |
this.latestEditorState   re-renders &          |                                        |
                         passes in             |                                        |
                         state.editorState +--------> We can use                        |
                         via props             |      this.latestEditorState            |
                                               |      and not worry                     |
                                               |                                        |
                                               +----------------------------------------+

Overall I think this makes sense, and when I manually tested it (briefly) it works.

I'm working on the internal sync of 0.10.0, which includes a big API breaking change. I can either add this to 0.10.0 or do a 0.9.2 release. Either way I think it would be great to get this bug fix out soon. Thanks for working on it!

[sophiebits]

Yes, that's basically right! It's that we basically wanted two successive events with nothing in between and before we weren't getting the new editorState when processing the second event, because state updates are batched and we hadn't rerendered.

[doortts]

@spicyj Thanks you for your PR and I'm expecting merged version will be released soon (Because I'm using Korean.) And I wonder another similar problem is solved or not with this PR. The problem is Left and Right arrow key doesn't work properly with Korean IME because of following codes at 0.9.1 https://github.com/facebook/draft-js/blob/master/src/component/handlers/composition/DraftEditorCompositionHandler.js#L96-L98

When I press arrow key, normally I expect to move cursor but it doesn't move and just change editor mode to composition end. Unlike Japanese or Chinese, Korean IME doesn't use arrow key to select or finish composition. In Korean (same as English), arrow keys are used as arrow key whether IME is in composition or not.

@spicyj Cloud you test your PR also solve this problem or not?

[sophiebits]

Yes, it should. This Wednesday you should be able to confirm that typing works correctly on facebook.com. If not, please file a new issue and tag me in it describing exactly which sequence of keys behaves incorrectly, with your OS and browser version.

[TylerShin]

is it applied to npm?

if it is applied, I think 2-set korean removing still has problem. 😢

[sophiebits]

No, it is not yet.

[TylerShin]

too bad. But I really appreciate your work. I'll wait for the update.

[KendoJaaa]

Sorry @spicyj I'm new to Coding. Does this solve the Enter key problem I have to Enter twice to go to the new line? the first time to trigger compositionend. thx in advance

[sophiebits]

Yes, it should.

[KendoJaaa]

thank you I'll test it 👍 you response so fast