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

Consider using beforeinput event on Android for typing and delete improvements #3131

Closed
f1ames opened this issue Jul 10, 2018 · 9 comments · Fixed by ckeditor/ckeditor5-typing#198
Labels
package:typing type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@f1ames
Copy link
Contributor

f1ames commented Jul 10, 2018

Some time ago @szymonkups did some research regarding typing on Android and beforeinput/input events - #614 (comment). At that time it doesn't seem reasonable to go with those events as they didn't bring much improvement for what we had.

Now, when we started working on typing improvements for Android - #614, we have revisited those events and the way they can help improve typing.

Since beforeinput and input events are available on Android 6+ now (tested on Android 6.x and 8.x with Chrome 67.0.3396.87 on this fiddle) we should reconsider utilising them especially that we have encountered some not so trivial cases while working on Backspace support - #3126, #3129, #1127.


One idea is to integrate Delete plugin with beforeinput/input as for content removal it always has a deleteContentBackward flag, so it could be used instead or together with ckeditor/ckeditor5-typing#165. However, it might be tricky to integrate with injectUnsafeKeystrokesHandling as keydown event is fired before beforeinput and the keydown handler modifies DOM content.

The other thing I have noticed (Android 6.0.1) is that events sequence is not always the same, could be:

keydown
keyup
beforeinput
input

or:

keydown
beforeinput
input
keyup

depending on what is removed I suppose (the fact that some events sequence is not always the same is also mentioned here).

So while it seems reasonable to recheck if those events could bring some improvements for typing on Android, we need to make sure to do proper testing.

@Reinmar
Copy link
Member

Reinmar commented Jul 10, 2018

In fact, we could also check whether we couldn't base on beforeinput on desktop (Chrome, Safari) too. It wouldn't make much sense to overwrite the entire existing implementation but some preliminary research might shed some light on what we could improve in the future.

@f1ames
Copy link
Contributor Author

f1ames commented Jul 11, 2018

After some initial testing it looks like the beforeinput event may make sense for Delete plugin. It plays quite nicely with keydown handler as:

  • DeleteObserver listens to the keydown event to detect content deletion (Backspace, Delete key press).
  • If deleting is detected, it is processed (removing content) and the keydown event propagation is stopped. This results in beforeinput event not being fired at all.
  • However on Android, due to 229 key code for keydown event, the above flow is not triggered. The beforeinput event is fired after the keydown event.
  • By adding beforeinput listener as a fallback for keydown handler makes it possible to recognise content deletion (Backspace press) on Android (evt.keyCode === 229 && evt.inputType === 'deleteContentBackward'). In such cases delete command should be fired in the same way as in keydown handler.
  • One more thing is that injectUnsafeKeystrokesHandling should not delete selected content in such cases so some additional check is needed there.

@Reinmar
Copy link
Member

Reinmar commented Jul 11, 2018

If deleting is detected, it is processed (removing content) and the keydown event propagation is stopped. This results in beforeinput event not being fired at all.

We can go both ways – use beforeinput on full throttle so, on browsers which support it, also to handle unsafe keystrokes. In fact, in such a case we'd need to check whether we need that hack (but I think we will, to handle IME).

@Reinmar
Copy link
Member

Reinmar commented Jul 11, 2018

One more thing is that injectUnsafeKeystrokesHandling should not delete selected content in such cases so some additional check is needed there.

Ouch, ignore my last comment. I see you covered everything in your message.

@f1ames
Copy link
Contributor Author

f1ames commented Jul 31, 2018

I just wanted to sum up what I have concluded after few days of testing beforeinput event on Android (since I will be switching to other tasks for now).

I have pushed some MVP branches which utilises beforeinput event - https://github.com/ckeditor/ckeditor5-typing/tree/t/167 and https://github.com/ckeditor/ckeditor5-engine/tree/t/ckeditor5-typing/167.

Utilising beforeinput helps with issues to some extend like #1127, however:

  • Since beforeinput cannot be prevented it is not possible to prevent default browser action when Backspace is pressed (keydown is fired before beforeinput) which results in additional characters being removed in some cases (I've experiment with preventing further events, but without a success).
  • After a successful content removal, selection is placed in an incorrect place. From what I've checked it is correctly placed after delete command is executed, but then it jumps to a different place - it might be connected to the point above.
  • It behaves a little different on Android 6 and Android 8 (also [Android] Unstable selection on 'backspace' press results in incorrect content removal in some cases #1127 is not reproducible on Android 6 so there are few different behaviours).

It seems it might be possible to reach some satisfactory improvement, but still beforeinput event is not easy to work with and it requires a lot of time consuming testing (especially due to some unstable behaviours like in #1127). It seems doable, but will definitely require significant amount of work.

@scofalik
Copy link
Contributor

scofalik commented Jun 19, 2019

I am picking this up now and want to summarize some of the research I did to get into the issue. There is a lot of information in a lot of posts added across various issues and it is hard to follow that, especially since some of that already got outdated either due to changes in our code or due to changes in browsers.

For that reason, I want to sum up once again what's going on and what problems we have with typing on Android. Thanks to @f1ames for setting foundations for that.

So, deleting is incorrectly handled on Android. For non-collapsed selection, twice as much content is deleted than it was selected. At the beginning, I thought it is a lot of magic, quirkiness and nonsense but after digging deeper and deeper I think that we might be able to do something there. Okay. How does it happen?

tl;dr: Input handling differs a lot between Android and desktop and we need to cater our algorithms for those differences. Not sure if one-solution-to-fit-all will be possible. On Android, the content is first removed programmatically by us (in one of the event handlers) and then also remove mutations are generated because our events handling algorithms are not suited for how Android Chrome behaves.

First of all, deletion is not recognized as deletion because in keydown event, the keyCode is 229. So DeleteObserver does not interfere. If, for example, R was pressed (instead of Backspace) then it also gets 229.

We have injectUnsafeKeystrokeHandler. 229 is not a safe key code but it is also a key code connected with compositions so, depending on other factors, content in the selection sometimes is and sometimes isn't deleted. Let's assume it was deleted. injectUnsafeKeystrokeHandler is hooked on keydown event. After it is handled, other (internal) browser mechanisms kick in. They base on the selection state to perform further actions on the content.

If the content is deleted, then the selection is set to be collapsed. This is where the problems start. Selection behaves differently on Android than it does on the desktop. I am not sure if it is a browser's fault (Android Chrome vs desktop Chrome) or IME's fault (virtual keyboard vs physical keyboard). So let's see what happens using plain contenteditable without CKE5. We will test Backspace and R on non-collapsed selection.

I modified @f1ames codepen to check what exactly happens: https://codepen.io/anon/pen/rELxwd?editors=1010. You can uncomment and change the if statement to beforeinput to test the differences.

Desktop Backspace scenario. It works the same for collapsed and non-collapsed selection:

  1. Selection remains unchanged on keydown and beforeinput.
  2. Browser performs "deletion" action treating current selection like it was the selection before pressing Backspace. This is important, I'll get back to that later.
  3. Mutations are generated.

Desktop R scenario:

  1. Selection remains unchanged on keydown and beforeinput.
  2. Browser performs "insert R" action treating current selection like it was the selection before pressing R. If the selection is non-collapsed, the selection's content is replaced with R. If the selection is collapsed, R is inserted.
  3. Mutations are generated.

What happens on Android though is more magical. I am sure it can be somehow explained given that all virtual keyboard input is treated kind-of-like-IME-or-composition (according to Chrome devs/docs). But, unfortunately, it differs from what happens on a desktop by quite a lot.

Android Backspace scenario:

  1. Selection becomes collapsed to its end on keydown.
  2. Browser somehow remembers how many characters were to be removed and on beforeinput extends the selection properly (so back to the selection that was set before pressing Backspace). Note, that if the selection was collapsed before pressing Backspace, it gets extended by one character (the one to be removed).
  3. The action ("deletion") is performed exactly on the current selection. It is different than on a desktop, where the selection is not extended if it was collapsed. This is important if we are to change the selection in one of those events.
  4. Mutations are generated.

Android R scenario:

First of all, if the selection was non-collapsed, there are two "actions" performed here. First is deletion and it works like above. Then events for "insert R" are generated. TBH it is quite nice that the browser separates those actions, unfortunately, it is not a standard behavior.

  1. Selection remains unchanged on keydown and beforeinput.
  2. "R" is inserted.
  3. Mutations are generated.

The next step was to check how changing DOM selection in events callbacks affects what happens on Android:

  1. If selection is collapsed at some position on keydown, on beforeinput it is extended from that position. But it is still extended, so the browser remembers somehow how many characters needed to be removed. For example, if Foo [bar] baz was selected, on keydown selection is collapsed at Foo bar baz[], then baz will get removed.
  2. If selection is changed on beforeinput, the new selection range is used to apply changes. For example, if Foo [bar] baz was selected, on beforeinput selection is changed to Foo bar [baz] then baz will get removed. If selection would get collapsed, nothing would be removed.
  3. However, after the action is handled, on keyup the selection is moved by the number of characters that were supposed to be removed.

At this point, I was happy with the research I've done. I understood how typing works on Android and how to change our scripts to handle it correctly. Then I decided to switch keyboard to GBoard and see how it works. And I got extremely disappointed.

It seems that GBoard is the default keyboard, while I tested Switfkey (which is default on Huawei). Honestly, I didn't think which one I am testing and that the differences might be so big.

So, long story short GBoard treats every typing as composition. This is because of auto-correction / word suggestions (when I turned them off it started to work similarly to Swiftkey). I guess one could argue that it makes some sense 🤷‍♂but really it wasn't something I expected.

So, fired events, their data, selection behavior, browser reacting to selection change on events -- it is all different. Even removing a character is treated as a composition change and insertCompositionText is fired instead of deleteContentBackward. It fires even more events than Pinyin keyboard on the desktop. Even moving selection inside a word fires composition events. It is also bugged itself. On clean contenteditable selecting a word (or generally, some text between two spaces) and pressing a letter key sometimes ends up in removing only the first character in the selection instead of everything in the selection. And it happens randomly (although pressing the keyboard key for a longer period of time seems to cause it every time).

Fortunately, removing selected content generates events as on Swiftkey. Unfortunately, replacing selecting text does not provide two events as on Swiftkey. Instead, one beforeinput with inputType: 'insertText' is fired. Thankfully, it behaves as expected when selection is changed on beforeinput. So, given that the bugs on Android are connected with non-collapsed suggestions handling, we might get somewhere (as long as the bug described above do not happen, that is).

After this research, it is obvious that our current solution won't work well on Android. Our handling is done on keydown where content is deleted and selection collapsed but the selection change has no effect on Android because it gets re-collapsed and content is deleted again.

We cannot go fully into beforeinput because most desktop browsers do not provide that event. So we need a hybrid solution. We need to listen to keydown and to beforeinput. It seems that there's no place after both of them where we could check if beforeinput was fired/handled. I mean, there is, but it is input event and AFAIR there is too late for taking actions at that point.

So, I think that we need dedicated handling on keydown for desktop browsers and dedicated handling on beforeinput for Android. Not sure if it is a good idea to sniff if beforeinput event is available, just better use UA I guess. We can skip handling in keydown (or set high priority listener that will cancel keydown) on Android and make correct handling for Android in beforeinput.

@jodator
Copy link
Contributor

jodator commented Jun 24, 2019

It seems that GBoard is the default keyboard, while I tested Switfkey (which is default on Huawei). Honestly, I didn't think which one I am testing and that the differences might be so big.

Did you try also Samsung keyboard? I had to install GBoard on Samsung as they provide their own keyboard. Also this might be relevant to other vendors.

@scofalik
Copy link
Contributor

Samsung keyboard, pre-installed on Samsung devices, works even differently 😵😵😵. From a very quick check, it looks like it works similarly to Chrome desktop. It fires correct keydown event with keycode 8 (Backspace) and do not extend selection on beforeinput.

@scofalik
Copy link
Contributor

PLOT TWIST: When you switch to Chinese (Pinyin) on GBoard, it starts to behave like Samsung keyboard, that is, when using Backspace, it suddenly starts sending "correct" keydown events and stops expanding selection on beforeinput.

pjasiun referenced this issue in ckeditor/ckeditor5-typing Jul 1, 2019
Feature: Improved typing on Android devices by handling `beforeinpnut` event. Introduced `options.selection` in `DeleteCommand` params. Introduced `selectionToRemove` parameter in `view.Document#event:delete` data. Closes #167.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-typing Oct 9, 2019
@mlewand mlewand added this to the iteration 25 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. package:typing labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:typing type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants