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

Commit

Permalink
fix decorator handling in editOnBeforeInput
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Frank Thompson authored and facebook-github-bot committed Nov 11, 2019
1 parent 0601090 commit 1452b87
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
23 changes: 15 additions & 8 deletions src/component/handlers/edit/editOnBeforeInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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)
);
});
}
Expand Down

0 comments on commit 1452b87

Please sign in to comment.