Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Commit

Permalink
[Draft] Fix first-character prevention with async rendering
Browse files Browse the repository at this point in the history
Summary:
When typing the first character of a block, Draft always tries to call .preventDefault() on the beforeinput(/keypress) even for that character's insertion, because browsers are likely to mess up the DOM into a form we don't expect.

We check this by reading from the editorState and checking whether the selection is at the start of a block (either at the beginning of a line that is present, or if there is no text in a line yet).

In the case of React Fiber's async rendering, it was possible to get into a state where when inserting two characters AB, A's insertion would be correctly prevented and queued to occur via React rendering, but B's insertion would not get prevented, so it would happen natively even though in many cases A's rendering had not occurred yet.

We already had two copies of editorState we could consider to determine which path to take:

* ._latestEditorState is the last state we have enqueued a rerender for. We always read from this in most of our event handlers so that if you type multiple characters, they are queued in order and don't clobber each other, regardless of whether there has been a rerender in the meantime.
* .props.editorState is the last state that we've run cWRP/sCU/cWU/render on. In sync React, this always matches what we have actually rendered to the DOM (by the time we make it to an event handler). In async React, it doesn't, since we don't always run render in the same tick that we commit.

Checking isSelectionAtLeafStart on .props.editorState is more correct since it more closely resembles the current state of the DOM (and this logic should depend on the actual state of the DOM, not the model state), but it is still incorrect in some cases. I added:

* ._latestCommittedEditorState is the last state that we've committed to the DOM and run cDU on. In sync React, this matches .props.editorState. In async React, it is meaningfully different and does not get updated until the commit, while .props.editorState could get updated during a render that gets paused or aborted.

Changing the logic to call isSelectionAtLeafStart on ._latestCommittedEditorState makes it so we correctly determine when to prevent native character insertion. (I think.)

Test Plan:

Open plain text editor with React's useSyncScheduling set to false.
With the editor empty, press two characters on your keyboard simultaneously.
Previously, this would cause an error dialog (.removeChild of the <br> node within DraftEditorLeaf throws since it's already deleted); now it seems to reliably inserts both characters. I did this 40 or so times in a row without it crashing.
(If changed to .props.editorState but not ._latestCommittedEditorState, it fails about once every dozen or two times on my machine. Could easily be different on another machine due to scheduling randomness.)

I still occasionally saw an issue where you end up with both a <span data-text="true"> and a <br data-text="true"> rendered. I believe this is due to other bugs in async React, not any failing of Draft.
  • Loading branch information
sophiebits committed Jul 12, 2017
1 parent 4ec743e commit 1c6a49b
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 1 deletion.
3 changes: 3 additions & 0 deletions src/component/base/DraftEditor.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ class DraftEditor extends React.Component {
_editorKey: string;
_placeholderAccessibilityID: string;
_latestEditorState: EditorState;
_latestCommittedEditorState: EditorState;
_pendingStateFromBeforeInput: void | EditorState;

/**
Expand Down Expand Up @@ -138,6 +139,7 @@ class DraftEditor extends React.Component {
this._editorKey = props.editorKey || generateRandomKey();
this._placeholderAccessibilityID = 'placeholder-' + this._editorKey;
this._latestEditorState = props.editorState;
this._latestCommittedEditorState = props.editorState;

this._onBeforeInput = this._buildHandler('onBeforeInput');
this._onBlur = this._buildHandler('onBlur');
Expand Down Expand Up @@ -333,6 +335,7 @@ class DraftEditor extends React.Component {

componentDidUpdate(): void {
this._blockSelectEvents = false;
this._latestCommittedEditorState = this.props.editorState;
}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/component/handlers/edit/editOnBeforeInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,9 @@ function editOnBeforeInput(editor: DraftEditor, e: SyntheticInputEvent): void {
return;
}

var mayAllowNative = !isSelectionAtLeafStart(editorState);
var mayAllowNative = !isSelectionAtLeafStart(
editor._latestCommittedEditorState,
);
var newEditorState = replaceText(
editorState,
chars,
Expand Down

0 comments on commit 1c6a49b

Please sign in to comment.