try to prevent re-rendering at the Leaf level

ianstormtaylor commented 5 years ago

Do you want to request a feature or report a bug?

Idea/debt.

What's the current behavior?

Right now, we re-render the DOM in Slate with every change that occurs in the editor. This is different from Draft.js (which also uses React), which aborts re-rendering for "simple" changes to the DOM, like inserting or deleting characters.

Our current approach has a lot of benefits in terms of simplicity...

It makes it easy to ensure that schema validations can be applied even at the text level. Since all changes will result in re-rendering, we don't have to differentiate and handle the "simple" changes in a more complex way to account for them already having been applied to the DOM, but now needing to be normalized.
It makes it possible to create editors which render elements that update for any change, even basic ones like inserting text. Since every changes results in a re-render, plugins can render things like word counters, or block-specific styles, easily without having to account for the fact that some changes don't actually trigger renders.
It makes all of the logic in the Before/After plugins a lot easier to follow, since they don't have to reverse engineer what the specific changes the browser may or may not have made in the contenteditable element out from under React.

However, even right now we're not doing it for 100% of DOM operations. We currently don't do it for spellchecking, since those changes are applied to the DOM immediately, without going through a usable event (at least in most browsers).

But this "always re-render" approach also has some downsides...

It makes old browser and mobile browser support harder. Since these browsers use a system where the beforeinput event can't be prevented, preventing it makes it hard (and/or impossible) to get Slate to work in these browsers. https://github.com/ianstormtaylor/slate/issues/2047 https://github.com/ianstormtaylor/slate/issues/1720
It makes IME support harder? Unsure of this one. But since IME is also a case where we can't actually prevent the defaults, since we need to read the text in the DOM, this might be a similar situation.
It makes spellcheck glitchier, since browsers blink when text is re-rendered before they spellcheck it again. https://github.com/ianstormtaylor/slate/issues/1934
It's less performant, in the case of simple insertions. Not terribly so, and for most editors this isn't a bottleneck, but it does require re-rendering the DOM even for "simple" cases like inserting a single character. This is not a big reason for doing this, since there are many other places that would be important to improve first for this.

So, where does that leave us...

What's the expected behavior?

First, a look at what the render tree looks like for Slate:

<UsersApp> (userland)
  <Editor>
    <Content>
      <Node>
        <UsersNode> (userland)
          <Text>
            <UsersMark> (userland)
              <Leaf>
      <Node>...
      <Node>...

The important thing that differentiates Slate from other React libraries/frameworks is that it allows for "userland" islands of rendering inside its own rendering layers. For this reason, if you use shouldComponentUpdate to abort rendering at the <Editor> level, the <UsersNode/UsersMark> userland levels will not re-render, breaking expectations.

We used to actually do the same thing as Draft.js, and let the DOM be updated by the browser for "simple" changes, and then aborting our own rendering. However, as mentioned above, this prevents us from doing certain things, because it aborts rendering at the <Content> level in the diagram above, which means that the <UsersNode> (userland) never gets re-rendered.

We removed this logic a long time ago, because we thought it was necessary to allow for user-defined schemas to be validated and to allow for more complex node rendering behaviors. It was before we had Operation level semantics. But also because we were able to memoize the Immutable.js structures, which meant it was no longer a required case for performance.

We might need to bring it back in some form though, for mobile support, IME support, and graceful spellcheck support.

Instead, I think it may be possible to re-render at the <Editor> level for each change, but abort rendering at the <Leaf> level, which is the lowest component Slate renders.

This would be different from our old approach in that it would allow us to gain the "always re-renders" benefits higher up the tree, so that custom nodes can still have total flexibility in what they render. But it would hopefully give us the benefits of "selective re-rendering" that come from allowing the browser's native DOM editing to occur.

It would preserve one of the constants that (I think) is required for Slate's flexibility, which is that userland can always count on the editor re-rendering as if the DOM did not exist. (Kind of like the same tenet React offers for the regular DOM, but for contenteditable too.)

I think the way to do this best might be to add an isNative flag (like we used to have on Value objects) to Operation objects instead. This way we might be able to consider in <Leaf> nodes whether or not to re-render if all of the operations reference the leaf and are native, then abort rendering.

The newly added paths can also help because operations are path-based, and can hopefully be directly mapped to the leaves in the tree.

This is just an idea, I'd love others's thoughts if you see any issues. It will definitely result in more complexity in core, but hopefully it unlocks some compatibility.

thesunny commented 5 years ago

@ianstormtaylor Just thinking this through and how we might handle the Android issues.

I wonder if we need a concept in here to do with uncertainty and finalization.

For example, autosuggest, autocorrect and IME may result in Slate not being able to predict what is in the DOM. So as we type characters, we aren't sure what is in the DOM. Then, at some point, we finalize our entry and then we have to reconstruct the actual operation from what is in the DOM.

According to nathanfu in this document https://docs.google.com/document/d/1Hex89Di-r-Wfpo1DLAtxpetoX588ziXVoNyC87Je3Xc/edit# it doesn't seem likely that we can predict DOM state reliably across all versions of mobile Android browsers using events. Maybe at some point, old versions disappear and new API implementations may be able to get us there but for now, I think it's impossible.

In order for this to work with collaborative editing, I presume we would have to freeze incoming operations until a finalize event occurs.

Maybe it works something like this across browsers for typing "Hello" with some sort of autosuggest that can complete after "He" so in most browsers we get:

// Note: Pseudocode. Probably not the actual operations...
==> keydown "H"
{op: 'insert', value: 'H', isNative: true}
==> keydown "e"
{op: 'insert', value: 'e', isNative: true}
==> autocomplete: "Hello"
{op: 'insert', value: 'llo', isNative: true}

The above assumes in most browser we are able to reliably predict state through events.

In Android without reliable prediction we get

==> keydown "H"
{op: 'entry', isNative: true}
==> keydown "e"
{op: 'entry', isNative: true} // a noop
==> autocomplete "Hello"
{op: 'finalize', isNative: true}
==> Slate intercepts the `finalize` and then the history is rewritten to:
{op: 'insert', value: "Hello"}

During finalize Slate reads the DOM in order to reconstruct the actual op. What do you think?

I think the other idea that goes along with this is that entry and finalize are ops that do not get sent during collaborative editing. We wait for finalize to fix the op to an insert and then that gets sent.

thesunny commented 5 years ago

@ianstormtaylor One other thought, @mattkrick suggested we move the iOS specific code out of Content which is, I presume, the code I added to prevent autoscroll in iOS due to it not working in that browser.

Should we perhaps move hints like shouldScroll into the op? These can probably all be ignored when used in collaboration or in undo/redo but could be useful in putting some code closer to where they should be. For example, we could have an iOS Plugin that adds {shouldScroll: false} to the op only when in an iOS browser; but furthermore, there may be other cases where we don't want scrolling to happen during an update.

zhujinxuan commented 5 years ago

About IME, it is another problem. IME sometimes triggers beforeInput if you do not cancel the typing. But if you cancel the typing, like blur the editor while typing with IME. It will trigger an Input event.

Draft.js has a method editor.resolveComposition, which is triggered when a composition end event is not followed by beforeInput event.

PS: I am working on neither spell-checking nor IME in recent days. I may work on them on Oct when I have upgraded my project and plugins with the newest slate.

zhujinxuan commented 5 years ago

For mobile support, I am not sure how to deal with the following situation without re-rendering the parent

<leaf>a<anchor/></leaf>
<leaf>b</leaf>
<leaf><focus/>c</leaf>

then press d

<leaf>ad<cursor/></leaf>
<leaf>c</leaf>

The middle HTML element is deleted if the beforeInput cannot be cancelled, while the react will try to unmount this HTML element in the re-rendering process. (Perhaps shouldComponentUpdate is null when is deleted, and expose the setState(({parentKey}) => ({parentKey: parentKey+1})) to all children?

ianstormtaylor commented 5 years ago

@thesunny I think as far as "iOS specific code" that was referring to the onNativeBeforeInput logic that's currently inside <Content> but should really be inside the After plugin instead. Which makes sense, it was pull requested that way a while back, and the only reason it hasn't been refactored is for lack of time. The selection logic is not bad, although it too will probably need to be tweaked to accommodate aborting re-renders.

I think augmenting operations is a good way to go. That said, I think it would be best to try to limit the number of total operations, ideally by not adding any new ones. To do that, we often can suss out the true "data" of an operation and use that, instead of introducing a new type.

For example, in the selection updating case, I think having an isLocal: true boolean (or similar) on operations might be enough to know that these operations should be scrolled to when not in view? Either that, or potentially augmenting set_selection to somehow include that flag instead.

The collaborative editing concern is a good point though. We might be able to solve it by instead queueing incoming operations while isComposing === true, and then flushing the queue after that happens? (Or even flushing right before the composition takes effect.)

@zhujinxuan I think you're right, that situation would need to end up re-render the parent <Text> node completely. I think that might be solvable? We might need to look at the path/key that operations occur on and be able to "group" them together and "lift" the decision to parents when multiple leaves are edited at once.

To make this easier to think about, I've opened a series of issues:

zhujinxuan commented 5 years ago

Hi, @ianstormtaylor @thesunny . If I am not wrong, the ime events in android will trigger onNativeBeforeInput rather than onBeforeInput, and onNativeBeforeInput is also cancellable that by event.preventDefault can prevent the following onNativeInput?

ianstormtaylor commented 5 years ago

@zhujinxuan I think it's unclear right now which native beforeinput events are actually cancellable. Safari's are almost all cancellable. Chrome's supposedly are not, but I've seen insertText events be cancelled fine. And then supposedly none of the beforeinput events fired during compositions are cancellable.

Would need someone to research it.

ianstormtaylor / slate