defer native events within Editable to avoid bugs with Editor

[jaked]

Description
In #3888 some insertText events are allowed to run their default native behavior, in order to avoid re-rendering DOM nodes (re-rendering defeats some browser behaviors—in particular, showing spellchecking squigglies only when typing has stopped). The corresponding Slate insertText operation is deferred until the onInput handler; after the Slate operation runs, the DOM already matches the new Slate state, so React does not re-render the node.

In the original #3888 code, this is achieved by setting a global flag AS_NATIVE in onDOMBeforeInput, then running editor.insertText. The withReact plugin overrides editor.apply to add Slate ops to a queue when AS_NATIVE is set; then this queue is flushed in the onInput handler. However, this interacts badly with other plugins that expect ops to be applied immediately (see #4530, #4541).

To achieve the desired result, it's not necessary to defer individual Slate ops; it's enough to defer the whole insertText call.
This can be done entirely in Editable, without involving the Editor implementation, so that's what this change does. Note that it doesn't try to bail out when insertText is overridden (as #3888 does in editor.apply), but I think this is fine—in that case a re-render is triggered, so the effect of the native event is overwritten.

(I guess there are possible overrides of insertText where this is not true—one which skips some inserts, or re-renders a different part of the tree. If this is a valid use case then maybe a deeper change is needed.)

Issue
Fixes: #3888 #4530 #4541

Example
Old behavior showing spellcheck squigglies (current code with native always false):

New behavior showing proper spellcheck squigglies

Old behavior showing wrong undo behavior:

New behavior showing correct undo behavior:

Context
I am not totally sure why it's necessary to defer the Slate operation. If my explanation above about how #3888 works is correct, then it shouldn't really be necessary—the React redraw is already deferred with ReactDOM.unstable_batchedUpdates in the onChange from withReact.

I think maybe it's necessary to wait for onDOMSelectionChange to fire in order to update Slate's view of the selection to match the changed selection due to the native operation, or else the Slate operation can fail because the selection is wrong. So waiting until onInput gives onDOMSelectionChange a chance to run first. I haven't investigated this theory however.

On second thought this theory doesn't make sense—we want the Slate selection to be in the original spot before running the Slate operation. Still, if we don't preventDefault and also don't defer the Slate operation, we get a Cannot resolve a DOM point from Slate point, so somehow the selections are out of sync.

Checks

• The new code matches the existing patterns and styles.
• The tests pass with yarn test.
• The linter passes with yarn lint. (Fix errors with yarn fix.)
• The relevant examples still work. (Run examples with yarn start.)
• You've added a changeset if changing functionality. (Add one with yarn changeset add.)

[changeset-bot]

🦋 Changeset detected

Latest commit: a8c431e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
slate-react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[nemanja-tosic]

I think the initial couple of discussions started on slate 0.4x, and the implementation was ported to slate 0.50+ at which point some of the changes stopped being relevant, maybe blocking operations was one of them. Anyway, the original PR i believe contained some ports that should not apply to slate 0.50+.

On thing i can't figure out right now is if this forced re-rendering is necessary - https://github.com/ianstormtaylor/slate/blob/main/packages/slate-react/src/components/string.tsx#L61 as it came as part of #3888. I've removed it and had less remounts, but curious if anyone else can take a look.

Couple of suggestions added - capture the op and apply it later - closes onInput for further changes if new types of native ops are added (removes the switch basically).

[echarles]

Trying to replicate the issues with latest main branch:

• Old behavior showing spellcheck squigglies (current code with native always false): I have difficulties to see if the spellcheck squigglies or not. I don't think it does.

Old behavior showing wrong undo behavior: With the last commit in examples, move withReact in front of withHistory #4604 the undo behavior works fine even without this PR.

Side question: slate-react use reaction v16. What would be the consequence to migrate to react-17, cfr changes in the event delegation as documented on https://reactjs.org/blog/2020/08/10/react-v17-rc.html#changes-to-event-delegation

[echarles]

I have done a quick test with react 17 and main branch behave the same on my env.

For the spellcheck squigglies, it only squigglies when I type a whitespace. Is this expected?

[dylans]

I have done a quick test with react 17 and main branch behave the same on my env.

For the spellcheck squigglies, it only squigglies when I type a whitespace. Is this expected?

Yes, the idea is to not show spellcheck errors until you've had a chance to finish typing a word which matches typical behavior.

[nemanja-tosic]

I have done a quick test with react 17 and main branch behave the same on my env.

For the spellcheck squigglies, it only squigglies when I type a whitespace. Is this expected?

For whatever reason spellcheck in slate works on the entire "element" as opposed to the word you just finished.

I have been trying to figure this one out and have not been able to - the default behavior of the spellchecker is to show squigglies whenever a word is finished, as you can test in a vanilla contenteditable. I wasn't sure if it's react doing this, so i double checked by looking at the form example and the answer is no - squigglies still have the same behavior. Then i removed this guy (not sure how to call it) to make sure it isn't the re-mounts, and it isn't that either.

Any ideas would be appreciated.

[echarles]

@jaked I have further tried this PR in various cases and found the following issues:

1. Redo does not work anymore. When you type `CTRL-SHIFT-Z' after some undo action, nothing is shown in the editor (no error in the console either).
2. Typing at the end of the decorator as reported on https://slate-js.slack.com/archives/C1RH7AXSS/p1634146964395500 : If you run https://codesandbox.io/s/slate-anchor-decoration-8rg8k with this PR, you won't get exeception anymore when typing at the end of the decorated link, but the character is still double inserted.

[echarles]

@jaked Did a clean build, and now undo/redo works great with this PR. Sorry for the confusion here.

Regarding my 2nd point for the issue reported on slack https://slate-js.slack.com/archives/C1RH7AXSS/p1634146964395500, I guess this is out of scope of this PR. Just wanted to give it a try to see if it would bring changes.

[jaked]

On thing i can't figure out right now is if this forced re-rendering is necessary - https://github.com/ianstormtaylor/slate/blob/main/packages/slate-react/src/components/string.tsx#L61 as it came as part of #3888. I've removed it and had less remounts, but curious if anyone else can take a look.

I think it covers the case where the native event changes an element that the Slate operation does not change, so without the forced re-rendering the element changed by the native event is not updated. This leads to double-character bugs, see discussion towards the end of #4445. It's easy to trigger with decorations, probably possible to trigger other ways. It should only cause a remount when the DOM text doesn't match the Slate text.