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

Commit

Permalink
Fix unlucky failures in character replacement
Browse files Browse the repository at this point in the history
Summary:
Fixes #1830 (bug introduced in #719; this is a proper fix for #398).

* When we're allowing native insertion, we don't want to modify the selection because doing so blocks browser features like spellcheck from working
* In all other cases when typing, we *do* want to force the selection – failing to do this is why the original issue occurred
* We use the `insert-characters` change type for both native and non-native insertions
* Instead of guessing in `EditorState.push` whether to force a selection, accept it as a parameter
* Gate all changes by `draft_non_native_insertion_forces_selection`

I verified with the feature flag disabled (same behavior as before):

* Typing over a character works, and the cursor moves as expected (because of the `chars === currentlySelectedChars` check)
* With editor contents "ABC\ndef", replacing "d", "e", or "f" with the character above *fails* to replace the char (because that check is buggy).

And with the feature flag enabled (new behavior):

* Typing over a character still works (but does so without the buggy check)
* With editor contents "ABC\ndef", replacing "d", "e", or "f" with the character above *succeeds* at replacing the char.

Reviewed By: flarnie

Differential Revision: D9209691

fbshipit-source-id: f63551dbad689391aa9c2a69f1d6ba395b8bf1ac
  • Loading branch information
sophiebits authored and facebook-github-bot committed Aug 8, 2018
1 parent 2d7ad18 commit ae25b8f
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 4 deletions.
25 changes: 23 additions & 2 deletions src/component/handlers/edit/editOnBeforeInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,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');
Expand All @@ -36,6 +37,10 @@ const FF_QUICKFIND_CHAR = "'";
const FF_QUICKFIND_LINK_CHAR = '/';
const isFirefox = UserAgent.isBrowser('Firefox');

const nonNativeInsertionForcesSelection = gkx(
'draft_non_native_insertion_forces_selection',
);

function mustPreventDefaultForCharacter(character: string): boolean {
return (
isFirefox &&
Expand All @@ -52,6 +57,7 @@ function replaceText(
text: string,
inlineStyle: DraftInlineStyle,
entityKey: ?string,
forceSelection: boolean,
): EditorState {
const contentState = DraftModifier.replaceText(
editorState.getCurrentContent(),
Expand All @@ -60,7 +66,12 @@ function replaceText(
inlineStyle,
entityKey,
);
return EditorState.push(editorState, contentState, 'insert-characters');
return EditorState.push(
editorState,
contentState,
'insert-characters',
forceSelection,
);
}

/**
Expand Down Expand Up @@ -124,7 +135,10 @@ function editOnBeforeInput(
.getCurrentContent()
.getPlainText()
.slice(selectionStart, selectionEnd);
if (chars === currentlySelectedChars) {
if (
!nonNativeInsertionForcesSelection &&
chars === currentlySelectedChars
) {
editor.update(
EditorState.forceSelection(
editorState,
Expand All @@ -144,6 +158,7 @@ function editOnBeforeInput(
editorState.getCurrentContent(),
editorState.getSelection(),
),
true,
),
);
}
Expand All @@ -158,6 +173,7 @@ function editOnBeforeInput(
editorState.getCurrentContent(),
editorState.getSelection(),
),
false,
);

// Bunch of different cases follow where we need to prevent native insertion.
Expand Down Expand Up @@ -254,6 +270,11 @@ function editOnBeforeInput(

if (mustPreventNative) {
e.preventDefault();
if (nonNativeInsertionForcesSelection) {
newEditorState = EditorState.set(newEditorState, {
forceSelection: true,
});
}
editor.update(newEditorState);
return;
}
Expand Down
9 changes: 7 additions & 2 deletions src/model/immutable/EditorState.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@ import type {List, OrderedMap} from 'immutable';
const BlockTree = require('BlockTree');
const ContentState = require('ContentState');
const EditorBidiService = require('EditorBidiService');
const Immutable = require('immutable');
const SelectionState = require('SelectionState');

const gkx = require('gkx');
const Immutable = require('immutable');

const {OrderedSet, Record, Stack} = Immutable;

type EditorStateRecordType = {
Expand Down Expand Up @@ -340,12 +342,15 @@ class EditorState {
editorState: EditorState,
contentState: ContentState,
changeType: EditorChangeType,
forceSelection: boolean = true,
): EditorState {
if (editorState.getCurrentContent() === contentState) {
return editorState;
}

const forceSelection = changeType !== 'insert-characters';
if (!gkx('draft_non_native_insertion_forces_selection')) {
forceSelection = changeType !== 'insert-characters';
}
const directionMap = EditorBidiService.getDirectionMap(
contentState,
editorState.getDirectionMap(),
Expand Down

0 comments on commit ae25b8f

Please sign in to comment.