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: Double-tapping the space bar doesn't insert a period #1177

Closed
rgrove opened this issue Sep 24, 2017 · 23 comments
Closed

iOS Safari: Double-tapping the space bar doesn't insert a period #1177

rgrove opened this issue Sep 24, 2017 · 23 comments

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?

Even when iOS's '"." Shortcut' keyboard setting is enabled, double-tapping the spacebar in Slate doesn't insert a period.

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

2017-09-23 20_15_21

What's the expected behavior?

When iOS's '"." Shortcut' keyboard setting is enabled, double-tapping the spacebar should insert a period followed by a space, as it does in a <textarea> or a native text field.

@doodlewind
Copy link

How about using the slate-auto-replace plugin?

@rgrove
Copy link
Contributor Author

rgrove commented Sep 24, 2017

How about using the slate-auto-replace plugin?

Thanks for the suggestion! This could be a good start for a workaround, but there's a little more to it than just a text replacement. iOS doesn't just replace any two space characters with a period and a space; it only does so if you tap the space bar twice within about a second. It also seems to apply a few other heuristics. For example, the replacement won't occur if you've recently deleted an auto-inserted period.

I think trying to emulate this is likely to lead to an uncanny valley, since it's probably not possible to faithfully replicate iOS's native behavior.

@dmitrizzle
Copy link

I've approximated this behavior with Slate AutoReplace plugin. It sorta works, but not 💯.

On the plus side you can create all kinds of custom heuristic

@ianstormtaylor
Copy link
Owner

Hey @rgrove, thanks for reporting this. I think I agree with you that trying to emulate is going to be hard.

Does the double-stop period emit any other events that we can listen for? Like input or beforeInput?

@rgrove
Copy link
Contributor Author

rgrove commented Sep 25, 2017

@ianstormtaylor It looks like the normal sequence of events on a double space tap (assuming the editor didn't interfere with any of these events) would be:

  1. onBeforeInput - data: " "
  2. onInput - data: undefined
  3. onBeforeInput - data: " "
  4. onBeforeInput - data: "."
  5. onInput - data: undefined
  6. onInput - data: undefined

At the end of this sequence, the result would be that a period and a space are inserted.

However, the core plugin's onBeforeInput handler calls preventDefault() and manually inserts text, which means the actual sequence of events seen by Slate is:

  1. onBeforeInput - data: " "
  2. onBeforeInput - data: " "

In other words, iOS never gets a chance to handle the double tap or generate a replacement. I have a feeling this may be what's behind #1176 as well.

@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 #1176). Please see #1176 for more discussion of this issue.

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

@rgrove thinking about this more, is there a way to tell that the specific beforeinput events we want to let through (in this case the second one with data: " ") are "special" in some way? However that may be.

The reason I ask is because if so, we could choose to not prevent default them, but also not insert any text. And then let our existing onInput handler kick in.

I think the most helpful thing for this issue and for #1176 would be to compile a very detailed list of the exact events that are fired (and their properties) in iOS, so we know exactly how much information we have to work with. Using something like https://danburzo.github.io/input-methods/ perhaps.

@rgrove
Copy link
Contributor Author

rgrove commented Oct 12, 2017

@ianstormtaylor

For the sake of clarity, since this discussion has bounced back and forth between this issue and #1176: my analysis in #1177 (comment) above was referring to synthetic React events. My analysis in #1176 (comment) is more recent, more conclusive, and covers the underlying native input-related events. I'll try to provide more detail here.

is there a way to tell that the specific beforeinput events we want to let through (in this case the second one with data: " ") are "special" in some way? However that may be.

Not if we're working with React's synthetic onBeforeInput event, because React's synthetic onBeforeInput event uses the native textInput event under the hood in iOS, and textInput doesn't provide any information that would allow us to distinguish between input caused by the user pressing a key and input that occurs due to autocorrect or related functionality.

But if we were talking about native iOS beforeinput or input events, then we can distinguish between user keyboard input and autocorrect input by looking at the value of the event's inputType property. The beforeinput and input events themselves don't provide enough information to know what was inserted, but textInput does.

Here's the exact sequence of native events on iOS when a double spacebar tap results in a period insertion. Empty cells indicate undefined values.

# event code inputType data
1 keydown Space
2 keypress Space
3 textInput " "
4 beforeinput insertText " "
5 input insertText " "
6 keyup Space
7 keydown Space
8 keypress Space
9 textInput "."
10 beforeinput insertReplacementText null
11 input insertReplacementText null
12 textInput " "
13 beforeinput insertText " "
14 input insertText " "
15 keyup Space

Events 1-6 correspond to the first tap on the spacebar. Events 7, 8, and 12-15 correspond to the second tap on the spacebar. Events 9-11 correspond to the autocorrect operation, which removes the space that was inserted by the first spacebar tap and replaces it with a period.

Unfortunately, while these events provide enough information for us to know that a replacement occurred and what text was inserted, there doesn't seem to be any way to know what text was actually replaced by the text that was inserted.

In other words, if the user types "florid" and then taps a suggestion to autocomplete it to "florida", we can tell that the letters "florida" were inserted, but we can't tell that the letters "florid" were deleted first. I'm not sure how we could perform the correct operation on the editor without this information.

Sidenote for anyone following along who might want to test iOS keyboard events using the iOS Simulator: the events generated by the simulator when you type on your physical keyboard don't have the same properties as events generated by tapping/clicking on the software keyboard. Always use the software keyboard when testing iOS events.

@rgrove
Copy link
Contributor Author

rgrove commented Oct 12, 2017

Additional info: the InputEvent spec says an event with inputType insertReplacementText should have a dataTransfer property containing a DataTransfer instance whose data is a "representation of the content that is in the clipboard, or in the kill buffer, to be dropped or otherwise the content that is to be added."

This sounded promising, but it turns out that, on iOS at least, this data only consists of the text being inserted, not the text being replaced. So it would eliminate the need to handle the textInput event to find out what replacement text was inserted, but still doesn't solve the problem of not knowing what text was replaced.

@ianstormtaylor
Copy link
Owner

Thank you @rgrove that is awesome!

In your summary of the React synthetic events you say that the order is ' ', ' ', '.'. But in the native events it looks like the order is instead ' ', '.', ' '. Why is that?

There's also supposedly an InputEvent.getTargetRanges() method, but I wasn't seeing that give us anything useful in the browser, although maybe it's implemented in iOS?

@rgrove
Copy link
Contributor Author

rgrove commented Oct 12, 2017

In your summary of the React synthetic events you say that the order is ' ', ' ', '.'. But in the native events it looks like the order is instead ' ', '.', ' '. Why is that?

Good question. It looks like I was mistaken in my assumption that React's synthetic onBeforeInput event always uses textInput under the hood. It looks like it sometimes also relies on native keypress events.

I re-tested, and the precise sequence of React synthetic events (with corresponding native events) for a double tap on the iOS spacebar is:

# React event Native event data
1 onBeforeInput keypress " "
2 onInput input
3 onBeforeInput keypress " "
4 onBeforeInput textInput "."
5 onInput input
6 onInput input

There's also supposedly an InputEvent.getTargetRanges() method, but I wasn't seeing that give us anything useful in the browser, although maybe it's implemented in iOS?

Oh, thanks for pointing that out! I missed that completely. This may be exactly what we need.

On an iOS beforeinput event with inputType insertReplacementText, getTargetRanges() returns an array containing a StaticRange instance that looks like it has exactly the info we'd need. It has startContainer and endContainer properties referring to the affected DOM nodes, and startOffset and endOffset properties indicating the range being replaced.

I should probably add that I'm testing on iOS 11. Not sure if earlier versions behave the same way, but I think it's probably safe to target the latest version as a best case scenario.

@ianstormtaylor
Copy link
Owner

@rgrove that's awesome! I was probably using a React synthetic event that was mapped to something else under the covers when I wasn't seeing it return useful things.

It seems like we're going to need to bind to the underlying events, and skirt around some of React's synthetic events. Since we're targeting newer browsers that React is this might be okay actually.

I'd be open to a PR that uses the underlying native events to fix this behavior.

Thanks for your help!

@rgrove
Copy link
Contributor Author

rgrove commented Oct 12, 2017

@ianstormtaylor

It seems like we're going to need to bind to the underlying events, and skirt around some of React's synthetic events. Since we're targeting newer browsers that React is this might be okay actually.

Do you envision replacing Slate's current usage of synthetic events with native events, or just augmenting the current usage to try to glean additional information from native events when possible?

Also, I could use some guidance with regard to what you said earlier in #1177 (comment):

The reason I ask is because if so, we could choose to not prevent default them, but also not insert any text. And then let our existing onInput handler kick in.

It's not clear to me, in terms of Slate's internals, how I can avoid preventing synthetic onBeforeInput events but also not allow the browser to insert any text.

As far as I've been able to tell in my experiments so far, the only way to prevent the browser from inserting text is to prevent the associated native beforeinput, textInput, or keypress event, but this also breaks autocorrect. But it sounds like you might have something in mind that I haven't considered?

@ianstormtaylor
Copy link
Owner

@rgrove I think augmenting the current usage would be a good start, unless replacing is the only way to achieve what we want. I'm open to either solution there.

As for preventing the browser from inserting text—I don't think we actually have to do this. The current onInput handling assumes that the browser has already inserted text, for example this is how the current spellcheck-handling behavior works. We could either let all of the input through, and backport the changes to the internal model. Or we could just expand the cases that let the input through, to handle these iOS cases, if there's a way to discern them. (Potentially by using the event.inputType flag.)

@ianstormtaylor
Copy link
Owner

Also, keep in mind that these beforeinput events are only supported in Chrome/Safari (and maybe Edge?) properly. So the logic we're doing needs to fallback for Firefox well.

@renchap
Copy link
Contributor

renchap commented Oct 14, 2017

Is #725 in the scope of this discussion? It is about auto-complete & Android and currently prevents any text editing on Android (pressing Space removes the previous word except if you use auto-complete).
It looks like related but I am not sure if this is the same issue, and the same fix.

@ianstormtaylor
Copy link
Owner

@renchap could very well be. If you investigate the native and react events that are thrown in that situation and write them up we could figure out if it is.

@renchap
Copy link
Contributor

renchap commented Oct 14, 2017

@rgrove By any chance, could you share the code you used to grab the events in your previous comments (if you have a JSFiddle ready for it)? I will try to see what happens on Android

@rgrove
Copy link
Contributor Author

rgrove commented Oct 14, 2017

@renchap Yep, you can use this fiddle. I'm interested to see what you find out!

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
@rgrove
Copy link
Contributor Author

rgrove commented Oct 15, 2017

@renchap If you have a chance, could you test #1232 in Android? I'm not super optimistic that it'll make any difference there since I found Chrome's desktop support for beforeinput to be lacking, but I'd be curious to hear if it changes anything.

@renchap
Copy link
Contributor

renchap commented Oct 16, 2017

I forked your Fiddle @rgrove to simplify testing on mobile devices: https://jsfiddle.net/renchap/ud0tk1jo/7/
It now displays the output in a textarea directly formatted to Markdown, so it is easy to copy/paste into issues.

Here are the results, reproducing https://user-images.githubusercontent.com/42070/31381061-a1caf238-adb3-11e7-952c-9f7bc978683d.gif (from #725 (comment))

User Agent: Mozilla/5.0 (Linux; Android 7.0; LG-H870 Build/NRD90U) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.98 Mobile Safari/537.36

1 - Typing "Try it", without using auto-complete

In Slate, this results in "Try" to be deleted when space is pressed, and a space to be inserted

# event code inputType data dataTransferText targetRanges
1 keydown undefined undefined
2 beforeinput insertCompositionText T null
3 input insertCompositionText T null
4 keyup undefined undefined
5 keydown undefined undefined
6 beforeinput insertCompositionText Tr null
7 input insertCompositionText Tr null
8 keyup undefined undefined
9 keydown undefined undefined
10 beforeinput insertCompositionText Try null
11 input insertCompositionText Try null
12 keyup undefined undefined
13 keydown undefined undefined
14 beforeinput insertText null [object StaticRange]
15 textInput undefined
16 input insertText null
17 keyup undefined undefined
18 keydown undefined undefined
19 beforeinput insertCompositionText i null
20 input insertCompositionText i null
21 keyup undefined undefined
22 keydown undefined undefined
23 beforeinput insertCompositionText it null
24 input insertCompositionText it null
25 keyup undefined undefined

2 - Typing "Try<press on "try" auto-complete suggestion> it":

In Slate, this results in the intended behaviour (editor content is "Try it").

# event code inputType data dataTransferText targetRanges
1 keydown undefined undefined
2 beforeinput insertCompositionText T null
3 input insertCompositionText T null
4 keyup undefined undefined
5 keydown undefined undefined
6 beforeinput insertCompositionText Tr null
7 input insertCompositionText Tr null
8 keyup undefined undefined
9 keydown undefined undefined
10 beforeinput insertCompositionText Try null
11 input insertCompositionText Try null
12 keyup undefined undefined
13 keydown undefined undefined
14 beforeinput insertCompositionText Try null
15 textInput Try undefined
16 input insertCompositionText Try null
17 keyup undefined undefined
18 keydown undefined undefined
19 beforeinput insertText null [object StaticRange]
20 textInput undefined
21 input insertText null
22 keyup undefined undefined
23 keydown undefined undefined
24 beforeinput insertCompositionText i null
25 input insertCompositionText i null
26 keyup undefined undefined
27 keydown undefined undefined
28 beforeinput insertCompositionText it null
29 input insertCompositionText it null
30 keyup undefined undefined

@rgrove
Copy link
Contributor Author

rgrove commented Oct 16, 2017

@renchap Thanks! This is very useful.

The bad news is that based on these results, it looks like #1232 won't have any effect on Android Chrome. The good news is that it at least shouldn't make things worse.

It also looks like Android Chrome's insertCompositionText input type doesn't include information about the affected range, which could make this tricky to fix. But the fact that there are beforeinput events with some information is at least slightly promising.

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

No branches or pull requests

5 participants