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

Android input handing rewrite #4988

Merged
merged 41 commits into from
Jul 29, 2022

Conversation

BitPhinix
Copy link
Contributor

@BitPhinix BitPhinix commented May 14, 2022

Description

This PR contains a re-write of the android input handling replacing the mutation reconciliation-based approach with a beforeInput based one. It also merges the AndroidEditable and EditableComponent into one as maintaining both adds a lot of overhead and results in the AndroidEditable not receiving all changes (see e.g. #4621 for details)

The main issues with handling input on android compared to our 'normal' desktop handling are that beforeInput events on android aren't cancelable and that trying to modify the DOM with incorrect timing causes the keyboard to behave unexpectedly and 'fight' those changes.

A quick really broad overview of the changes in this PR:

RestoreDom(-Manager)

The beforeInput events not being cancelable causes major issues with react as some changes modify the DOM tree quite heavily and result in the DOM not being in the state react expects it to be, thus crashing on the next render. To get around this we changed the key of the editable component in the current implementation which caused react to re-create and replace the entire DOM node. This is both really bad for performance on longer documents and potentially causes issues for some IMEs.

This PR replaces this approach with a MutationObserver based approach which tracks all user input caused mutations inside the editor and reverts them directly before react commits the new editable state to the dom. This ensures the DOM is always in the exact state react expects it to be and saves us from re-mounting the entire editable thus resulting in a big performance gain over the current approach.

AndroidInputManager

Contains almost the entire input handling logic. We detect all changes based on beforeInput events. Since we can't apply them directly as they would cause a DOM modification/selection update which cancels composition and/or causes the IME to fight the dom changes we store the user-intended change either as a diff or an action.

diffs are simple text changes that don't require a re-render to visually match the state we would be in if we would apply those changes to the editor.

actions are changes that require us to perform DOM modifications to ensure the user sees a visually matching DOM state to the editor state with the changes applied or changes that result in a DOM state we can't get the slate selection from.

Generally, we apply actions as soon as it's safe to do so without interfering with the IME. This seems to be upon the next input event (or after a certain timeout as the input event isn't always fired reliably).

We wait until we apply pending text diffs to the editor state until the user stops composing since applying those diffs would cancel to composition thus interfering with typing compositing-based languages like Japanese, or composition would be canceled anyway (e.g. on re-rendering due to a remote change).

Since diffs are potentially long-lived with this approach we have to take care of transforming them by 'external' editor changes to not be invalid after performing e.g. changes by a remote user. This also comes with a lot of challenges of changes being based on the editor's state after applying those diffs without us being able to ensure that the state actually matches. I won't go into details here but most of this conflict resolution is done in the diff utils.

Mark placeholders

This PR replaces the current concept of composition insert prefixes with a decoration-based alternative of 'mark placeholders'. This is mainly due to insert prefixes not playing well with collaboration and a way more frequent usage on android. They both solve the issue of making the current editor marks part of the actual DOM to ensure the user sees a visually matching state while composing. On Android, we don't have the luxury of being able to move the selection around on composition start without interfering with the IME so we always render them.

Fixes:

and various other undocumented issues

Remaining issues:

You tell me

@changeset-bot
Copy link

changeset-bot bot commented May 14, 2022

🦋 Changeset detected

Latest commit: ecf2256

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
slate-react Minor
slate Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jasontlouro
Copy link

Just checking in since it's Friday @BitPhinix - anything I can do to help?

@BitPhinix
Copy link
Contributor Author

@jasontlouro Discovered a few more issues during the final QA round, but everything should be gtg now 🙏 I'm sure there are a few issues I haven't discovered yet, but these are things that can be ironed out later. @thesunny, I'll add a playground (and do a bit of refactoring) in a follow-up pr, but lets merge this as many people seem eager to have this released.

@RaulPROP
Copy link

Just so you know, all 3 of these bugs (#5052) are solved.

So, thank you and great work.

@dylans dylans merged commit fbab633 into ianstormtaylor:main Jul 29, 2022
@github-actions github-actions bot mentioned this pull request Jul 29, 2022
@jasontlouro
Copy link

This is working GREAT so far. Thank you 👏

@kaya-altin
Copy link

I'm using Microsoft SwiftKey Keyboard. The keyboard closes when I enter the first character.

Screen_Recording_20220801-135246_Chrome.mp4

@BitPhinix
Copy link
Contributor Author

Thanks for reporting! It's caused by Swiftkey not being able to handle the non-contenteditable placeholder element. I fixed this before but must have broken it in the last few commits. Will create a PR with a fix for this (and some other small things) asap 😄

@antonbakinowsky
Copy link

antonbakinowsky commented Aug 2, 2022

Example with mentions stopped working.
If i got it right then onChange is not being triggered anymore on Android on every change. But mentions example is relying on this.
https://www.slatejs.org/examples/mentions
https://github.com/ianstormtaylor/slate/blob/main/site/examples/mentions.tsx#L76

broken-mentions.mp4

@BitPhinix
Copy link
Contributor Author

Yes, that is to be expected as the new implementation mimics how composition is handled on desktop. I'll update the example soon, you can use the pendingDiffs to make it work

@kaya-altin
Copy link

When I focus or press any key, an error like the one below occurs. (0.82.1)

image

@Nantris
Copy link
Contributor

Nantris commented Sep 6, 2022

@kaya-altin does it occur with an earlier version like 0.81.x? Can you report the first affected version of Slate? And can you share what keyboard you're using and your Android version?

@kaya-altin
Copy link

kaya-altin commented Sep 7, 2022

There is no problem in slate-react 0.81.0, but this error occurs after updating to slate-react 0.82.1. In addition, the error is not only available on android, but also on pc or mac.

Screen.Recording.2022-09-07.at.08.41.38.mov

@Nantris
Copy link
Contributor

Nantris commented Feb 9, 2023

@BitPhinix thanks again for your great work. I noticed your project LiveBlocks is pinned to "slate": "^0.82.0". I wonder how realistic it is for us to expect these changes to continue to work in newer versions?

We have to decide between upgrading Slate from 0.47.x or moving to Lexical, so I'm trying to gather whatever information I can.

z2devil pushed a commit to z2devil/slate that referenced this pull request Dec 6, 2024
* wip

* wip

* wip - fully working without hard marks

* fix editor crashes when inserting/deleting at the edges of marks

* fix various restore dom related crashes

* fix delete with pending changes, zero widths on android, mutation tracking

* track placeholder delete in detached strings, zero-widths

* wip mark placeholders

* get rid of mutation detection in favor of beforeinput

* fix various selection race conditions

* fix various crashes when deleting at the beginning of nodes

* wip diff transforms, selection handling fixes

* cleanup restoreDOM and fix noop restore edge-case

* fix mark placeholders

* fix toSlatePoint edge-case

* properly flush user select with pending changes

* Prevent editor crash when deleting before a non-contenteditable element

* wip markdown shortcut example

* transform pending changes and selection by remote changes, simplify pending actions, handle all input types

* improve change transform, mark(-placeholder) handling

* manually handle gboard bug, fix restoredom nested editor

* fix parent mutation condition

* cleanup, mark placeholder fixes

* mark placeholder fixes

* fix mark placeholder condition

* hide placeholder if we have pending diffs

* cleanup

* yarn install

* add workaround for swiftkey placeholder issue

* cleanup

* add changeset

* feat(slate-react): fix edge-case crash, add androidPendingDiffs, rename scheduleFlushPendingChanges

* flush pending selection on same line without pending changes/action

* keep formatting of pending diffs when adding/removing selection marks

* unref selection ref on unmatching dom state

* improve markdown shortcut example flush trigger to show how a more generic solution would work

* fix markdown shortcut example trigger logic

* fix isInsertAfterMarkPlaceholder logic
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 this pull request may close these issues.

10 participants