Fixed autocorrect causing dupe text #655
Conversation
…ake it more reusable.
During autocorrection (in iOS’s Safari), `onSelect` event is triggered after `onBeforeInput` event. The plugins/core updates the state during `onBeforeInput` event, thus causing selection triggered by autocorrect to be lost/overridden. This behaviour caused dupe text bug during autocorrection. To overcome this issue, we try to query the selection and conditionally fix out of sync cases with an additional transform before inserting the text.
There was a problem hiding this comment.
@lxcid thank you for this! I'm sorry for the delay. This is a seriously amazing pull request, with absolutely great research. Thank you for putting in the time!
I left a few comments inline, nothing big just little tweaks for our future selves.
I agree that it's unfortunate that we need to add more processing on every input, but it seems necessary.
Question for you: if we were to only trigger this extra processing when the native selection was expanded, would that have any negative effects? It seems like that could get around having to trigger it for 99% of the cases where people are just typing individual characters. And might be good for performance.
src/components/content.js
Outdated
| @@ -148,24 +148,7 @@ class Content extends React.Component { | |||
|
|
|||
| getPoint(element, offset) { | |||
There was a problem hiding this comment.
Can we remove this #getPoint method entirely now, and just use the new utility function that you created inline in each of the places?
src/plugins/core.js
Outdated
| @@ -97,9 +98,35 @@ function Plugin(options = {}) { | |||
|
|
|||
| const chars = initialChars.insert(startOffset, char) | |||
|
|
|||
| let nextTransform = state.transform() | |||
There was a problem hiding this comment.
Can you change this variable to just be called transform instead of nextTransform?
src/plugins/core.js
Outdated
| let nextTransform = state.transform() | ||
|
|
||
| // Determine if selection is out of sync, which can happen during autocorrect, | ||
| // and update accordingly. |
There was a problem hiding this comment.
Can you change this comment to:
// COMPAT: In iOS, when choosing from the predictive text suggestions, the
// native selection will be changed to span the existing word, so that the word
// is replaced. But the `select` event for this change doesn't fire until after
// the `beforeInput` event, even though the native selection is updated. So we
// need to manually adjust the selection to be in sync. (03/18/2017)|
Hey, thanks for reviewing! I've made the necessary changes!
I think that's a great improvement. It do sounds like a reasonable hypothesis, I couldn't think of a scenario where autocorrect would perform without expanding the selection. So we just had to check for I think that is all of it! 😃 |
|
Unfortunately, f4aa680 caused Android to regress and the autocorrect to work intermittently. 😞 Without f4aa680, Android is usable (autocorrect works), although it has it quirks (Some repeats may happen after the cursor at times, which will disappears as you continue typing at times as well, weird). I decides to remove f4aa680 from this PR. |
|
Ah damn, alright well we'll just have to keep an eye in the performance and if it becomes a problem we can figure something out! Thanks @lxcid! |
* Refactored out `getPoint` from components/content to utils so as to make it more reusable. * Fixed autocorrect causing dupe text. (ianstormtaylor#540) During autocorrection (in iOS’s Safari), `onSelect` event is triggered after `onBeforeInput` event. The plugins/core updates the state during `onBeforeInput` event, thus causing selection triggered by autocorrect to be lost/overridden. This behaviour caused dupe text bug during autocorrection. To overcome this issue, we try to query the selection and conditionally fix out of sync cases with an additional transform before inserting the text. * Removes Content#getPoint and use the new utility function instead. * Renames local variable nextTransform to transform. * Describe the solution to the autocorrect issue in a more descriptive manner.
For #540
Here's an explanation of what the following codes try to achieve.
The con of this fix is that it adds an additional query to native selection per text input.
I choose this solution because it is less destructive to existing execution flow.
Feel free to reject or rewrite if this isn't the right way to fix this.