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

Fix modifying state during onChange triggered by beforeinput #667

Merged
merged 1 commit into from
Dec 6, 2016

Conversation

sophiebits
Copy link
Contributor

Fixes #92.

If an onChange handler replaces every "x" with an "abc", then when you type "x", we trigger onChange from beforeinput and rerender immediately with the content "abc", then the browser still inserts an "x" character!

This fix is surprisingly simple: instead of triggering an update during beforeinput, we simply wait until the input event to do so. This means that when we rerender, the browser would have already inserted the "x" character and React will correctly delete it when rerendering.

Test Plan: In plaintext example, change the handler to

this.onChange = (editorState) => {
  var content = editorState.getCurrentContent();
  return this.setState({
    editorState: EditorState.set(editorState, {
      currentContent: Modifier.removeInlineStyle(content, content.getSelectionAfter(), 'OVERFLOW');
    }),
  });
};

then type. Characters don't get duplicated like they did before.

@sophiebits
Copy link
Contributor Author

cc @hellendag – This fix felt too obvious to me. Am I missing some reason why it can't work?

@sophiebits
Copy link
Contributor Author

This doesn't work; typing #a in the tweet example only inserts a #. Need to investigate.

@sophiebits
Copy link
Contributor Author

Fixed – I had a silly mistake. :)

@stopachka I choose you! Think this makes sense? And @hellendag too if you have time.

@hellendag
Copy link

Thanks! This seems reasonable to me!

I may not have considered this because I regard the post-mutation nature of the input event to be generally useless for Draft purposes (except for things like spellcheck, as used here), but this seems like a pretty clever way to use it.

This is enough of a core input change that it's worth staying on your toes to make sure it doesn't lead to any mysterious editing issues.

🦄

@paulyoung
Copy link
Contributor

If there's anything I can do to help move this along, please let me know.

@davidgeilfus
Copy link

Hi, any idea when this will be merged? Thank you

Fixes facebookarchive#92.

If an onChange handler replaces every "x" with an "abc", then when you type "x", we trigger onChange from beforeinput and rerender immediately with the content "abc", then the browser still inserts an "x" character!

This fix is surprisingly simple: instead of triggering an update during beforeinput, we simply wait until the input event to do so. This means that when we rerender, the browser would have already inserted the "x" character and React will correctly delete it when rerendering.

Test Plan: In plaintext example, change the handler to

```
this.onChange = (editorState) => {
  var content = editorState.getCurrentContent();
  return this.setState({
    editorState: EditorState.set(editorState, {
      currentContent: Modifier.removeInlineStyle(content, content.getSelectionAfter(), 'OVERFLOW');
    }),
  });
};
```

then type. Characters don't get duplicated like they did before.
@sophiebits sophiebits merged commit 07892ba into facebookarchive:master Dec 6, 2016
acusti pushed a commit to brandcast/draft-js that referenced this pull request Dec 14, 2016
…karchive#667)

Fixes facebookarchive#92.

If an onChange handler replaces every "x" with an "abc", then when you type "x", we trigger onChange from beforeinput and rerender immediately with the content "abc", then the browser still inserts an "x" character!

This fix is surprisingly simple: instead of triggering an update during beforeinput, we simply wait until the input event to do so. This means that when we rerender, the browser would have already inserted the "x" character and React will correctly delete it when rerendering.

Test Plan: In plaintext example, change the handler to

```
this.onChange = (editorState) => {
  var content = editorState.getCurrentContent();
  return this.setState({
    editorState: EditorState.set(editorState, {
      currentContent: Modifier.removeInlineStyle(content, content.getSelectionAfter(), 'OVERFLOW');
    }),
  });
};
```

then type. Characters don't get duplicated like they did before.
flarnie added a commit to flarnie/draft-js that referenced this pull request Jan 9, 2017
…archive#667"

This fixes some issues with the fix originally merged from PR facebookarchive#667[2]

Credit to @balpert and @srmcconomy for this fix. For now I'm getting
them back in sync.

Also thanks to @davidchang for pinging me about landing this,
@colinjeane for reporting this bug in facebookarchive#917 and submitting a nearly
identical fix: facebookarchive#919

@colinjeane since we already had this fixed internally I'm going to use
our approach to keep things in sync.

fixes facebookarchive#917, facebookarchive#849

[2]: facebookarchive#667

We also had to tweak this compared to the current internal version to
accomodate a recent change that makes 'editOnBeforeInput' and the other
handlers take 'editor' as an explicit argument instead of assumming
'this' will be the editor;
facebookarchive@e64c2c3

Note that we are updating the version of 'fbjs' required in order to get
access to the 'setImmediate' polyfill.
flarnie added a commit that referenced this pull request Jan 12, 2017
)

This fixes some issues with the fix originally merged from PR #667[2]

Credit to @balpert and @srmcconomy for this fix. For now I'm getting
them back in sync.

Also thanks to @davidchang for pinging me about landing this,
@colinjeane for reporting this bug in #917 and submitting a nearly
identical fix: #919

@colinjeane since we already had this fixed internally I'm going to use
our approach to keep things in sync.

fixes #917, #849

[2]: #667

We also had to tweak this compared to the current internal version to
accomodate a recent change that makes 'editOnBeforeInput' and the other
handlers take 'editor' as an explicit argument instead of assumming
'this' will be the editor;
e64c2c3

Note that we are updating the version of 'fbjs' required in order to get
access to the 'setImmediate' polyfill.
max-winderbaum pushed a commit to textioHQ/draft-js that referenced this pull request Jan 23, 2017
…archive#667" (facebookarchive#871)

This fixes some issues with the fix originally merged from PR facebookarchive#667[2]

Credit to @balpert and @srmcconomy for this fix. For now I'm getting
them back in sync.

Also thanks to @davidchang for pinging me about landing this,
@colinjeane for reporting this bug in facebookarchive#917 and submitting a nearly
identical fix: facebookarchive#919

@colinjeane since we already had this fixed internally I'm going to use
our approach to keep things in sync.

fixes facebookarchive#917, facebookarchive#849

[2]: facebookarchive#667

We also had to tweak this compared to the current internal version to
accomodate a recent change that makes 'editOnBeforeInput' and the other
handlers take 'editor' as an explicit argument instead of assumming
'this' will be the editor;
facebookarchive@e64c2c3

Note that we are updating the version of 'fbjs' required in order to get
access to the 'setImmediate' polyfill.
flarnie added a commit that referenced this pull request Jan 27, 2017
)

This fixes some issues with the fix originally merged from PR #667[2]

Credit to @balpert and @srmcconomy for this fix. For now I'm getting
them back in sync.

Also thanks to @davidchang for pinging me about landing this,
@colinjeane for reporting this bug in #917 and submitting a nearly
identical fix: #919

@colinjeane since we already had this fixed internally I'm going to use
our approach to keep things in sync.

fixes #917, #849

[2]: #667

We also had to tweak this compared to the current internal version to
accomodate a recent change that makes 'editOnBeforeInput' and the other
handlers take 'editor' as an explicit argument instead of assumming
'this' will be the editor;
e64c2c3

Note that we are updating the version of 'fbjs' required in order to get
access to the 'setImmediate' polyfill.
intentionally-left-nil referenced this pull request in textioHQ/draft-js Feb 8, 2017
Fixes textioHQ/frontend#2259

This is an alternative method to getting around the problem of IE11 having no input event and wanting to allow native browser insertion to increase react performance
See also: facebook#667
See also: facebook#871

The premise of this is that it updates the editor state immediately, on the onBeforeInput event. And then it adds extra flags to know what to do during the render. As opposed to waiting for the input event on IE.
max-winderbaum referenced this pull request in textioHQ/draft-js Feb 28, 2017
Fixes textioHQ/frontend#2259

This is an alternative method to getting around the problem of IE11 having no input event and wanting to allow native browser insertion to increase react performance
See also: facebook#667
See also: facebook#871

The premise of this is that it updates the editor state immediately, on the onBeforeInput event. And then it adds extra flags to know what to do during the render. As opposed to waiting for the input event on IE.
ouchxp pushed a commit to ouchxp/draft-js that referenced this pull request Apr 7, 2017
…karchive#667)

Fixes facebookarchive#92.

If an onChange handler replaces every "x" with an "abc", then when you type "x", we trigger onChange from beforeinput and rerender immediately with the content "abc", then the browser still inserts an "x" character!

This fix is surprisingly simple: instead of triggering an update during beforeinput, we simply wait until the input event to do so. This means that when we rerender, the browser would have already inserted the "x" character and React will correctly delete it when rerendering.

Test Plan: In plaintext example, change the handler to

```
this.onChange = (editorState) => {
  var content = editorState.getCurrentContent();
  return this.setState({
    editorState: EditorState.set(editorState, {
      currentContent: Modifier.removeInlineStyle(content, content.getSelectionAfter(), 'OVERFLOW');
    }),
  });
};
```

then type. Characters don't get duplicated like they did before.
ouchxp pushed a commit to ouchxp/draft-js that referenced this pull request Apr 7, 2017
…archive#667" (facebookarchive#871)

This fixes some issues with the fix originally merged from PR facebookarchive#667[2]

Credit to @balpert and @srmcconomy for this fix. For now I'm getting
them back in sync.

Also thanks to @davidchang for pinging me about landing this,
@colinjeane for reporting this bug in facebookarchive#917 and submitting a nearly
identical fix: facebookarchive#919

@colinjeane since we already had this fixed internally I'm going to use
our approach to keep things in sync.

fixes facebookarchive#917, facebookarchive#849

[2]: facebookarchive#667

We also had to tweak this compared to the current internal version to
accomodate a recent change that makes 'editOnBeforeInput' and the other
handlers take 'editor' as an explicit argument instead of assumming
'this' will be the editor;
facebookarchive@e64c2c3

Note that we are updating the version of 'fbjs' required in order to get
access to the 'setImmediate' polyfill.
kgdev added a commit to kgdev/slate that referenced this pull request Aug 2, 2017
midas19910709 added a commit to midas19910709/draft-js that referenced this pull request Mar 30, 2022
…871)

This fixes some issues with the fix originally merged from PR #667[2]

Credit to @balpert and @srmcconomy for this fix. For now I'm getting
them back in sync.

Also thanks to @davidchang for pinging me about landing this,
@colinjeane for reporting this bug in #917 and submitting a nearly
identical fix: facebookarchive/draft-js#919

@colinjeane since we already had this fixed internally I'm going to use
our approach to keep things in sync.

fixes #917, #849

[2]: facebookarchive/draft-js#667

We also had to tweak this compared to the current internal version to
accomodate a recent change that makes 'editOnBeforeInput' and the other
handlers take 'editor' as an explicit argument instead of assumming
'this' will be the editor;
facebookarchive/draft-js@e64c2c3

Note that we are updating the version of 'fbjs' required in order to get
access to the 'setImmediate' polyfill.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typed char gets duplicated if also changing block type in onChange
5 participants