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 Firefox and Chrome: Swipe typing is aborted very often #2952

Closed
Macil opened this issue Jul 29, 2019 · 9 comments
Closed

Android Firefox and Chrome: Swipe typing is aborted very often #2952

Macil opened this issue Jul 29, 2019 · 9 comments
Assignees

Comments

@Macil
Copy link

Macil commented Jul 29, 2019

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

bug

What's the current behavior?

On the Slate JS front page rich text demo (https://www.slatejs.org/#/rich-text), position the cursor at the end of the first line (after the "!"). Type "the quick brown fox jumps over the lazy dog", only by swiping each word.

On Firefox Android (version 68), the result is " The quickbrown jumps overthe dog". When "fox" and "lazy" are swiped, the swipe is aborted partway through (the blue line left by your finger while swiping disappears while swiping the word!) and the word is dropped entirely. The words "brown" and "the" don't get leading spaces, so they're combined with the previous word. It seems like there's a repeating pattern: two words work fine, followed by one word that doesn't get a leading space, followed by one word dropped.

On Chrome for Android (version 75.0.3770.143), the result is " The brown jumps the dog". Every second word is aborted partway through swiping, similar to how it happened on Firefox.

Testing on Android 9 on a Pixel 3.

@Nantris
Copy link
Contributor

Nantris commented Sep 15, 2019

It's hard to reference the input tester on mobile. I wonder if maybe debugging this is easier on a tablet?)

I think this occurs because the onCompositionEnd event doesn't fire when you begin a second. I think this comes down to a timing bug that can be seen elsewhere if you try to select words via longpress, sometimes it loses the selection and reverts to compose mode.

I'm thinking this because of the following order of events:

  1. Swipe in a word
  2. Begin to swipe a second word (the swipe is cancelled)
  3. Now press a key and it continues the original composition

Similarly if you type a word key by key and then quickly start tapping the auto-text option will insert it but the newly inserted word will enter the document as if it's being composed (it has the underline under it, and the vibrate that normally accompanies word insertion is oddly cut off during this.


@thesunny I'm finally approaching a time where I can start to turn more towards the internals of Slate that you've done so much great work on. If you can give me your thoughts on what the cause might be I'd love to take a look so my subconscious can hopefully come up with something by a couple weeks from now when I'll have some time. I tested out Draft and it doesn't seem to have this issue. I know we're using mutation observers which I think we turned to after seeing the success they had with that approach, if I'm not mistaken, so I'm hopeful we can find some clues in their implementation.

Thanks again for all your hard work bud! Hope I can help build on it! I'll hopefully be on Slack more starting later this week if you want to discuss in real-time.

PS: I recall Slate and/or Slate-React having before and after plugins, and Matt Krick mentioning we'd need to rewrite things to abandon that concept. I'm not sure if that's the case, but it seems those concepts are removed based on a quick look around the 0.47 codebase. Am I right about that? I've been on 0.45 for a while so I've got some catching up to do on the internals.

@thesunny
Copy link
Collaborator

You can enable useful debugging information by setting your localStorage.debug to slate:events,slate:batch-events,slate:mutations. You can do this by opening up the Chrome devtools under the application tab and add a debug key with the property as specified earlier. You can also choose a subset of them (whatever is useful to diagnose the current issue).

Everything for android is in one plugin you can find here https://github.com/ianstormtaylor/slate/tree/master/packages/slate-react/src/plugins/android

There are support files in the same directory.

There are timing issues and I'm pretty sure right now things aren't happening synchronously even though they should. The flushControlled code here https://github.com/ianstormtaylor/slate/blob/master/packages/slate-react/src/plugins/android/composition-manager.js doesn't seem to be doing it's job but maybe there is a way to make it work better.

If I remember correctly, the basics of the plugin is that we wait for a composition to start and do nothing until it finishes. When it finishes, we inspect the DOM for changes and then run the commands in Slate. Certain changes like enter and backspace, especially at certain boundaries, cause a flurry of DOM mutations. We make an educated guess based on what the mutations are doing in aggregate that translates them into commands like splitBlock or deleteBackward.

There is also additional documentation here https://github.com/ianstormtaylor/slate/blob/master/packages/slate-react/src/plugins/android/composition-manager.md

@Nantris
Copy link
Contributor

Nantris commented Sep 18, 2019

That's all extremely helpful! I'm excited to see what I can find now that I've got some leads.

I've only gotten to take the briefest look so far. I'll take a closer look at flushControlled to see if that's the source of any timing issues. I wonder though, if that's not the issue, could the solution to some timing issues be abandoning requestAnimationFrame in favor of a seemingly sub-optimal approach using setTimeouts of shorter durations? Ignoring the obvious downsides of using setTimeout when requestAnimationFrame would generally be more appropriate, is this a possible avenue to explore? Or are we locked into specifically using requestAnimationFrame for reasons beyond simply best practices? Just spit-balling since we suspect some timing/racing issues.


Regarding swiping specifically, I've noticed that the behavior is seemingly 100% consistent no matter how long you wait, word length, etc. Combined with what I found previously, I'm thinking the cause is that we have no way to detect the end of one swipe event and the start of another, so the composition stays open.

I can't think of any time this would be valid under normal circumstances, so my suspicion is that the keyboard recognizes that beginning a composition while you're already mid-composition is invalid and cancels the user's request. After cancelling, the keyboard re-focuses on the still open composition. This manifests as a loss of the originally suggested alternatives.

The originally suggested alternatives were specifically tailored to the unique imprecisions of that particular swipe event, but are lost under this circumstance in favor of the generic composition suggestions based only on the word and its context, with the original swipe pattern having no relevance to the new suggestions. This is what happens when you revisit a word you previously swiped after losing focus on it. Is what I wrote clear? If so, thoughts on this possibility?

I guess, in short, do we think this is a problem of us not doing something (eg, closing the composition) or is this because we are doing something (eg, updating the underlying editor value)


By the way, the Composition Manager doc reminds me, has anyone had a chance to try Android 10 with Slate yet? I hope Google didn't screw us.

Thanks again for taking the time @thesunny!

@Macil
Copy link
Author

Macil commented Sep 25, 2019

Android 10 test results:

  • Firefox 68.1.1: " The quick" gets typed normally, but after "quick" shows up, the cursor disappears and the keyboard auto-switches to upper-case as if a sentence had been completed. The page acts like the cursor is at the top of the textbox, and so bug Firefox Android: All keypresses at start of line are treated as Enter #2951 starts expressing itself: all keypresses get interpreted as newlines until the cursor is placed somewhere other than the beginning of a line.
  • Chrome 77.0.3865.92: No change from my original results with Chrome.

@thesunny
Copy link
Collaborator

It's worth experimenting with timing; however, at the same time, be careful about adjusting the times with requestAnimationFrame and setTimeout. Too short, and the browser doesn't have time to complete its DOM manipulations. Too long and the user can start a second action. If you find better timing, make sure to run through all the android tests in the examples at a minimum after making a timing adjustment in case it broke something else.

This is not ideal and it's one of the reasons why the recovery mechanism of a broken DOM is important (i.e. we redraw the content on a Slate crash).

Some things that you may also want to pay attention to:

  • The exact state of the DOM after the end of a swipe
  • Compare it to the exact state of value.document after the end of a swipe
  • Look for zero width spaces that might be affecting things
  • Look at the composition manager to see if things are being interpreted correctly
  • Look at the order of compositions and other events to get hints that something is happening out of order

Also if it is indeed a composition that is not being finished, try and look at the mutations and events to see if there is a way to figure out that a composition has completed at the end of a swipe. You may have to look at combinations of mutations and events.

@gracicot
Copy link
Contributor

It seem that the swipe aborting is connected to restoreDOM calls when a composition ends. Sadly, simply removing it causes all sorts of strange bug of text reappearing and other strangeness. Also, removing the restoreDOM really made editing feel more responsive. I'll try to reseach on that.

@thesunny
Copy link
Collaborator

@gracicot To provide some more details and ideas for future, there are some improvements we can performance-wise for most uses of restoreDOM. At the moment, we use restoreDOM to do a full rebuild of the DOM. It basically re-renders everything from scratch which is why it's not responsive, especially on larger documents.

I have code which I think is in Slate now but is not used that will rebuild only two root level blocks (the current one and the one before). The way it does this is that it creates a mirror of the DOM in memory using arrays and objects. This mirror stores the positions and text contents of the DOM. When we need to restore the DOM, we run this method and it moves the DOM elements around to match the mirror and fixes the text inside it where it has differed.

I didn't include this in the initial release because restoreDOM is safer. It is pretty much guaranteed to work because it blows everything away and then rebuilds it. It follows the rule of simple, correct, fast: in that order. My goal was to get to correct in this version.

@gracicot
Copy link
Contributor

gracicot commented Oct 21, 2019

@thesunny Only restoring mininum amount of nodes would be awesome. It may fix some slugginess I observed when editing larger texts.

It would require testing, but from previous experiment (when the whole document was re-rendered), the keyboard lost context from previously typed words. That context loss prevent some operation that span multiple composition. One simple example would be pressing and holding down the backspace key to delete many characters and autocorrect on previous words (android and iOS can apply autocorrect on the word before the composed one). Swipe typing may also be included since a swipe usually starts before the previous composition ends.

Right now I'm investiguating on a way to avoid calls to restoreDOM at the end of a composition, but from what I see that would require more advanced matching of the mutation stream. Instead of trying to match an observed mutation buffer that wasn't correctly handled, I'll try to make a more generic one, that could keep track more closely what is happening in the DOM and how it would translate to Slate commands. This should help this issue as swipes won't be aborted if there is no re-render of the DOM at moments that conceptually shouldn't be required.

@ianstormtaylor
Copy link
Owner

Hey, there are lots of open Android-related issues that are out of date with the latest 0.50 release because the codebase was completely rearchitected. We'll need to figure out how to have proper support for Android going forward based on beforeinput events somehow. I'm tracking this in #3112, so I'm going to close this in favor of that one.

If it cannot be implemented simply in core with beforeinput we'll likely need to have a separate plugin for android support be created, because it's too much of a departure from all of the other simpler logic in core and very hard for me to test or respond to issues. Using a separate plugin should be possible now as the newest version simplifies a lot of the internals.

Thank you for understanding.

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

No branches or pull requests

5 participants