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

fix editing with "soft keyboards" (eg. Android, IMEs) #2062

Open
ianstormtaylor opened this issue Aug 8, 2018 · 201 comments

Comments

@ianstormtaylor
Copy link
Owner

commented Aug 8, 2018

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

Bug.

What's the current behavior?

The issue with using Slate on Android is complex, and due to a difference in how its OS keyboard is designed. On mobile devices, keyboards are starting to move away from the "keys" concepts in many ways...

  • Autocorrect will make changes without any "key" being press.
  • Autosuggest will insert words that don't map to keys.
  • Swipe-to-type will insert entire words in one go, instead of using keys.
  • etc.

Because of this, it sounds like the Android team (reasonably from their point of view) decided to not reliably fire key-related events.

As soft input methods can use multiple and inventive ways of inputting text, there is no guarantee that any key press on a soft keyboard will generate a key event: this is left to the IME's discretion, and in fact sending such events is discouraged. You should never rely on receiving KeyEvents for any key on a soft input method.
KeyEvent, Android Reference

It sounds like the behavior is:

  • The keypress event is never triggered, which is fine for us.
  • Text-based keys like a, b, etc. fire a keydown event but always with a key code of 229, indicating that the key is unidentifiable because the keyboard is still busy processing IME input, which may invalidate the actual key pressed.
  • Pressing keys like enter fires a keydown event as normal, with an event.key that can be recognized? (This is unclear whether this happens or not.)
  • Pressing backspace does not fire a proper keydown event.

A few different resources:

What's the expected behavior?

The fix for this is also complicated. There are a handful of different, overlapping pieces of logic that need to change, to accommodate a handful of different input types...

The first stage is to handle basic insertions, and auto-suggestions...

  • Remove the preventDefault in onBeforeInput, so that the DOM is updated, and the onInput logic will trigger, diffing the insertion and then "fixing" it.
  • Update the <Leaf> (and other?) components to increment their key such that React properly unmounts and reconciles the DOM, since it has changed out from under it.

This is actually the same starting steps as is required for #2060, so I'd recommend we solve that issue in its entirety first, to work from a solid base.

This fixes the actual text insertion pieces, and probably deletions as well. Splitting blocks can still be handled by enter because it still provide proper key codes.

  • Check that all other behaviors that aren't text insertions (eg. splitting blocks) are handled properly without access to many keydown events.

And then there's some selection issues, which apparently can mess up Android's IME (and potentially others) if the selection is manually changed during a composition.

  • Prevent re-rendering the editor on compositionstart and compositionend.
  • Prevent updating the selection while a composition is taking place.

I think this would solve the 90% case for soft keyboard input.


Separately, there's still another question of how to properly handle these kinds of behaviors other plugins. For example if a plugin uses backspace at the start of a block to reset that block, that won't work on Android. So after solving the input issues, we need to step back to an architectural level and solve this plugin handling problem. But that can wait.

@ianstormtaylor ianstormtaylor changed the title fix Slate's behavior with "soft keyboards" (eg. Android, IMEs) fix behavior with "soft keyboards" (eg. Android, IMEs) Aug 8, 2018

@ianstormtaylor ianstormtaylor changed the title fix behavior with "soft keyboards" (eg. Android, IMEs) fix editing with "soft keyboards" (eg. Android, IMEs) Aug 8, 2018

@ianstormtaylor

This comment has been minimized.

Copy link
Owner Author

commented Aug 9, 2018

I've done some input tests in different browsers and devices using Dan Burzo's input tester.

I think the only way we're going to arrive at proper Android support is to use a tool like this, and take extremely detailed recordings of inputing the exact same text across the different platforms. Otherwise the intricacies of the ordering of events, and when compositions do or do not start is going to be too complex to guess.

I've started with a few, we'll need more. I think we'll need:

  • Inserting text.
  • Inserting IME text.
  • Inserting a word via auto-suggest.
  • Inserting a word via gesture typing.
  • Auto-correcting a word.
  • Deleting characters backwards.
  • Deleting characters forwards.
  • Delete characters backwards, across word boundaries.
  • Selecting a word, then deleting it.
  • Selecting a range across blocks, then deleting it.
  • Moving to the end of a word.
  • Moving to the middle of a word.
  • Moving to the middle of a word, then inserting text.
  • Splitting a block at the end.
  • Splitting a block in the middle.
  • Joining a block from the start.

Even from just the ones I did, we can already see complex composition behaviors on Android that we're going to have to solve for:

  • Moving the cursor into or to the edge of a word, starts a new composition immediately (even without any input) with the word as content.

  • Deleting characters backward doesn't trigger any useful key events, but it does update the composition ticking down the characters in a word.

  • All text insertions on Android is via composition events, so in solving for its composition handling, we'll probably get desktop IME support in the process.

I think we'll need to store additional information in these tests of what the current window.getSelection() range is, because that is key to understanding how much information we can get from these composition events. Someone might need to build a custom input tester for Slate specifically to make this easier.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Aug 10, 2018

@ianstormtaylor

You are making a lot of progress here. I’ve committed next week to working on mobile support but by a large margin, you are more qualified to solve this than I.

I think we might make more progress if you give me any menial tasks, research, etc. In order to help you. Don’t feel obligated to give me work but if you have any tasks, I will work on them full time for you starting Monday. Let me know.

@ianstormtaylor

This comment has been minimized.

Copy link
Owner Author

commented Aug 10, 2018

@thesunny thanks! One of the most helpful things would be getting more of those detailed event samplings across devices/browsers for each type of edit. That would allow anyone who works on this in the future to reference them to make sure they're thinking through the event logic correctly.

Without those I think we'd just be fumbling around guessing what logic we need to add to the After plugin to get composition events working.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Aug 10, 2018

Okay, I will start with that Monday

@Slapbox

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2018

@ianstormtaylor Dan Burzo's input tester is exactly what I was looking for the other day to send you. Glad you found it/already had it.

@thesunny and @ianstormtaylor how/where are you laying down code for this? I don't think there's much I can contribute in the way of code in the short term, but I'm happy to test ideas on my Android device and help any other way that you two need.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2018

I created DropBox Paper documents. I set up a testing environment and put together all the tests below

So, a few differences from @ianstormtaylor is that I tested 6 different versions of Android and they all provided different values. There are more differences within the different Android versions than across all browser/OS versions. iOS, Safari and Chrome are very similar.

(1) Forked the @danburzo app to accept HTML tags like <p></p> and can be found here

I'll keep updating this list.

@Slapbox

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2018

@thesunny can you list the browser versions for the Android tests? I'm guessing Chrome 68?

Also, we should agree on which field we're using in that input tester, either the Simple DOM contenteditable or the React-managed contenteditable.

I think it's reasonable to expect Slate to only have out of the box mobile support via React. This would simplify the task, I think. What do you think @ianstormtaylor ?

I believe the only events we should be relying on, if we want the widest base of support on Android, are:

  • compositionStart
  • compositionUpdate
  • compositionEnd
  • deleteContentBackward (maybe)

FYI when typing, each keystroke updates the composition, but choosing a suggestion ends the composition and creates the final word like this:

compositionData: Barnac
Suggestion: Barnacles

If you click Barnacles, the final word is constructed like this.

compositionEnd Barnac
insertText les

Barnacles

after this word, Google suggests the word, "the" - If you click this word, it is inserted like this:

insertText the (there is a space inserted before the word)

Note, the above data is from the React contentEditable, but appears to be the same in the simple contentEditable.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2018

The browser version I believe is locked to the Android API version. I'm using whatever Chrome came with the specific Android version. I guess one thing where you can help is to look up which Chrome comes with which Android version.

I'm using the simple DOM contenteditable because that's what Ian used in his examples.

I was considering writing a web App that would store all this information in a database so I wouldn't have to screenshot everything and we'd also have the raw data as JSON somewhere. I'm not sure how long that would take though so for now I'm just manually see if I can get this all done.

I'm surprised how big the differences are between each Android version.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2018

@ianstormtaylor Just a heads up that I'm going to redo your Input Tests adding the multiple Android versions.

Edit: Completed tests that don't require blocks as the Input Tester doesn't support them.

Edit: I'm going to go back and add tests for Android API 27. It's a minor upgrade Android 8.0 - 8.1 but just to be safe.

Edit: Also missed Android API 24 (which was mistakenly API 23). Fixing with an @v1.1 to identify that it's been fixed.

Edit: Forked input-methods so that the contenteditable contained two <p> blocks with some text in it in order to complete the tests. https://thesunny.github.io/input-methods/index.html

@ianstormtaylor

This comment has been minimized.

Copy link
Owner Author

commented Aug 14, 2018

@thesunny these are absolutely amazing! Thank you!

Having them for multiple versions of Android is a great idea. And using the non-React input is best too, since we need to know what exact DOM events are fired, regardless of how React chooses to interpret them. Because we'll probably need to bypass React's event handling logic in places, until they catch up.

Another question for us to answer would be what are the usage levels of the different Android versions.

Regardless, starting by trying to get things working with the latest versions that have Chrome's that fire beforeinput seems best, since that makes things easier I think. There's a decent change that adding support for some of the older ones would be much harder.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2018

@ianstormtaylor popularity of platform can be found here but unfortunately beforeinput support starts at API27 which has only 2% market share.

If we go back to API 23, we can capture 66.4%. The biggest version in distribution is API 23 which has 23.5% distribution.

I'm just looking through some of the charts and looking for some patterns.

In Inserting Text I think we may be able to get away with:

  • Ignore keydown events. This is because a keydown will happen before a compositionstart so we can't know if a keydown is part of a composition until later. We can't know if we want to process based on the keydown event.
  • We process all keyup events that don't happen within a compositionstart and compositionend using a DOM diff. In the test, this is always a space (presumably because it doesn't require composition since it's valid in itself).
  • When there is a compositionstart then we ignore all the keystrokes. When a compositionend event happens, then we do a DOM diff to create a Slate insert operation.

Some analysis I did on other input methods to see if this strategy holds:

  • Japanese IME: It appears valid for IME in API 28 as well.
  • Gesture typing: appears valid for gesture typing
  • Moving to the middle of a word then inserting text. This appears to work but in some API versions, the text is wrapped in a composition and in others they are just simple key events with no composition. Either way, the logic still works.

It seems like the code might be written something like:

  • For Android only, we allow all regular insertion keypresses to all flow through naturally (not including ENTER, BACKSPACE, etc)
  • If we find a compositionstart event, we block all operations until a compositionend event materializes. At the time o f a compositionend, we do a diff of the DOM and create an insert op with {isNative: true}.
  • If we find a keyup event that is not wrapped in a composition we also do a diff and create an insert op with {isNative: true}

If we aren't in Android, we could theoretically do a diff and use the same code base for everything; however, this might be a performance issue. On the other hand, it could potentially help performance if we debounce key events and end up with a bigger diff and one insert operation at time of keyboard rest. I think we should think about this.

Of course we'd still have to handle ENTER, BACKSPACE, etc. individually but we can probably make a short list of operator keys which is probably ENTER, BACKSPACE, SPACE, and PUNCTUATION.

Usually we don't have Plugin actions from typing in Slate unless they are accompanied with one of these keys anyways. For example, a markdown plugin might use * for a bullet or #heading but all of these are punctuation. I also use o for an empty task myself but that has a space in it. So we would have to limit special Plugin handling to be around these keys but that might be acceptable and it's also a good proxy for the special handling to work in Android anyways because Android will always composition normal words so that key handling would fail in Android.

The plugins may need a different API though that exposes something like onInsertText and onNonWordKey. In this API \we can't do something like have the input onetwothree be converted on the fly to 123 as we type it but we could have one two three work because of the spaces calling an onNonWordKey. This may be a feature since onetwothree live converting could never work in Android due to the way it composes.

@ianstormtaylor

This comment has been minimized.

Copy link
Owner Author

commented Aug 14, 2018

@thesunny great points all around.

It sounds like one thing we need is a list of key presses that don't trigger compositions and instead use keydown to insert directly? It sounds like space is one of these? But we need a full list. (And as you mention, plugins that want to support Android would need to restrict their keydown usage to these keys, or we need to come up with some new abstraction later.)

Do we need to worry about isNative: true for now? Or can we re-render once the composition has finished with compositionend? Being able to punt that (even just for getting it working with later Android versions) would make it a lot easier.

Fair enough for backwards compatibility. I think whoever implements it would likely want to start with API 28, since it's the most standardized. Or even just ignore Android quirks to start and focus purely on the compositionstart/update/end trifecta. And then handle all of the specific quirks around compositions that need to be ignored.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2018

@ianstormtaylor

I'll do some more research on what keys have special cases with respect to composition like space. I assume enter is one but I'm not sure about punctuation.

That's an interesting idea about not worrying about isNative: true. So I guess the idea would be that we simply would not update the state (and hence not trigger a re-rerender) until compositionend? Feels like this could work...

In my opinion, we should focus on the compositionstart/end events. Everything I've read on this issue across all of these Editors suggests to me that it is impossible to use the events to reconstruct Editor state without doing a DOM diff with reasonable coverage of Android versions. Since we have to go there eventually, why not start with it. I've designed an algorithm for the DOM diff which I think (hope) will be quite simple to implement so it may not be that hard to get this working.

My next goal is to see if I can create a sample contenteditable React-based editor that uses compositionstart, compositionend and DOM diffing to sync a Slate like Editor State. If I can get that working, we can see how that can merge into Slate proper.

Edit: A preliminary check just on Android 28 suggests that punctuation doesn't trigger composition.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2018

@ianstormtaylor @Slapbox

OMG! I'm super excited!

I created a working prototype. It's a simple React contenteditable editor (one text node in a div) and tested this on three Android versions (including the newest and oldest API versions) and so far it works perfectly!

  • composition works
  • IME works
  • gesture writing works
  • auto correct works
  • suggestions work

It's generalized enough that it works on desktop with no code changes.

I created a custom editor because I don't know Slate internals well enough yet but I tried to make it as close to Slate as possible.

  • It uses insert_text and remove_text ops with the same semantics as Slate (but without path and marks to keep prototype simple)
  • They are applied to a text state (similar to Slate's editor state but simplified to one text node).

It works like this:

  • It reconciles on keyup unless the keyup is within compositionstart and compositionend events.
  • It reconciles on compositionend events
  • It uses text diffing to generate the operations and applies them to Editor state

I also track selection and restore it after update but Slate already does that so can be ignored.

Here's a screenshot for now...

image

I'll try and get this published tomorrow to get some feedback. There will probably be issues but it's promising so far!

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2018

@ianstormtaylor

Here is where you can access the Live Composition Editor

This is the GitHub Repo for Composition Editor

If you type in it from most browsers, it is diffing on every keystroke to create the insert operations and then the Editor is being rendered. After render, I set the selection so the cursor doesn't get lost.

If you use it from Android, when you start a composition via the compositionstart event which happens for most input, the Editor waits until compositionend and then does the diff and creates the operation. You can see this in the operations dump as instead of a single character insertion, you'll see an insert_text with multiple characters in it. In some cases you will also see a remove_text.

I dump the Editor state for transparency which includes all the ops or operations that have been applied.

You can reset the editor to the default text and empty the ops by clicking the Reset button on the page.

@Slapbox can you test this on your Android device?

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2018

@ianstormtaylor

I think to implement this, we would have to change the Plugin event handler properties.

Here's my recommendations.

  • Add an onCompose synthetic event handler property on plugins.
  • deprecate onKeyDown and onKeyUp because it will be unpredictable in Android and could break it

We could have onCompose be the default handler for insertion of characters even for desktop where they'd always compose 1 character at a time. We could consider calling it something like onInsertText but the issue is that in some cases (like switching a word using a suggestion) we'd actually have a remove_text operation along with the insert_text operation. Maybe onChangeText? But onCompose feels okay to me even if it's a little misleading.

My suggestion is to replace the key handlers with a whitelist of specially named key handlers for all the cases we want to make sure Slate can handle. This is safer than having a catch all like keydown and assuming the user will know, for example, that letters in a composition won't fire the event or that they need to not preventDefault on those events or that they shouldn't modify Slate during a composition.

Here's handlers I'd recommend to replace onKeyDown.

  • onHotKeyDown which would fire whenever a key combination (i.e. a character in combination with CTRL, ALT, OPT or CMD is pressed) like CMD+B or CMD+SHIFT+2.
  • onSpace
  • onEnter
  • onDeleteBackward
  • onDeleteForward
  • Or possible combine the two above into onDelete with an event property that specifies the direction? Not sure but I kind of like them separate so as to avoid confusion...

I will look into punctuation but I'm not hopeful that we can rely on these being 100% handled outside compositions because of contractions like can't and isn't.

@ianstormtaylor

This comment has been minimized.

Copy link
Owner Author

commented Aug 15, 2018

@thesunny I think it might be best to separate the work of "getting Android working" from "getting the plugin system fully adapted for compositions". Because the latter is going to be more work, and require thinking lots of different API considerations through.


As for how events flow through the editor...

  1. The <Content> component is the one that renders the contenteditable element in the DOM, and attaches events handlers when it does.
  2. For handlers where we need the native DOM semantics, instead of React's synthetic event semantics, we have to attach them in componentDidMount to the DOM node.
  3. The <Content> component itself is rendered in the After plugin's renderEditor hook, and it passes in the <Editor>'s handler callbacks.
  4. The editor takes any event and runs it through the stack which cascades through all of the plugins.

That's the current flow. There's a bit of confusing indirection around how the <Content> element is rendered. But for the most part you can just look directly from <Content> to plugins and ignore it. In the future we may be able to simplify that stuff.


As for how to handle compositions in plugins. I don't think we need to deprecate onKeyDown/Up yet, because Android helpfully fires the keys with 229 codes so that they won't ever be recognized, last I checked. There may be cases where this doesn't work, but those should be researched/documented well before we make a decision.

I'd rather avoid having onSpace, onEnter, etc. all be their own handlers. As for onDeleteBackward and onDeleteForward, see the "commands" concept discussion.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2018

@ianstormtaylor Thanks for the feedback.

Just working my way through everything now.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2018

Here is my execution plan for now

  • disable state updates in Slate from relevant events
  • allow native updates to flow through to contenteditable div
  • generate diff by locating relevant slate node and comparing it to DOM node (attach initially to keyup events)
  • use diff to create operations to modify state and rerender
  • now do it with composition events
  • enable back all the event handlers and such that are necessary to make slate work properly like hitting enter key

Testing
For all these tests we need to make sure (a) the changes are in the Editor State (b) the selection is in the right spot

IME

  • go to end of a word and type hideki in Japanese
  • go to start of a word and type hideki in Japanese
  • go to middle of a word and type hideki in Japanese

Gesture Writing

  • cursor to end of word then use gesture writing
  • cursor to start of word then use gesture writing
  • cursor to middle of word then use gesture writing

Regular Writing

  • enter a full word then press space
  • enter a full word then press enter
  • enter 2 letters then select a suggestion
  • type "hi" at end of line then hit "enter" (broken in API28)

Suggestions

  • click on a word and select an alternate suggestion
  • click on a word, type a letter, select an alternate suggestion

Auto-correct

  • type cant and let it auto-correct to can't
  • type can and cursor away. Move cursor back to the end of can and type t and then let auto-correct fix it to can't

Physical Keyboard

  • type "Hello World" pretty quickly. Make sure there are no duplicate characters or incorrectly positioned selection.
  • hold the backspace key down
@Slapbox

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2018

@thesunny is there anything in particular I can be of help testing? I checked out what you've put together and it looks very useful. All input and output is rendered to the editor as expected.

One note, not a problem of any sort; I'm surprised by how backspacing is handled. Backspacing away the last character of a completed word fires a single remove_text event, and then another one doesn't fire until you have removed the entire word. Compositions are strange.

@birtles

This comment has been minimized.

Copy link

commented Aug 16, 2018

Thanks for working on this. Please do test with Firefox on Android too. We are looking to update Firefox's behavior (see discussion in w3c/uievents#202) and would appreciate your feedback.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Aug 16, 2018

@birtles You're welcome. I'll test out Firefox once I get Android Chrome working.

As a quick update, I have many things working properly now although I'm not using the diffing algorithm yet and just the replace algorithm that currently exists. It's pretty much working except there are issues with selection placement, the fact that Android doesn't always have an onCompositionEnd event (which makes no sense) and also the composition events are fired before the content is actually in the DOM.

I will add the special cases we need to test for in the execution plan above.

@Slapbox

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2018

@thesunny can you give an example of a situation where onCompositionEnd doesn't fire?

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

@brynshanahan Thank you. That's an interesting idea. getSnapshotBeforeUpdate does appear to execute at an important time (i.e. immediately before the render in what appears to be a synchronous manner). I'm not clear yet on how to use this to fix the issue but it does open up some more avenues to explore and experiment with.

I wonder if we can tie our DomSnapshot to the getSnapshotBeforeUpdate so that we can fix the DOM immediately before render and whether that would help or not. I think I might need to figure out what order the DOM manipulations are happening and from where (i.e. when React's render vs Android's keypress manipulations happen). MutationObserver might help here.

@brainkim

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

@thesunny Have you considered simply setting the contenteditable property to false in getSnapshotBeforeUpdate and setting it to true in componentDidUpdate? Does this break android?

@brynshanahan

This comment has been minimized.

Copy link

commented May 9, 2019

Yeah figuring out the edit order should be really helpful @thesunny. Also here are some bits of info I found this morning that could possibly help us.

Info on getSnapshotBeforeUpdate
Info on render/reconciliation timing. Also a really interesting article about React in depth from Dan Abromov's blog.

Consistency
Even if we want to split the reconciliation process itself into non-blocking chunks of work, we should still perform the actual host tree operations in a single synchronous swoop. This way we can ensure that the user doesn’t see a half-updated UI, and that the browser doesn’t perform unnecessary layout and style recalculation for intermediate states that the user shouldn’t see.
This is why React splits all work into the “render phase” and the “commit phase”. Render phase is when React calls your components and performs reconciliation. It is safe to interrupt and in the future will be asynchronous. Commit phase is when React touches the host tree. It is always synchronous.

And I think this diagram might help with the discussion . Arrows are where breaking user edits could be happening
image

I'm not entirely confident but it sounds like the React Updates DOM and refs step is synchronous, and therefore maybe getSnapshotBeforeUpdate is synchronous as well. Which would cross out user edits breaking anything at point D (which is good because surely that would be the least recoverable point)

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

@brainkim Thank you for the suggestion. That's an interesting idea but at this time I don't think I'm willing to go there. The reason is that it opens up a whole set of unspecified/unknown behaviors. For example, do we lose the selection when we switch contenteditable to false? What happens if we switch in the middle of a repeating keydown (because compositionend events happen in the middle of repeating keydown events)?

The solution might be in there somewhere but at the moment, there are so many unknowns already that I'm reluctant to try and solve the current unknowns by adding some new ones.

@brynshanahan Thank you for the additional detail. I haven't spent much time on it but I haven't wrapped my head around using getSnapshotBeforeUpdate to create a solution yet.

The big issue revolves around timing of state changes. In particular, if we have to revert, we revert to a DOM Snapshot (DOM state) at a given point in time. Then the user does something (like "backspace") and then we have to create a new Slate Value (Editor State). So now we have an old DOM State and we have a new Editor State.

Then before the Editor State gets rendered to the DOM (getSnapshotBeforeUpdate to the rescue?) we revert the DOM State. But what if before the Editor State gets rendered, the user presses backspace again. Then we are reverting to a DOM state before the second backspace has been pressed. The initial revert might save us from an unrecoverable error, but now the DOM state doesn't reflect the second backspace because its been reverted out so how do we make sure we don't lose that second backspace. I'm not entirely sure the sequence of the actions here and there may be several depending on small timing issues.

So, long story short is that I don't yet have confidence on a way to proceed that will fix the issues. I have one other idea that might fix the timing issue with respect to key repeats; however, I still lack confidence that solving that is a band aid that won't prevent small timing errors from causing React to crash.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

Note: The ideal solution is to have React not panic and die if it can't find DOM nodes. Instead, it should just rebuild the DOM. But I don't know that the React team would care to do this for the very specific use case of a "Rich Text Editor on Android".

@brynshanahan

This comment has been minimized.

Copy link

commented May 10, 2019

Hey @thesunny this may remove any need for messing with React lifecycles. facebook/draft-js@cda13cb
Also draft just merged their android fix into master 9 hours ago, so it might be worth looking for some inspiration there.
https://github.com/facebook/draft-js/blob/master/src/component/base/DraftEditor.react.js

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2019

@brynshanahan Slacked you on this but just wanted to say the find in there about ReactDOM.flushSync and friends is amazing!

Not documented but seems like that has the potential to solve the timing issues. While getSnaphotBeforeUpdate might have solved it, it seemed needlessly complex (maybe even unsurmountable) if events snuck in between setState and render and since React is aiming on async going into the future, I was worried that this was something that I could never resolve without React features which appear to have already been built. Of course, you had to be Facebook to know about them (or know React internals which I don't).

@robbertbrak

This comment has been minimized.

Copy link

commented May 10, 2019

@thesunny:

The ideal solution is to have React not panic and die if it can't find DOM nodes. Instead, it should just rebuild the DOM.

Isn't that what error boundaries are for? In our DraftJS-based editor we have accepted as a sad fact of life, that the DOM sometimes gets out of sync and causes React to panic. Our error boundary catches such errors and resets the editor to the last known state to recover.

Not ideal, but much better than getting stuck in an error state and ending up with an unusable editor.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

@robbertbrak Error boundaries would save the rest of the React app from crashing but it wouldn't prevent the Editor component from crashing unfortunately.

edit: Sorry, I didn't read that properly. I see what you are saying. That sounds like a useful fallback. If everything dies, then re-render using the last known data.

General Update

Lots of thanks to @brynshanahan for pointing me to the recent (as in a few days ago) update to DraftJS. There are two really important techniques that I was unaware of in React and that I haven't previously seen documented. I think these two will greatly simplify the code. They are:

  • The ability to flush synchronously if required using ReacDOM.flushSync and friends.
  • The ability to render the DOM without worrying about broken DOM state. Interestingly what they do here is they just set the key to the editor's Content area to a new value. Under the hood I'm assuming that if the key changes then the entire Element is blown away so even if there are inconsistencies, we don't have to worry about them. The negative is that you are completely rebuilding the DOM which is a definite performance degradation, especially on bigger documents; however, you get back the consistency of not crashing your Editor if the DOM is out of sync. I think that we should keep the DOMSnapshot object I built (should be much faster) but use the DraftJS method for now. Then we can introduce DOMSnapshot back slowly working out all of the edge cases and building the performance up.

Finally, it seems like ProseMirror and Draft have basically gone all in on MutationObserver over doing event analysis. DraftJS having adopted much of the ProseMirror code. The benefit, long run, seems to be that we should be able to build something on MutationObserver that can work in more than just Android which I think @ianstormtaylor would be much happier about instead of the Android only approach we've had to take when analyzing events.

Sadly, for me, this means abandoning a lot of the work I've done. Happily, these discoveries, especially with respect to React, means that Android is possible with React.

@Slapbox

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Sadly, for me, this means abandoning a lot of the work I've done.

It always hurts to throw away hard work, but the lessons learned make the new code that much better.

I think MutationObserver is a great idea. My only real reservation about this method was the idea that they should be a solution of last resort. Seeing other editors going that route makes me think that's the best way to go for sure. Besides them providing proof that the method works, Slate will be able to poach useful related code from two other well maintained projects. In the long run that latter benefit will probably prove enormous.

@adjourn

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

@thesunny

Haven't looked into Android at all, I have a question: could combination of (Android) events contaminate whole document? Can't you use "blow DOM away with key" strategy in block or even smaller scope?

Even if we have to re-render whole doc.. I'd take worse performance on Android over not working any day.

@signalwerk

This comment has been minimized.

Copy link

commented May 15, 2019

@thesunny Thank you for keep working on that issue and for the constant updates!

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

@adjourn it feels like "blow DOM away with key" strategy should work in smaller scopes although at that granularity level, we have to worry about the edge cases so would still need the whole document version.

A few issues I can think of are

  • a backspace that merges into the block prior and
  • an enter which splits a block into the existing block and a sibling block.
@thesunny

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

@robbertbrak Hey, I just wanted to say that the more I think about it, I really like the idea you posted about using error boundaries to recover the Editor from a React crash. Thank you so much for posting this.

@ianstormtaylor

This comment has been minimized.

Copy link
Owner Author

commented May 18, 2019

Unsure of what editors are doing for the race condition. But have we considered queueing operations while the editor is inside a composition? That way during the entire duration of a composition the editor will not apply changes, until the composition ends after which all the queued changes will be applied.

@Slapbox

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

@ianstormtaylor, @thesunny can speak to this better, but there are some edge cases this might leave behind. There might be more common issues I'm not aware of.

The edge case that occurs to me is if a user enters a word, but does not press space, enter, or enter some punctuation character, the composition event will not end. If a user goes to enter a word in the middle of an existing sentence, and then changes the editor's selection seems the most likely real world presentation of this edge case.

That solution does sound elegant to me though if we can do things that way.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2019

@ianstormtaylor FYI, I haven't been doing operations until compositions have ended for a while now. Those compositions themselves work pretty smoothly and have been for some time.

The timing issue was related to backspace. When you hit backspace a second time at just the right moment, we seem to interrupt the React reconciliation and get weird issues. I think sync solves this though.

Although isolated compositions work well, I'm working on improving the boundary of compositions and non-composition events within a single tick (i.e. within a single requestAnimationFrame).

There is actually a rather larger roster of issues to deal with but I am getting closer. Every iteration I remove more edge cases and decrease complexity.

I think the error boundary to save Slate from crashing in an unrecoverable way is huge. That's already been merged and fixes previously fatal errors.

@Slapbox I think you are referring to compositions not ending and carrying over to another node. Yes, that happens sometime and requires us to keep track of all nodes that are part of the composition where previously I only tracked the last node. I might try to actually force the compositionEnd instead when the selection changes to a new node to simplify the code. I think ProseMirror does this as well.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented May 25, 2019

A short update is that I have made progress using mutations. I also bought a Google Pixel 3a which makes working on Android so much more pleasant!

Currently, the main Editor behaviors like compositions, hitting enter to splitBlock and hitting backspace to merge blocks and to delete entire elements (like bold text) all basically work. There are still issues I'm working through like restoring the DOM causing the mutations observer to disconnect and holding down backspace continuously causing timing issues.

Note: I actually had a version of the editor where it does all edits using mutations on desktop Chrome which speaks to the flexibility of using mutations. We may wish to consider using mutations for everything which could simplify code in the future. Right now on desktop we use events like keydown or splitBlock. By tying into commands (like insertText and splitBlock) rather than events for plugins we could potentially settle on a unifying codebase for regular edits (one character at a time) and composition edits (a word at a time).

WRT, the timing issues which I think are related to all the code in Slate and React that work in an async nature, I'm trying one by one to remove those cases. The good news is that while there are bugs nothing seems to crash the Editor anymore. Sometimes you get the Value out of sync with the DOM however and some weird behavior.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2019

Okay, I have a stable-ish version for Android 9 (edit: and 8.1) using mutations. Please test it out here here:

https://thesunny.github.io/slate/#/composition/split-join

And submit bugs here:

#2839

NOTES:

  • Holding down backspace does not work well (pressing it one at a time does and so do selection deletes)
  • Only tested on Android 9 (edit: also testing on 8.1 and works)
  • Passes all the manual tests but I'll likely have to add more
@thesunny

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2019

Okay, good news everyone!

I've run all my basic tests on Android 8.1 and everything works with no changes!!! This is absolutely amazing! Seriously, adding another version of Android was previously just as hard as adding the first one. The fact that it just works is boggling my mind. I actually had to go check to see if I was on the right device because the fact that it worked didn't make sense to me.

edit: Forgot to run the enter test. There was a few bugs there but all the other tests work.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2019

Android 8.1 including the enter tests are running now at https://thesunny.github.io/slate/#/composition/split-join

@robbertbrak

This comment has been minimized.

Copy link

commented May 30, 2019

@thesunny: Great to hear that using MutationObserver is working out so well! 👍 With Draft (which now uses MutationObserver for all composition-based input, not just on Android) I have also seen good results.

Just one question though: when you say that "everything works" on Android 8.1 / 9, what exactly do you mean? In my experience, whether composition-based input works on Android is not just limited to the API version, but also varies depending on:

  • Keyboard (Gboard, Huawei Swype, Google Voice Typing, Samsung keyboard, ...)
  • Language (Chinese, Korean, Japanese, Arabic, ...)
  • Browser (Chrome, Firefox, ...)
  • Keyboard variant and settings (Correct words while typing yes/no, Pinyin / Handwriting, ...)
@thesunny

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2019

To clarify I only meant that all my manual tests pass on Android Chrome on Gboard with the exception of continuous backspace. I’m sure there are more bugs which is why I’m asking for bug reports.

My tests do include gesture writing, foreign IME but not some of the others; however if they work as compositions then they theoretically should work. I did nothing extra for IME or gestures for example but they both work.

The initial goal is to get the most common use case for Android at an MVP (mostly works and no unrecoverable crashes) then build on there.

I could see a switch to using only MutationObserver being a path for Slate and I even had Desktop Chrome working on MutationObserver while developing the Android version; however that’s probably a decision ultimately for Ian.

@thesunny

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

Made the PR for Android support using mutations here

#2853

@Macil

This comment has been minimized.

Copy link

commented Jul 29, 2019

I've reported two bugs affecting Android Firefox and Chrome: #2951 and #2952. I figure they're related to this issue but that it might be good to make issues for each of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.