From 1452b87a51d83010be74d65489a9bdd969ef3d86 Mon Sep 17 00:00:00 2001 From: Frank Thompson Date: Mon, 11 Nov 2019 14:37:31 -0800 Subject: [PATCH] fix decorator handling in editOnBeforeInput Summary: This fixes a pretty widespread bug with decorators introduced by D7941738 and D7977136. ## The problem As far as I can tell, the bug happens in any editor containing a decorator that auto-linkifies content. Here are some repro steps using https://our.intern.facebook.com/intern/editor Diffs: - Type "D12". No link. - Update to "D123". Link appears. - Update to "D1234". Link disappears. - Delete "4". Still no link. - Try to delete "3". Crash. (Typing "D1234a" also causes a crash, as does typing "D1234" and then deleting the "D". Anything that causes the regex to fail does the trick.) URLs: - Type "www.facebook.c". No link. - Update to "www.facebook.co". Link appears. - Update to "www.facebook.com". Link disappears. - Delete "m". Still no link. - Try to delete "o". Crash. You can do the exact same thing in other surfaces as well, including comments on Tasks (although here instead of crashing, the editor just freezes until you click out of and into it again) and the Intern Q&A form. I think this issue has also been reported several times on Github: - https://github.com/draft-js-plugins/draft-js-plugins/issues/1328 - https://github.com/facebook/draft-js/issues/2133 - https://github.com/facebook/draft-js/issues/2143 ## Why it happens I don't know enough about DraftJS to know exactly what was causing the problem, but here's what I observed in the DOM when playing around with this: - If your decorator uses a simple `span` or `div` (like in the test plan of D7941738), then native events can update the contents just fine. - However, if your decorator uses anything more complicated (like an `a`), then allowing the native event to go through actually removes the `a` from the DOM. If React later attempts to unmount it, it causes an error. ## The fix The fix is pretty simple. If some text was already decorated and its length changed, always prevent native insertion. This means the decorator React component will always properly re-render. ## Side-effects I assume this could have minor perf implications for simple decorators like the hashtag decorator on the FB composer, because it will prevent native handling for some cases where it wasn't actually causing problems. I'm not sure whether this matters or not, but I don't think it should be worse than what it was before D7941738. Reviewed By: claudiopro Differential Revision: D18259308 fbshipit-source-id: f81b23440f7d9467396f3108a4d940b1ec98ca68 --- .../edit/__tests__/editOnBeforeInput-test.js | 8 ++++--- .../handlers/edit/editOnBeforeInput.js | 23 ++++++++++++------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/component/handlers/edit/__tests__/editOnBeforeInput-test.js b/src/component/handlers/edit/__tests__/editOnBeforeInput-test.js index 960bd68da6..5180991275 100644 --- a/src/component/handlers/edit/__tests__/editOnBeforeInput-test.js +++ b/src/component/handlers/edit/__tests__/editOnBeforeInput-test.js @@ -218,12 +218,14 @@ test('decorator fingerprint logic bails out of native insertion', () => { testDecoratorFingerprint('hi #', 4, 'f', true); testDecoratorFingerprint('x #foo', 3, '#', true); testDecoratorFingerprint('#foobar', 4, ' ', true); + testDecoratorFingerprint('#foo', 4, 'b', true); + testDecoratorFingerprint('#foo bar #baz', 2, 'o', true); + testDecoratorFingerprint('#foo bar #baz', 12, 'a', true); // but these are OK to let through - testDecoratorFingerprint('#foo', 4, 'b', false); - testDecoratorFingerprint('#foo bar #baz', 2, 'o', false); testDecoratorFingerprint('#foo bar #baz', 7, 'o', false); - testDecoratorFingerprint('#foo bar #baz', 12, 'a', false); + testDecoratorFingerprint('start #foo bar #baz end', 5, 'a', false); + testDecoratorFingerprint('start #foo bar #baz end', 20, 'a', false); } finally { global.getSelection = oldGetSelection; } diff --git a/src/component/handlers/edit/editOnBeforeInput.js b/src/component/handlers/edit/editOnBeforeInput.js index e79b4d5281..f0be9ad894 100644 --- a/src/component/handlers/edit/editOnBeforeInput.js +++ b/src/component/handlers/edit/editOnBeforeInput.js @@ -199,13 +199,15 @@ function editOnBeforeInput( // // 5. '[#foo]' and append 'b' // desired rendering: '[#foob]' - // native rendering would be: '[#foob]' (native insertion is OK here) + // native rendering would be: '[#foob]' + // (native insertion here would be ok for decorators like simple spans, + // but not more complex decorators. To be safe, we need to prevent it.) // // It is safe to allow native insertion if and only if the full list of - // decorator ranges matches what we expect native insertion to give. We - // don't need to compare the content because the only possible mutation - // to consider here is inserting plain text and decorators can't affect - // text content. + // decorator ranges matches what we expect native insertion to give, and + // the range lengths have not changed. We don't need to compare the content + // because the only possible mutation to consider here is inserting plain + // text and decorators can't affect text content. const oldBlockTree = editorState.getBlockTree(anchorKey); const newBlockTree = newEditorState.getBlockTree(anchorKey); mustPreventNative = @@ -218,14 +220,19 @@ function editOnBeforeInput( const oldEnd = oldLeafSet.get('end'); const adjustedEnd = oldEnd + (oldEnd >= selectionStart ? chars.length : 0); + const newStart = newLeafSet.get('start'); + const newEnd = newLeafSet.get('end'); + const newDecoratorKey = newLeafSet.get('decoratorKey'); return ( // Different decorators - oldLeafSet.get('decoratorKey') !== newLeafSet.get('decoratorKey') || + oldLeafSet.get('decoratorKey') !== newDecoratorKey || // Different number of inline styles oldLeafSet.get('leaves').size !== newLeafSet.get('leaves').size || // Different effective decorator position - adjustedStart !== newLeafSet.get('start') || - adjustedEnd !== newLeafSet.get('end') + adjustedStart !== newStart || + adjustedEnd !== newEnd || + // Decorator already existed and its length changed + (newDecoratorKey != null && newEnd - newStart !== oldEnd - oldStart) ); }); }