Skip to content
This repository was archived by the owner on Feb 6, 2023. It is now read-only.

Commit 1452b87

Browse files
Frank Thompsonfacebook-github-bot
authored andcommitted
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: - draft-js-plugins/draft-js-plugins#1328 - #2133 - #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
1 parent 0601090 commit 1452b87

File tree

2 files changed

+20
-11
lines changed

2 files changed

+20
-11
lines changed

src/component/handlers/edit/__tests__/editOnBeforeInput-test.js

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -218,12 +218,14 @@ test('decorator fingerprint logic bails out of native insertion', () => {
218218
testDecoratorFingerprint('hi #', 4, 'f', true);
219219
testDecoratorFingerprint('x #foo', 3, '#', true);
220220
testDecoratorFingerprint('#foobar', 4, ' ', true);
221+
testDecoratorFingerprint('#foo', 4, 'b', true);
222+
testDecoratorFingerprint('#foo bar #baz', 2, 'o', true);
223+
testDecoratorFingerprint('#foo bar #baz', 12, 'a', true);
221224

222225
// but these are OK to let through
223-
testDecoratorFingerprint('#foo', 4, 'b', false);
224-
testDecoratorFingerprint('#foo bar #baz', 2, 'o', false);
225226
testDecoratorFingerprint('#foo bar #baz', 7, 'o', false);
226-
testDecoratorFingerprint('#foo bar #baz', 12, 'a', false);
227+
testDecoratorFingerprint('start #foo bar #baz end', 5, 'a', false);
228+
testDecoratorFingerprint('start #foo bar #baz end', 20, 'a', false);
227229
} finally {
228230
global.getSelection = oldGetSelection;
229231
}

src/component/handlers/edit/editOnBeforeInput.js

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -199,13 +199,15 @@ function editOnBeforeInput(
199199
//
200200
// 5. '[#foo]' and append 'b'
201201
// desired rendering: '[#foob]'
202-
// native rendering would be: '[#foob]' (native insertion is OK here)
202+
// native rendering would be: '[#foob]'
203+
// (native insertion here would be ok for decorators like simple spans,
204+
// but not more complex decorators. To be safe, we need to prevent it.)
203205
//
204206
// It is safe to allow native insertion if and only if the full list of
205-
// decorator ranges matches what we expect native insertion to give. We
206-
// don't need to compare the content because the only possible mutation
207-
// to consider here is inserting plain text and decorators can't affect
208-
// text content.
207+
// decorator ranges matches what we expect native insertion to give, and
208+
// the range lengths have not changed. We don't need to compare the content
209+
// because the only possible mutation to consider here is inserting plain
210+
// text and decorators can't affect text content.
209211
const oldBlockTree = editorState.getBlockTree(anchorKey);
210212
const newBlockTree = newEditorState.getBlockTree(anchorKey);
211213
mustPreventNative =
@@ -218,14 +220,19 @@ function editOnBeforeInput(
218220
const oldEnd = oldLeafSet.get('end');
219221
const adjustedEnd =
220222
oldEnd + (oldEnd >= selectionStart ? chars.length : 0);
223+
const newStart = newLeafSet.get('start');
224+
const newEnd = newLeafSet.get('end');
225+
const newDecoratorKey = newLeafSet.get('decoratorKey');
221226
return (
222227
// Different decorators
223-
oldLeafSet.get('decoratorKey') !== newLeafSet.get('decoratorKey') ||
228+
oldLeafSet.get('decoratorKey') !== newDecoratorKey ||
224229
// Different number of inline styles
225230
oldLeafSet.get('leaves').size !== newLeafSet.get('leaves').size ||
226231
// Different effective decorator position
227-
adjustedStart !== newLeafSet.get('start') ||
228-
adjustedEnd !== newLeafSet.get('end')
232+
adjustedStart !== newStart ||
233+
adjustedEnd !== newEnd ||
234+
// Decorator already existed and its length changed
235+
(newDecoratorKey != null && newEnd - newStart !== oldEnd - oldStart)
229236
);
230237
});
231238
}

0 commit comments

Comments
 (0)