ianstormtaylor / slate

A completely customizable framework for building rich text editors. (Currently in beta.)
http://slatejs.org
MIT License
29.65k stars 3.23k forks source link

Copy pasting (really) large text no longer seems to work? #4056

Open pietrop opened 3 years ago

pietrop commented 3 years ago

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

A bug, I've also mentioned it in the slack channel

What's the current behavior?

If you take the plain text of a book, eg Moby Dick from Gutemberg's books and you copy paste it in  SlateJs(plain text demo) It freezes the UI.


Slate: 0.XX.X // Whatever slateJs version you have on slatejs.org? Browser: Tried on Brave/ Chrome / Safari / Firefox OS: Mac

What's the expected behavior?

For the UI not to freeze when pasting large body of text.

It used to work a while ago, when I had tried this before, that the UI did not freeze, and it was surprisingly quite performant at handling such a large document, eg if you tried to delete a paragraph, there was very little lag.

You can try to do the same thing in draftJs and you'll see, you'll be able to copy/paste the whole text. And that if you delete a paragraph there's a few seconds delay before the changes takes effect.


sandbox 1 Since you ask for sandbox env to reproduce, I splitted the books line into draft paragraphs, and added it to a sandbox - altho that is not quite the sam as reproducing the bug describe above. ```js const text = `//string containing moby dick book from gutemberg's books plain text link const slateData = text.trim().split('\n').map((line)=>{ return { text: line, marks: [] } }) // slateData is what I've added to the code sandbox. ``` https://codesandbox.io/s/slate-reproductions-forked-4j5r9 As you can see in the gif below it takes a few seconds to delete some text. If I recall correctly that did not used to be the case when I tried the same thing in earlier versions. ![slateJS issue](https://user-images.githubusercontent.com/4661975/105659631-33f99780-5e97-11eb-84f8-9044e2f1bb0c.gif)
sandbox 2 I also realized the example above was putting all the text into one paragraph so tried as separate paragraphs ```js const text = `//string containing moby dick book from gutemberg's books plain text link const slateData = text.trim().split('\n').map((line)=>{ return { children: [{ text: line, marks: [] }]} }) ``` https://codesandbox.io/s/slate-reproductions-forked-5inqx That's a bit better still a little lag ![slatejs-2](https://user-images.githubusercontent.com/4661975/105660738-b1bea280-5e99-11eb-8ca8-fa4f94a77d36.gif)

Let me know if there's anything else I can do to help figure this out.

piotrkubisa commented 3 years ago

I found possibly correlated issue that I have noticed and tried to raise a discussion about it on slate's Slack... but I failed :(

I was trying with other text (https://orgmode.org/worg/dev/org-syntax.org.html), which I find more contemporary example for an editor. When I paste it 2x or 3x into any kind of Slate editor, the lag to type Hello World is barely to withstand. I really wish to use slate but definitely many users will notice that problem and they might be very not satisfied with the editor.

The slate-react is simplified:

<Slate>
  <Editable> /* contentEditable, onMouseDown, onPaste, onKeyDown, onFocus, onClick, onDrop, onCut, onCopy etc. etc */
   <Children
       decorate={decorate} /* (entry: NodeEntry) => Range[] */
       decorations={decorations} /* Range[] */
       node={editor} /* ReactEditor */
       renderElement={renderElement} /* (props: RenderElementProps) => JSX.Element */
       renderLeaf={renderLeaf} /* (props: RenderLeafProps) => JSX.Element */
       selection={editor.selection} /* Range | null */
     >
     { [ ...nodes ].map(node => renderNode())  }}
   </Children>
  </Editable>
</Slate>

Every time any event fires the <Children /> is going to be updated, which not surprising, but the problem it causes hit a loop there https://github.com/ianstormtaylor/slate/blob/dd351c9d0aca2599f9375e707361feff370996bb/packages/slate-react/src/components/children.tsx#L39-L82

Aforementioned loop is also triggered every time you change cursor or type any character, not saying about the paste operation (AFAIK it will be called that many times how many lines you have).

It is completely tragic in the result, comparing it to the Zoho Writer (it has own editor canvas, jQuery-based, they use a absolute- positionable <span contentEditable={true}>&nbsp;</span> block as a cursor), TipTap (Vue.js), Remirror (ProseMirror in React.js) but similar to Quill.js.

There is an also attempt to implement slate in Vue (https://github.com/marsprince/slate-vue) and the result is notably better in terms of responsivity after the pasting operation - I can write some text easily, which is unlikely in slate-react. What I have noticed is that only specific editor operations causes to call re-render on all tree.

Going back to the problem. Do you have any suggestion to improve the responsivity? Virtualizing the rendering? It was discussed earlier (https://github.com/ianstormtaylor/slate/pull/2627) and likely it is not going be the eyed solution. More aggressive caching? Any ideas?

piotrkubisa commented 3 years ago

Likely related https://github.com/ianstormtaylor/slate/issues/3656 and https://github.com/ianstormtaylor/slate/issues/3507

piotrkubisa commented 3 years ago

FYI: Performance fixes found in https://github.com/ianstormtaylor/slate/pull/3832 helps a lot to relieve strain on browser. Please take a look at them.

williamstein commented 3 years ago

@piotrkubisa remarked:

It is completely tragic in the result. [...] More aggressive caching? [...] Virtualizing the rendering?

I'm also building an editor around Slate.js (because slatejs has the best API and philosophy of any WYSIWIG editor builder I've ever seen, by far!), but the issues with scalability mean that I also can't use slate-react exactly as is. Lately I've been tearing apart slate-react to better understand why some things are slow.

One interesting oddity in the code relevant to this discussion is in slate/packages/slate-react/src/components/text.tsx, which has this function:

const MemoizedText = React.memo(Text, (prev, next) => {
  return (
    next.parent === prev.parent &&
    next.isLast === prev.isLast &&
    next.renderLeaf === prev.renderLeaf &&
    next.text === prev.text
  )
})

In a situation where there are say 5000 Text sibling children nodes, if you change exactly one of them, then the comparison next.parent === prev.parent will be false for all 5000 of them. This is because all 5000 children text nodes have the same parent, and that parent changed (because the parent has the children array as an attribute, and one element of that array changed). This is why you observe:

"causes to call re-render on all tree."

It's not just that it goes through that loop you mentioned (which is mildly annoying), it's also that every single child is also definitely re-rendered because MemoizedText can never ever work in this setting.

I'm not 100% sure how to fix MemoizedText properly yet; in my personal fork I'm just commenting out next.parent === prev.parent && and seeing a very significant performance improvement. Note that in slate/packages/slate-react/src/components/element.tsx there is a very similar MemoizedElement, and it does not include the parent in its comparison function.


[...] Virtualizing the rendering?

I implemented a quick proof of concept of this today using the latest slate-react, and looking at #2627. What I wrote is I think pretty close to #2627 (though I think that was patches against a version different version of slate). It's not possible for me to do this as a plugin as far as I can tell; you really have to just rewrite bits of slate-react (which is of course a plugin so technically what I'm saying is false). Using react-windowed (tested on a bunch of big documents) works incredibly well for long documents, where editor.children.length is large. I'm mainly using slatejs to implement markdown editing and a new Jupyter notebook client, so large editor.children.length is exactly my use case, and my users often make really long documents (with 500+ top level elements). The drawback to windowing, as explained at #2627, is that using react-window means that you suddenly get a new todo list of nontrivial tasks (selection beyond the window size definitely breaks right without significant work, ctrl+f to find is of course broken, subtle issues if the selection contains something that gets removed from the DOM, etc.). I expect it will be pretty hard to rewrite slate-react to do all that, but I don't really see any choice (I would love to be wrong!).

None of this code I wrote is stable or usable, but I just thought somebody might find this little report on what I'm finding of interest. I'm pushing my code to the branch associated to this PR (see src/smc-webapp/frame-editors/markdown-editor/slate/slate-react/ in there) in case anybody wants to look at code.

styxlab commented 3 years ago

Experiencing exactly the same issue. When you delete the text in the large document example it takes ~ 5 seconds in my browser (Firefox). Pasting the same text in an empty document takes ~ 7 seconds. I compared with other editor solutions and both operations feel instantaneous < 500ms.

Are we confident that the mentioned issue #3656, #3507, #3832 or the findings by @williamstein identified the underlying issue? Is someone of the slate team measuring performance of the editor components, so bottlenecks can be easily discovered?

This is an awesome project and I would really like to continue using slate. I already put in quite some effort in customizing my own editor version, but this magnitude of slowness needs to be identified and fixed, because end users simply don't tolerate it.

williamstein commented 3 years ago

Are we confident that the mentioned issue #3656, #3507, #3832 or the findings by @williamstein identified the underlying issue?

I disabled the react-windowing in my branch (but kept the Text caching bug fix), and it takes about a half second to cut and less than one second to paste that large test document on Firefox. I tested on an M1 Macbook Pro with the Firefox profiler, so I could clearly see when the cpu was being used. Running the same tests at slatejs was significantly slower. So there is some hope that the one little change I suggest to text.ts caching does significantly help. I'm not using #3832 as part of this test (I'm personally "not convinced" by that PR, which just means I don't really understand it).

Noobulater commented 2 years ago

Any progress on this?

jackow98 commented 2 years ago

Hello,

I have also had some issues with large resources and re-rendering. I have tried memoising within each of the components rendered in the editor. I am considering memoising within the renderElement() function but unsure how to do this and looking for guidance

Solutions attempted so far

https://user-images.githubusercontent.com/33574605/152259071-5952da17-f1bb-479d-997b-25e0805db7bf.mov

[profiling-data.03-02-2022.00-00-29.json](https://s3-us-west-2.amazonaws.com/secure.notion-static.com/db733166-9b63-47d4-9793-cdd37684289f/profiling-data.03-02-2022.00-00-29.json)

Any advice on how to fix the error or if you have some time for a call, that would be much appreciated!