search

ianstormtaylor / slate

A completely customizable framework for building rich text editors. (Currently in beta.)
http://slatejs.org
MIT License
29.9k stars 3.25k forks source link

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

Closed ianstormtaylor closed 4 years ago

ianstormtaylor commented 6 years ago

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...

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:

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...

This is actually the same starting steps as is required for https://github.com/ianstormtaylor/slate/issues/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.

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.

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.

thesunny commented 5 years ago

I merged the PR into master after waiting for any more comments.

Next step is to wait for @ianstormtaylor to either release it or ask for changes.

Nantris commented 5 years ago

This is an exciting moment! @thesunny - amazing work!

rbar2 commented 5 years ago

@thesunny, this is amazing! Very appreciative for this update🙌

thesunny commented 5 years ago

FYI, for those following, this has been unmerged and awaiting merge approval from @ianstormtaylor from this new PR here:

https://github.com/ianstormtaylor/slate/pull/2565

A lot of discussion, questions and updates on the old PR here though:

https://github.com/ianstormtaylor/slate/pull/2553

So far resolved all the issues except for one; however, more issues are likely to come up as Ian noted that it's a big PR and will take some time to get through.

thesunny commented 5 years ago

@ianstormtaylor

I can start work on the Combo library which should make the Android composition code easier to follow. The existing code is a little spaghetti-ish because it handles combination events over multiple event handlers with state mixed in. The Combo would move all that into one object.

It was originally my plan to begin work on this after the current PR was handled since it would be a refactoring of this code that already works. Would it be better for you if I add the Combo to the current PR or if I wait until the current PR is worked through?

ianstormtaylor commented 5 years ago

Hey @thesunny, it sounds like waiting may be best. So we don’t get it too much more complex first. I’m not entirely sure how the combo concept will work.

thesunny commented 5 years ago

Okay, thanks. Let me know if there's anything I can do to help with the PR.

thesunny commented 5 years ago

Hurray! Android support was published to NPM four days ago.

Nantris commented 5 years ago

@thesunny can you outline the next steps on this task? I know we're 95% of the way there, but you mentioned needing to address "combos" - I'm not sure what this was in reference, what it would do, or how it would work.

I know you've already done a stupid amount, but if you could briefly outline what remains to be addressed that would be a great jumping off point for completion of the remaining tasks. I know I need to file a few proper issues reports now for some minor bugs I mentioned in comments somewhere. I haven't been able to find them yet, but I'll get on that asap so we can wrap up the few remaining areas where mobile support lags behind desktop support.

saravanan10393 commented 5 years ago

@thesunny ,Are examples hosted in slate website is mobile supported now ?. So that i can check and i will migrate my code from older version to newer version, which is huge effort me since i have blendy of plugins to convert to newer version.

thesunny commented 5 years ago

@saravanan10393 The examples on the official Slate website should have Android support on it now although I haven't checked and aren't able to right now (I'm on vacation and I don't have any Android devices on me).

@Slapbox I was waiting on getting Android support merged before I did any more work on it. As mentioned, on vacation now, but after I get back and I finish some integration work, I'll probably begin work on the Combo. I posted some source code and progress on the Combo issue https://github.com/ianstormtaylor/slate/issues/2547

nonsleepr commented 5 years ago

Sadly, just moving the cursor in code highlighting example on https://slatejs.org using Chrome on Android messes up the code.

thesunny commented 5 years ago

@nonsleepr please be more specific. Which Android version, what did you do, touch move, keyboard? Also we only have Last 2 major Android versions right now. Sounds like you might be on an earlier version

nonsleepr commented 5 years ago

I'm on Android 9, Chrome 72. Tried again with different virtual keyboards. It looks like only Goggle's keyboard mess the code. With GBoard or Google Voice Typing when I tap in different places the code disappears. Also tried Hacker Keyboard and Grammarly. They work fine. GBoard underscores the text under cursor and suggests alternatives. I guess that reformatting is what causes the problem.

glebtv commented 5 years ago

For me it more or less works here https://www.slatejs.org/#/rich-text but if you delete all the content in this demo, you can't add any content back (editor is completely broken if empty)

thesunny commented 5 years ago

Hi guys, just checking in. Reading the comments but I’m on vacation right now. Will follow up after I’m back.

thesunny commented 5 years ago

@ianstormtaylor This is obviously a long issue now (168 comments) and we do have, shall we say, tentative support for Android.

I feel like it might be the time to close this issue and create a new one focused on fixing Android bugs which I'll use to clean up the majority of the remaining issues. After that, hopefully when most bugs are handled, we could file individual bug reports like the rest of Slate.

I'm still working on a major upgrade in the app I'm using Slate which includes updating to the latest Slate version; however, after that, my plan is to work on adding the Combo feature and fixing more bugs.

glebtv commented 5 years ago

For me, Slate is still unusable on both android 8 (Huawei mediapad m5) and 9 (Oneplus 5t), in Chrome. :( Both in rich text demo and in my app. It would be nice to have an issue with known problems or with check boxes what works and what doesn't..

Pre-populated text from rich-text demo is editable more or less, but text added from android is broken.

thesunny commented 5 years ago

@glebtv

Thanks for the feedback. I'll be working on getting Slate working better soon (probably starting this month).

What would be helpful in the mean-time are specific steps on specific platforms, preferably with a recorded screen. Unfortunately, the issues present themselves on Android in sometimes very specific ways related to order, timing, position in text, etc. Once I can replicate it reliably though I can usually fix it.

My goal is to build a manual test suite into the examples which you can see the beginnings of in the example labelled "Composition." You'll find some issues that definitely fail if we don't have special support for them. Ideally, we want people to be able to go through the manual test suite on their own platform to ensure that we've caught all the edge cases.

hecor commented 5 years ago

Hi @thesunny, Thanks for the great work. I still have some problem on Andriod phone, please give some help. #2702

thesunny commented 5 years ago

I'm working on fixing the remaining Android bugs now.

In order to be more efficient, please submit bugs and help with testing over in this issue:

Fix all showstopper bugs in Slate on Android (Help us by reporting and testing bugs) https://github.com/ianstormtaylor/slate/issues/2726

Nantris commented 5 years ago

@thesunny there's a number of major bugs and some minor ones too (at least in the current repo, I know you've linked to your fork which may be better,) but only a one or two bugs I would define as real showstoppers that actually make things crash or truly unusable.

Could you clarify where you want to set the bar for this pass of bug fixes? I know the cut-offs for each classification are nebulous, but I assume your aim might be more accurately described as fixing major and showstopper bugs, the distinction in my mind being that major bugs can be worked around with great effort, and critical/showstopper bugs cannot.

I just don't want to clutter up your issue on an already complicated and hard to manage topic with things that are below the "worth fixing now" threshold, and I suspect I'll be responsible for a substantial amount of the coming bug reports.

To help make things less nebulous, I just grabbed the first set of definitions for different bug levels:

Critical / Show Stopper: An item that prevents further testing of the product or function under test can be classified as Critical Bug. No workaround is possible for such bugs. Examples of this include a missing menu option or security permission required to access a function under test.

Major / High: Defect that does not function as expected/designed or cause other functionality to fail to meet requirements can be classified as Major Bug. The workaround can be provided for such bugs. Examples of this include inaccurate calculations; the wrong field being updated, etc.

Average / Medium: The defects which do not conform to standards and conventions can be classified as Medium Bugs. Easy workarounds exists to achieve functionality objectives. Examples include matching visual and text links which lead to different end points.

Minor / Low: Minor priority is most often used for cosmetic issues that don’t inhibit the functionality or main purpose of the project, such as correction of typos in code comments or whitespace issues.

Source

thesunny commented 5 years ago

@Slapbox That's a good question; however, I think it's okay for you to post all of them and I can make a determination what makes it in. It's probably too much to ask the bug reporters to do the analysis since some of the lower priority ones might be easy fixes so I might as well do them now.

I'll probably only not do the ones that are simultaneously (a) hard to fix and (b) not that important.

thesunny commented 5 years ago

I'm posting here instead of the Android bugs thread https://github.com/ianstormtaylor/slate/issues/2726 in order to reach the most developers.

I've come to an issue that think may be impossible to workaround. My hope is by explaining it, somebody might be able to find a solution or offer ideas that might help me get passed the issue. I'm happy to explain any more details or answer any questions.

You may wonder, since other editors have Android support, why it would be impossible with Slate? The main issue is that we cannot control when React renders to the DOM. We can push a change to our value and eventually that will flow through to the DOM, but we cannot force the render to happen synchronously.

This normally isn't a problem in Slate because we prevent an event (like an enter keypress) from happening, and then replace it with our own DOM changes. We are the only ones making changes to the DOM.

In Android, there are many events we cannot prevent. We have to wait for them to finish modifying the DOM. We then resolve the changes into Slate's value and React re-renders the DOM.

The problem happens when React re-renders the DOM at the same time as Android is modifying the DOM. This will happen when a user presses a key at the same time the re-render is happening. For example, it can happen when holding down the backspace key.

Here's a specific use case: Some events like backspace execute unintuitively. For example, if you were to delete the last letter in chicken, instead of an input:deleteContentBackward we get a input:insertCompositionText with data of chicke (without the n). This tells us Android is probably overwriting the word with a new word with the remaining characters.

While React is trying to reconcile the DOM against its virtual DOM, the user presses another key, and we end up with chickechick. I'm not sure whether React is adding chicke back in before Android's new chick insertion or visa versa.

I have found ways to reduce the likelihood of this happening and some double characters is very annoying but maybe worth the compromise. However, that's not what I'm worried about. The fact that there are conflicting DOM changes introduces a dangerous amount of complexity that could result in the editor crashing.

If Android deletes an element (which it will do when backspacing) or inserts a new element (which it will do when hitting enter), we must put the DOM back into a state before Android deleted or added the element. I've already built a library for this and it works.

If we don't do this, React tries to render into a DOM tree that does not match it's virtual DOM. This causes a crash.

However, if there are timing issues where React and Android are both trying to change the DOM at the same time, we can get into a bad state easily and, most problematic, it is not easy to replicate and to fix. I am finding that at times I can pass all my manual tests but just sometimes the editor gets into a bad state and I can't replicate it to fix it.

Eventually I found this dual edit issue but I can't think of a solution short of making changes to React itself which I have no experience with and likely would be difficult to get my changes accepted to.

brynshanahan commented 5 years ago

Hey, @thesunny I'm very new here, but I'm just going to take a stab. Would the ability to check the DOM before and after ReactDOM reconciliations help? In my understanding what your saying is that things get funky when a user makes an edit between a React render and a ReactDOM reconciliation. I was thinking maybe it would be possible to compare the intended and actual DOM with the getSnapshotBeforeUpdate and componentDidUpdate methods

thesunny commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

Hey @thesunny this may remove any need for messing with React lifecycles. https://github.com/facebook/draft-js/commit/cda13cb8ff9c896cdb9ff832d1edeaa470d3b871 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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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:

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.

Nantris commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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

thesunny commented 5 years ago

@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

thesunny commented 5 years ago

@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 commented 5 years ago

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.

Nantris commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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:

https://github.com/ianstormtaylor/slate/issues/2839

NOTES:

thesunny commented 5 years ago

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 commented 5 years ago

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

robbertbrak commented 5 years ago

@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:

thesunny commented 5 years ago

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 commented 5 years ago

Made the PR for Android support using mutations here

https://github.com/ianstormtaylor/slate/pull/2853

Macil commented 5 years ago

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