Close a race after handling before input

[colinjeanne]

Summary

The before input event is not a browser event, it is emulated by React. Draft uses this event in order to set up its expectation for the state of the document after the browser has had a chance to process user input and it relies on that expectation later when the input event comes in. Since before input is emulated this sets up a race condition in which the state of the DOM can change out from under Draft leading to a very broken experience.

This change closes this race condition by attempting to handle as much as possible during the before input event and by handling the case that the selection has changed between the time that the before input was handled and when the input event is handled.

This fixes #989

Test Plan

I've attempted to do some manual testing on this change. It has also been part of the textioHQ fork and running on our live site for almost a year.

Before:

After:

[ukrbublik]

Is this a duplicate of closed PR #1042 ?
Are you sure we don't need this code?

  if (blockChangedSinceBeforeInput) {
    // Trust window.getSelection() because that's all we have
    ...
  }

[ukrbublik]

+1 for this fix!
It's also present on facebook.com :(

Also I have to mention some more serious side effects of this bug.
See my attached gifs.
How to reproduce: After simultaneous text input & mouse click on another block, when you see issue on 1st gif in OP-post, I tried to press Ctrl+Z several times and then input more text, press Enter etc..

[colinjeanne]

Yes, this is an update to the older PR. @max-winderbaum was one of the people who worked on our solution for this quite a long time ago and this version is what we have been running.

[tberman]

Any update here, this is biting us in production. Happy to merge it into our fork, but curious about the upstream team's thoughts on the issue and fix.

[uvwxy]

+1 to add this

[colinjeanne]

As an FYI to anyone watching this: as I come back to this after a few months and try to sync it with what's in master and the test failure has actually filled me with a sufficient amount of doubt about the implementation that I need to think more about this to validate it is doing exactly what I think it should be doing.

The danger is that this change never allows native insertion to occur properly.

[niveditc]

As an FYI to anyone watching this: as I come back to this after a few months and try to sync it with what's in master and the test failure has actually filled me with a sufficient amount of doubt about the implementation that I need to think more about this to validate it is doing exactly what I think it should be doing.
The danger is that this change never allows native insertion to occur properly.

Considering this comment, I'm going to close the PR for now. Feel free to reopen if you figure out a better fix / want to iterate on it :)