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

[beforeInput] Bring back basic typing support in Android #12058

Closed
Reinmar opened this issue Jul 14, 2022 · 8 comments
Closed

[beforeInput] Bring back basic typing support in Android #12058

Reinmar opened this issue Jul 14, 2022 · 8 comments
Assignees
Labels
domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). Epic squad:core Issue to be handled by the Core team. type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@Reinmar
Copy link
Member

Reinmar commented Jul 14, 2022

In a course of action in #11438 we wrote a composition support for non-Android browsers (see #12024).

Unfortunately, while it works really well on every other system and browser and every language (including CJK), it does not work on Android at all.

We need to implement a completely custom handling for Android browsers.

Keyboards that we'll try to support from day one:

  • The default one
  • GBoard
  • Swift

ToC

@Reinmar Reinmar added type:task This issue reports a chore (non-production change) and other types of "todos". domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). labels Jul 14, 2022
@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Jul 14, 2022
@Reinmar
Copy link
Member Author

Reinmar commented Jul 14, 2022

Let's try to approach this in 3 stages:

  1. Research events fired by Android keyboards. See: https://www.notion.so/Typing-events-scenarios-8d750fcdc8984e4fa5126e78b98e4047.
    • The goal is to compile a event model common across all major Android keyboards and identify the most important quirks.
  2. Try recreating minimal support for typing on Android to unblock releasing the work done in Basic IME support #12024. It can be really incomplete as long as it does not crash. Release this minimal support together with the entire Basic IME support #12024 as soon as possible.
  3. Take time to figure out a more complete implementation and finally close the IME&beforeInput topic for real. The last step may take significantly longer than step 2, hence it's separated.

@Reinmar Reinmar added the squad:core Issue to be handled by the Core team. label Jul 14, 2022
@Reinmar Reinmar changed the title [IME] Bring back basic typing support in Android [beforeInput] Bring back basic typing support in Android Jul 14, 2022
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Aug 1, 2022
@niegowski
Copy link
Contributor

The main differences for Android IME (typing with an English keyboard):

  1. every time composition is replacing the whole word (even if only some characters were added or removed)
  2. it keeps active composition while changing the selection between words in the document
  3. there is no target ranges in the composition beforeinput events instead it's manipulating the DOM selection to reflect the text part that should be affected by the input

Emulate desktop IME behavior (the first attempt to solve those issues)

Trying to keep as much of the original IME handling as possible and try to work around the issues mentioned above.

Note: Renderer and selection observer were disabled while in the composition state (from the editor's point of view).

  1. the insertText event was triggered on the composition end (no matter if the real one or the fake one)
    • the content and affected range were accumulated on beforeinput composition events
    • while committing the composition the text affected by the composition was compared with the one in the composition event and only differences were fired in the event (to prevent striping text node attributes while replacing the text node)
  2. monitoring the selection changes to verify if it's still composing the same word or if it moved to some other place
    • in such case detected it was triggering false composition end
    • the composition (editor state) was restarted on the first composition event after that
  3. for the affected range, the DOM selection is used

This approach caused problems with DOM - View position mapping because of the disabled renderer and the fact that the browser is wrapping some text with links or bold etc. There were some other problems that I can't recall now but maybe we should try to check it again.

Trying to ignore composition and just use the beforeinput composition events

This idea was to not ignore insertCompositionText type of beforeinput event so that every change would get applied to the model and view immediatelly so in theory there should not be problems with the position mapping.

  1. the insertText event is triggered on every insertCompositionText 
    • the text affected by the composition was compared with the one in the composition event and only differences were fired in the event (to prevent striping text node attributes while replacing the text node)
  2. this approach completely ignores the isComposing flag, renderer is not disabled
  3. for the affected range, the DOM selection is used (as in the previous attempt)

Unfortunately, this approach also had issues with non-English keyboards because it broke the composition even harder than on the master branch. It was enough to compose in plain text to get the composition broken.

Rendering only if some document structure was changed (not text)

This is an extension of the previous attempt that is trying to trigger render in cases where some text node needs to get wrapped with some inline element (for example link or some bold etc). This would allow composing in plain text context but would still break composition while typing inside a link (same as on master).

While trying to detect whether render should update DOM I noticed that current beforeinput handling is marking the whole container element as modified and the renderer is replacing the whole DOM text node instead of modifying its content. This might be the case of broken composition with non-English keyboards. I need to research it more to find out if we could fix the rendering so that it would not replace text nodes but update those modified. This would also affect desktop browsers because this bug of replacing the whole node is also affecting them.

@niegowski
Copy link
Contributor

niegowski commented Aug 18, 2022

After expanding Renderer#_updateChildren() to be able to detect text node changes and update text nodes instead of replacing them I realized that this is still not enough because this whole flow is triggered on beforeinput so the text typed by the user is not yet in the DOM and since composition events can't be stopped this breaks composition. 

The idea to solve the above problem:

  • the renderer should be disabled while composing (even on android) 
    only updating text nodes should be disabled while composing
  • on every compositionupdate event, the renderer should be triggered
    the renderer is triggered after every change in DOM by the MutationObserver
    • if it was correct and matches the view then the renderer does not change anything
    • if it should be wrapped with a link or moved outside it then it fixes it and this breaks composition the same as on typing based on mutations (master)

@niegowski
Copy link
Contributor

niegowski commented Aug 19, 2022

PoC branch: ck/epic/11438-migrate-to-beforeinput...ck/11438-beforeinput-ime-research-vol1.1-android

Some problems with partial solutions:

  1. Replacing  misspelled words by picking the correct version from the dropdown was not rendered
    • This was caused by the fact that DOM text nodes were not updated while the isComposing flag is set
    • The beforeinput event got prevented (insertReplacementText is not a composition event, so it can be prevented)
    • The solution to this issue was to preventDefault() only those before input events that got triggered while not composing
  2. Backspace and deleting content was causing an inline filler to get reduced to 6 characters (and considered lost)
    • This was caused by the fact that on Android the deleteContentBackward is ignoring preventDefault()
    • The selection after a link was just after an inline filler so the last character of it got removed by the browser
    • There is a workaround in the above PoC branch that does not use inline filler on android if there is any node on any side of the caret
      • This fixes the backspace problem
      • This sometimes causes that typing after a link is not possible (content jumps into the link)
        • This needs more checking
      • Inline filler is still rendered in an empty inline element to make it possible to type inside a strong element etc.
  3. Pressing enter key does not trigger proper beforeinput event on some devices, instead it triggers insertCompositionText with data: test\n
    1. I wasn't able to reproduce it so no solution for now
  4. Composing after an English word by using the SwiftKey keyboard and trying to compose in Japanese causes the whole word to change to Japanese characters
    1. The same is happening in a plain content-editable so this is probably out of the scope

@Reinmar
Copy link
Member Author

Reinmar commented Aug 22, 2022

Pressing enter key does not trigger proper beforeinput event on some devices, instead it triggers insertCompositionText with data: test\n

  1. I wasn't able to reproduce it so no solution for now

That sounds like #11906.

@Reinmar
Copy link
Member Author

Reinmar commented Aug 23, 2022

@ckeditor/qa-team I think it's worth retesting the whole thing on Android.

The branch to test is ck/11438-beforeinput-ime-research-vol1.1-android.

Keyboards to cover:

  • Default
  • GBoard
  • Swift

Languages:

  • English
  • One of Japanese layouts

Note: Make sure to test on the newest Chrome as something changed recently. Also, we had problems setting up a simulator with all 3 keyboards. Together with @niegowski we were following this guide: https://www.notion.so/Setting-up-Android-Emulator-9708664ec48e429686ab587bb1167c8a but at different moments and I ended up having 3 keyboards to choose from while Kuba just 2. We didn't figure this out and each of us tested what we could.

One additional favor I'd like to ask you: Could you also review (and close if not reproducible anymore) the issues reported specifically for the previous attempt to resolve Android issues? The list: https://github.com/ckeditor/ckeditor5/issues?q=is%3Aissue+is%3Aopen+ck%2F11438-beforeinput-ime-research-vol1.2-android

@Reinmar
Copy link
Member Author

Reinmar commented Aug 31, 2022

Remaining issues to cover now:

@niegowski
Copy link
Contributor

Closed in #12446.

@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). Epic squad:core Issue to be handled by the Core team. type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

No branches or pull requests

3 participants