From fa88ee18cf9f0fd17bb29c47cc009a4cdf8c8c46 Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Wed, 9 May 2018 19:22:53 -0700 Subject: [PATCH] Fix block tree before/after comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: In some cases, we need to prevent native insertion because native insertion would leave the decorators in the wrong place (and critically – editOnInput's attempt to patch things up would just make things worse and process the insertion twice). This new logic should be correct. Previously, typing '#' before 'f' in 'x #foo' would result in 'x ###foo' (extra '#' inserted). Reviewed By: mitermayer Differential Revision: D7941738 fbshipit-source-id: eeff55ff082fc80c19f76fe1ea915bcbf0e7d86e --- .../edit/__tests__/editOnBeforeInput.test.js | 78 ++++++++++++++++++- .../handlers/edit/editOnBeforeInput.js | 78 ++++++++++++++++--- 2 files changed, 143 insertions(+), 13 deletions(-) diff --git a/src/component/handlers/edit/__tests__/editOnBeforeInput.test.js b/src/component/handlers/edit/__tests__/editOnBeforeInput.test.js index c80813b8e1..cc711c9ac8 100644 --- a/src/component/handlers/edit/__tests__/editOnBeforeInput.test.js +++ b/src/component/handlers/edit/__tests__/editOnBeforeInput.test.js @@ -14,11 +14,14 @@ 'use strict'; jest.disableAutomock(); +jest.mock('gkx', () => name => name === 'draft_improved_decorator_fingerprint'); +const CompositeDraftDecorator = require('CompositeDraftDecorator'); const ContentBlock = require('ContentBlock'); const ContentState = require('ContentState'); const EditorState = require('EditorState'); const SelectionState = require('SelectionState'); + const onBeforeInput = require('editOnBeforeInput'); const DEFAULT_SELECTION = { @@ -40,12 +43,12 @@ const rangedSelectionBackwards = new SelectionState({ isBackward: true, }); -const getEditorState = () => { +const getEditorState = (text: string = 'Arsenal') => { return EditorState.createWithContent( ContentState.createFromBlockArray([ new ContentBlock({ key: 'a', - text: 'Arsenal', + text, }), ]), ); @@ -53,7 +56,7 @@ const getEditorState = () => { const getInputEvent = data => ({ data, - preventDefault: () => {}, + preventDefault: jest.fn(), }); test('editor is not updated if no character data is provided', () => { @@ -151,3 +154,72 @@ test('editor selectionstate is updated if new text matches current selection and const newEditorState = editor.update.mock.calls[0][0]; expect(newEditorState.getSelection()).toMatchSnapshot(); }); + +const HASHTAG_REGEX = /#[a-z]+/g; +function hashtagStrategy(contentBlock, callback, contentState) { + findWithRegex(HASHTAG_REGEX, contentBlock, callback); +} + +function findWithRegex(regex, contentBlock, callback) { + const text = contentBlock.getText(); + let matchArr, start; + while ((matchArr = regex.exec(text)) !== null) { + start = matchArr.index; + callback(start, start + matchArr[0].length); + } +} + +function testDecoratorFingerprint( + text, + selection, + charToInsert, + shouldPrevent, +) { + const editorState = EditorState.acceptSelection( + EditorState.set(getEditorState(text), { + decorator: new CompositeDraftDecorator([ + { + strategy: hashtagStrategy, + component: null, + }, + ]), + }), + new SelectionState({ + ...DEFAULT_SELECTION, + anchorOffset: selection, + focusOffset: selection, + }), + ); + + const editor = { + _latestEditorState: editorState, + _latestCommittedEditorState: editorState, + props: {}, + update: jest.fn(), + }; + + const ev = getInputEvent(charToInsert); + onBeforeInput(editor, ev); + + expect(ev.preventDefault.mock.calls.length).toBe(shouldPrevent ? 1 : 0); +} + +test('decorator fingerprint logic bails out of native insertion', () => { + const oldGetSelection = global.getSelection; + try { + global.getSelection = () => ({}); + + // Make sure we prevent native insertion in the right cases + testDecoratorFingerprint('hi #', 4, 'f', true); + testDecoratorFingerprint('x #foo', 3, '#', true); + testDecoratorFingerprint('#foobar', 4, ' ', 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); + } finally { + global.getSelection = oldGetSelection; + } +}); diff --git a/src/component/handlers/edit/editOnBeforeInput.js b/src/component/handlers/edit/editOnBeforeInput.js index 425a60ad83..f637baac89 100644 --- a/src/component/handlers/edit/editOnBeforeInput.js +++ b/src/component/handlers/edit/editOnBeforeInput.js @@ -21,6 +21,7 @@ const EditorState = require('EditorState'); const UserAgent = require('UserAgent'); const getEntityKeyForSelection = require('getEntityKeyForSelection'); +const gkx = require('gkx'); const isEventHandled = require('isEventHandled'); const isSelectionAtLeafStart = require('isSelectionAtLeafStart'); const nullthrows = require('nullthrows'); @@ -187,16 +188,73 @@ function editOnBeforeInput( } } if (!mustPreventNative) { - // Check the old and new "fingerprints" of the current block to determine - // whether this insertion requires any addition or removal of text nodes, - // in which case we would prevent the native character insertion. - const originalFingerprint = BlockTree.getFingerprint( - editorState.getBlockTree(anchorKey), - ); - const newFingerprint = BlockTree.getFingerprint( - newEditorState.getBlockTree(anchorKey), - ); - mustPreventNative = originalFingerprint !== newFingerprint; + if (gkx('draft_improved_decorator_fingerprint')) { + // Let's say we have a decorator that highlights hashtags. In many cases + // we need to prevent native behavior and rerender ourselves -- + // particularly, any case *except* where the inserted characters end up + // anywhere except exactly where you put them. + // + // Using [] to denote a decorated leaf, some examples: + // + // 1. 'hi #' and append 'f' + // desired rendering: 'hi [#f]' + // native rendering would be: 'hi #f' (incorrect) + // + // 2. 'x [#foo]' and insert '#' before 'f' + // desired rendering: 'x #[#foo]' + // native rendering would be: 'x [##foo]' (incorrect) + // + // 3. '[#foobar]' and insert ' ' between 'foo' and 'bar' + // desired rendering: '[#foo] bar' + // native rendering would be: '[#foo bar]' (incorrect) + // + // 4. '[#foo]' and delete '#' [won't use this beforeinput codepath though] + // desired rendering: 'foo' + // native rendering would be: '[foo]' (incorrect) + // + // 5. '[#foo]' and append 'b' + // desired rendering: '[#foob]' + // native rendering would be: '[#foob]' (native insertion is OK here) + // + // 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. + const oldBlockTree = editorState.getBlockTree(anchorKey); + const newBlockTree = newEditorState.getBlockTree(anchorKey); + mustPreventNative = + oldBlockTree.size !== newBlockTree.size || + oldBlockTree.zip(newBlockTree).some(([oldLeafSet, newLeafSet]) => { + // selectionStart is guaranteed to be selectionEnd here + const oldStart = oldLeafSet.get('start'); + const adjustedStart = + oldStart + (oldStart >= selectionStart ? chars.length : 0); + const oldEnd = oldLeafSet.get('end'); + const adjustedEnd = + oldEnd + (oldEnd >= selectionStart ? chars.length : 0); + return ( + // Different decorators + oldLeafSet.get('decoratorKey') !== newLeafSet.get('decoratorKey') || + // 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') + ); + }); + } else { + // Check the old and new "fingerprints" of the current block to determine + // whether this insertion requires any addition or removal of text nodes, + // in which case we would prevent the native character insertion. + const originalFingerprint = BlockTree.getFingerprint( + editorState.getBlockTree(anchorKey), + ); + const newFingerprint = BlockTree.getFingerprint( + newEditorState.getBlockTree(anchorKey), + ); + mustPreventNative = originalFingerprint !== newFingerprint; + } } if (!mustPreventNative) { mustPreventNative = mustPreventDefaultForCharacter(chars);