draft-js-utils icon indicating copy to clipboard operation
draft-js-utils copied to clipboard

Fix weird performance issue

Open sbstjn opened this issue 6 years ago • 5 comments

Hi, Thanks for the great react-rte! We noticed a few performance issues with the underlying draft-js-import-html and draft-js-import-element libraries. I tried to fix it for "our use case" and added tests that make sure large/weird HTML structures can be parsed.

Existing tests still work fine 🎉

//cc @HenrikFricke

sbstjn avatar May 30 '18 15:05 sbstjn

This breaks other untested code/cases ;(

sbstjn avatar Jun 01 '18 13:06 sbstjn

@sbstjn is this ready to merge?

sibelius avatar Jul 27 '19 02:07 sibelius

This fix breaks parsing.

For this input:

<p style="">
    <span>Line one.</span>
</p>
<p style="">
    <span>Line two </span><a href="/foo/bar"><span>link</span></a><span>.</span>
</p>
<p style="">
    <span>Line three </span><a href="https://foo.bar"><span>link one</span></a><span> and </span><a href="https://foo2.bar"><span>link two</span></a><span>.</span>
</p>

Notice the text " and " between the links in the third paragraph.

characterMeta is incorrectly built for them and the output marks " and " as part of the second "link" entity which in turn results in DraftJS rendering it as <a href...>link</a> after which it completely omits the remaining five characters.

The bug this is trying to fix is in the processText method: var seq = Repeat(charMetadata, text.length); should actually be var seq = Repeat(charMetadata, 1);. I can't figure out why a Repeat was used here in the first place, but repeating the same sequence of characters the same times as it's length is for sure not correct. Replacing that with var seq = Repeat(charMetadata, 1); fixes the original bug and the fix here (if (!characterMeta.isSuperset(textFragment.characterMeta)) {) is not required.

sergiu-paraschiv avatar Sep 13 '22 10:09 sergiu-paraschiv

Later: my fix does not work.

sergiu-paraschiv avatar Sep 13 '22 12:09 sergiu-paraschiv

But this patch does:

diff --git a/node_modules/draft-js-import-element/esm/stateFromElement.js b/node_modules/draft-js-import-element/esm/stateFromElement.js
index eccabfd..d14b574 100644
--- a/node_modules/draft-js-import-element/esm/stateFromElement.js
+++ b/node_modules/draft-js-import-element/esm/stateFromElement.js
@@ -513,8 +513,9 @@ function collapseWhiteSpace(text, characterMeta) {
 
   text = _trimTrailingSpace.text;
   characterMeta = _trimTrailingSpace.characterMeta;
-  var i = text.length;
 
+  characterMeta = characterMeta.toArray();
+  var i = text.length;
   while (i--) {
     if (text.charAt(i) === ' ' && text.charAt(i - 1) === ' ') {
       text = text.slice(0, i) + text.slice(i + 1);
@@ -522,6 +523,8 @@ function collapseWhiteSpace(text, characterMeta) {
     }
   } // There could still be one space on either side of a softbreak.
 
+  characterMeta = Seq(characterMeta);
+
 
   var _replaceTextWithMeta = replaceTextWithMeta({
     text: text,
@@ -561,14 +564,16 @@ function canHaveDepth(blockType) {
 
 function concatFragments(fragments) {
   var text = '';
-  var characterMeta = Seq();
-  fragments.forEach(function (textFragment) {
-    text = text + textFragment.text;
-    characterMeta = characterMeta.concat(textFragment.characterMeta);
-  });
+
+  var characterMeta = [];
+  for (var i = 0; i < fragments.length; i++) {
+      text = text + fragments[i].text;
+      characterMeta.push(...fragments[i].characterMeta.toArray());
+  }
+
   return {
     text: text,
-    characterMeta: characterMeta
+    characterMeta: Seq(characterMeta)
   };
 }
 

sergiu-paraschiv avatar Sep 13 '22 13:09 sergiu-paraschiv