Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS Safari: Predictive text suggestions don't seem to recognize word breaks #1176

Closed
rgrove opened this issue Sep 24, 2017 · 7 comments · Fixed by #1365
Closed

iOS Safari: Predictive text suggestions don't seem to recognize word breaks #1176

rgrove opened this issue Sep 24, 2017 · 7 comments · Fixed by #1365

Comments

@rgrove
Copy link
Contributor

rgrove commented Sep 24, 2017

Do you want to request a feature or report a bug?

bug

What's the current behavior?

In Safari on iOS 11, every character typed into the editor becomes part of the predictive text suggestions, as if there are no spaces between the typed words.

For example, when I type the phrase "kittens are cute and fuzzy", Safari treats this as a single giant word, "kittensarecuteandfuzzy", when suggesting replacements. This effectively breaks autocorrection.

I spent some time trying to figure out what could be happening here to see if I might be able to send a PR, but I'm still fairly new to the Slate codebase (and to the intricacies of contenteditable in general). If anyone has any pointers, I'd be grateful.

https://jsfiddle.net/rgrove/v0mkz2w0/4/

2017-09-23 20_04_36

What's the expected behavior?

Predictive text suggestions should provide suggestions based on individual words, as is the case when typing in a <textarea> or a native text field.

@rgrove
Copy link
Contributor Author

rgrove commented Sep 30, 2017

It looks like this bug was introduced in #1088 with the removal of isNative (which also caused #1177).

It doesn't appear to be possible to support autocorrect, predictive text, auto-capitalization, or the double-space period shortcut (among other text input features) on iOS without allowing native updates. Since these features are all pretty crucial to providing a good typing experience on iOS, this seems like a strong argument for reverting the removal of isNative and continuing to allow native updates whenever possible.

@ianstormtaylor, any thoughts on this?

rgrove added a commit to rgrove/slate that referenced this issue Oct 4, 2017
Preventing native text input events completely breaks autocorrect,
autocomplete, auto-capitalization, predictive text, and double-space
period insertion on iOS. In order to allow iOS to perform these kinds of
operations, we have to avoid preventing the default event behavior
and allow native input handling whenever possible.

Fixes ianstormtaylor#1176
Fixes ianstormtaylor#1177

See also: ianstormtaylor#1088
@ianstormtaylor
Copy link
Owner

Hey @rgrove thanks for investigating this and for the #1205 PR.

It sounds like to maintain some of the more complex features support we'll definitely need to be aware of the native behavior, and use that to dictate our reaction. What I'd like to know is if there's a way to do it such that we always ignore what the native behavior renders, and instead convert everything to a series of change methods. That way we'd get the benefits of using the native behaviors, while maintaining the benefits of always having a single, simple-to-reason-about render code path.

Do you know if this is possible? If anyone else has thoughts that would be awesome!

@rgrove
Copy link
Contributor Author

rgrove commented Oct 5, 2017

What I'd like to know is if there's a way to do it such that we always ignore what the native behavior renders, and instead convert everything to a series of change methods.

I agree, this seems ideal. I'm not sure it's possible though.

Calling preventDefault() on React's synthetic onBeforeInput event (which is actually a native textInput event under the hood in browsers that support textInput) will always prevent iOS's autocorrect-related behaviors from being triggered, so we'd have to find a way to avoid calling preventDefault() but somehow still not allow the native input to be rendered.

In terms of native (not synthetic) events, iOS fires input-related events in this order when a character is typed:

  1. textInput
  2. beforeinput
  3. input

In iOS, canceling either the textInput event or the beforeinput event will prevent the text from being natively inserted or rendered, but will also prevent autocorrect from working. In Chrome, beforeinput is not cancelable (which appears to violate the spec). Firefox doesn't fire textInput or beforeinput.

As far as I can tell in my testing so far, if we want autocorrect to work, it looks like we have to let the native input event happen, and by that point the text has already been inserted and natively rendered.

I've been using this JSFiddle as an event testbed if you'd like to play around with these events yourself.

@ianstormtaylor
Copy link
Owner

Could we let the native event happen, but then "diff" (however) the resulting text, convert that diff into a change, and then when re-rendering reset the state to match Slate's internal representation?

@rgrove
Copy link
Contributor Author

rgrove commented Oct 5, 2017

Could we let the native event happen, but then "diff" (however) the resulting text, convert that diff into a change, and then when re-rendering reset the state to match Slate's internal representation?

I'm not sure I know enough about Slate's internals yet to answer this. Maybe?

It sounds to me like the end result of this is roughly what's already happening with the restored isNative code in #1205, but I don't yet fully understand Slate's change-to-render pipeline, and I'm guessing you have something different in mind.

@lxcid
Copy link
Collaborator

lxcid commented Oct 6, 2017

Just throwing ideas here.

I was wondering as (predictive text, auto correct, etc) are related to component instead of state. Should these value be stored in the component instead of the model? Is there value in knowing that isActive is true outside of the component render cycle?

@lxcid
Copy link
Collaborator

lxcid commented Oct 6, 2017

Could we let the native event happen, but then "diff" (however) the resulting text, convert that diff into a change, and then when re-rendering reset the state to match Slate's internal representation?

I like this idea. But will that add cpu load to input cycle as the text size grow?

rgrove added a commit to rgrove/slate that referenced this issue Oct 15, 2017
In browsers that support it (currently only Safari has full support),
the native `beforeinput` DOM event provides much more useful information
about text insertion than React's synthetic `onBeforeInput` event.

By handling text insertion with the native event instead of the
synthetic event when possible, we can fully support autocorrect,
spellcheck replacements, and related functionality on iOS without
resorting to hacks.

See the discussion in ianstormtaylor#1177 for more background on this change.

Fixes ianstormtaylor#1176
Fixes ianstormtaylor#1177
ianstormtaylor pushed a commit that referenced this issue Oct 16, 2017
#1232)

* Add support for finding a Slate range from a native StaticRange

* Add a `SUPPORTED_EVENTS` environment constant

This is an object mapping of DOM event names to booleans indicating
whether the browser supports that event.

* Use native `beforeinput` events to handle text insertion when possible

In browsers that support it (currently only Safari has full support),
the native `beforeinput` DOM event provides much more useful information
about text insertion than React's synthetic `onBeforeInput` event.

By handling text insertion with the native event instead of the
synthetic event when possible, we can fully support autocorrect,
spellcheck replacements, and related functionality on iOS without
resorting to hacks.

See the discussion in #1177 for more background on this change.

Fixes #1176
Fixes #1177

* Fix lint error.
rgrove added a commit to rgrove/slate that referenced this issue Nov 1, 2017
UA sniffing stops at the first match when trying to determine the
client's OS. Since the macOS sniff always ran before the iOS sniff and
matched the string "mac os x", which is also present in iOS Safari's
user agent string, `IS_MAC` was always `true` for iOS Safari and
`IS_IOS` was always `false`.

The iOS sniff now runs before the macOS sniff, which prevents false
positives. Existing uses of `IS_MAC` that should also apply to iOS have
been updated to check for `IS_MAC || IS_IOS`.

This also re-fixes ianstormtaylor#1176 and ianstormtaylor#1177, which regressed when a recent change
added `IS_IOS` checks that inadvertently prevented code from running on
iOS.
ianstormtaylor pushed a commit that referenced this issue Nov 1, 2017
UA sniffing stops at the first match when trying to determine the
client's OS. Since the macOS sniff always ran before the iOS sniff and
matched the string "mac os x", which is also present in iOS Safari's
user agent string, `IS_MAC` was always `true` for iOS Safari and
`IS_IOS` was always `false`.

The iOS sniff now runs before the macOS sniff, which prevents false
positives. Existing uses of `IS_MAC` that should also apply to iOS have
been updated to check for `IS_MAC || IS_IOS`.

This also re-fixes #1176 and #1177, which regressed when a recent change
added `IS_IOS` checks that inadvertently prevented code from running on
iOS.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants