ianstormtaylor / slate

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

Android Predictive typing broken - Firefox #5130

Open KatTurini opened 1 year ago

KatTurini commented 1 year ago

Description Predictive typing on android - firefox is broken, inserting duplicates of entered letter sequences and losing focus, as visible in the recorded example of the slatejs.org/examples/richtext editor.

Apart from that, in my implementation of the editor (web app to be viewed on mobiles, too), when typing in Firefox (android), first word entered gets reversed, as the focus keeps jumping to the start of the editor - instead of 'Hello', the editor ends up with 'OLleh', plus an error message - 'type error - parentNode is null' pops up sometimes, as the focus keeps jumping to the start of the editor (video 2).

Issue with duplicating letter sequences is present on other android browsers too (chrome, duck duck go, samsung internet) - instead of reply, you end up with r+re+rep+repl+reply -> rrerepreply. Issue is especially bad on older androids - current android, current chrome - issue present only occasionally, usually when typing fast. Issue disappears if predictive typing is switched off.

From my observations, on slate 0.80.0, duplicating did not happen, but the currently predictive text would either overlay the placeholder (decorator) until empty space gets entered, which inserts the text to the editor, and last word would not get submitted (unless separated by empty space). Versions of slate from 0.81 up don't have this problem, but the duplication of letter sequences occurs.

Recording Firefox current, android current:

https://user-images.githubusercontent.com/46718908/191484822-452184f4-b56c-4a57-9425-1c634311b237.mov

Firefox current, android current:

https://user-images.githubusercontent.com/46718908/191487596-e5eb8de8-50e5-4383-b87c-c082163c1339.mov

Steps To reproduce the behavior:

  1. Go to slatejs.org/examples/richtext using an android device, browser Firefox (can be reproduced on browserstack)
  2. Click on editor
  3. Start typing
  4. See error

Expectation Letters are not duplicated Focus not getting reset/lost Type error is handled

Environment

Context These problems are only present when predictive writing is enabled on android keyboards.

Nantris commented 1 year ago

You mentioned it doesn't occur in 0.80.x? Could it be related to: https://github.com/ianstormtaylor/slate/pull/4988?

@BitPhinix

KatTurini commented 1 year ago

Hi Slapbox ! That's correct, version 0.80.0, I didn't see issues with predictive text on androids. There were some bugs things with retaining focus, but writing, which is key, was fine.

The android commits fixed all the focus issues, (great work, by the way), but made Android- Firefox unusable with predictive typing enabled.

In fact, I've just discovered Chrome 106 update for Androids broke the predictive typing on Chrome android, too. (slate 0.82.1)

I am about to rollback to 0.80.0 hoping that still works.

BitPhinix commented 1 year ago

I didn't test firefox on android as it's a really, really niche group of users and the main use-case for us is inside an embedded webview which is guaranteed to be chrome.

That's correct, version 0.80.0, I didn't see issues with predictive text on androids.

Maybe not on firefox but definitely on chrome πŸ˜…

In fact, I've just discovered Chrome 106 update for Androids broke the predictive typing on Chrome android, too. (slate 0.82.1).

Hmm, I'll look into that next week. Quite busy this week but we haven't yet received user reports about this. Don't have my test devices with me atm to test.

KatTurini commented 1 year ago

I didn't test firefox on android as it's a really, really niche group of users and the main use-case for us is inside an embedded webview which is guaranteed to be chrome.

agreed, it's very few users, and from my reading, firefox historically tends to handle events in a niche way, as if there are not enough differences among androids already heh, but it's a requirement for me as it's considered one of the default options, and we are not going native anytime soon unfortunately :/ (plus, the 0.80.0 version worked on old androids, which seems to be more common among my users than I would have thought!).

Maybe not on firefox but definitely on chrome πŸ˜…

yes, focus & selection esp i had a lot of fun working out fixes for haha. so when i upgraded, i was very happy - so much improvement (and code i could get rid of) thanks to your work. but unfortunately, i have users who tend to be on older devices + some firefoxers and more obscure browsers, where enabled android predictions broke slate's new input handling.

Hmm, I'll look into that next week. Quite busy this week but we haven't yet received user reports about this. Don't have my test devices with me atm to test.

thank you very much ! truthfully, i didn't spend much time looking into this, i needed a quick solution. That's why it's also possible that my implementation is simply very heavy - there's multi-editor pages, validation & max length checking, tagging, mentions, formatting, accessibility considerations, normalising, yada yada πŸ˜… . anyways, i will have a play with it later, too, and update if i find anything.

thank you both for your help !

AdlerJS commented 1 year ago

Issue with duplicating letter sequences is present on other android browsers too (chrome, duck duck go, samsung internet) - instead of reply, you end up with r+re+rep+repl+reply -> rrerepreply. Issue is especially bad on older androids - current android, current chrome - issue present only occasionally, usually when typing fast. Issue disappears if predictive typing is switched off.

Just wanted to chime in on this thread and state that after upgrading from Slate 0.81.0 to the latest version to pickup the android changes I am now seeing this on a pixel 4a. When typing fast something like "working" it randomly will duplicate the text to "woworking" or "workingworkingworking". I will say Android has come a long way on this release as I was able to delete 100% of my Android specific code and this was the only issue blocking me from moving forward with this update

Note: This duplication happens with or without the predictive text / suggestion enabled on Android

EDIT: After looking at this more over the past week the source of the issue was actually the API call updating the react state and the reconciliation that took place after was causing duplications. After increase the debounce time to around 2 seconds Android is working great. Just wanted to update and thank @BitPhinix for the work he did on Android. It's by far the best experience we've had in 2 years of using slate on Android

ttkien commented 1 year ago

Issue with duplicating letter sequences is present on other android browsers too (chrome, duck duck go, samsung internet) - instead of reply, you end up with r+re+rep+repl+reply -> rrerepreply. Issue is especially bad on older androids - current android, current chrome - issue present only occasionally, usually when typing fast. Issue disappears if predictive typing is switched off.

Just wanted to chime in on this thread and state that after upgrading from Slate 0.81.0 to the latest version to pickup the android changes I am now seeing this on a pixel 4a. When typing fast something like "working" it randomly will duplicate the text to "woworking" or "workingworkingworking". I will say Android has come a long way on this release as I was able to delete 100% of my Android specific code and this was the only issue blocking me from moving forward with this update

Note: This duplication happens with or without the predictive text / suggestion enabled on Android

EDIT: After looking at this more over the past week the source of the issue was actually the API call updating the react state and the reconciliation that took place after was causing duplications. After increase the debounce time to around 2 seconds Android is working great. Just wanted to update and thank @BitPhinix for the work he did on Android. It's by far the best experience we've had in 2 years of using slate on Android

@danbunkr Could you explain how to increase debounce time to 2 seconds?

Nantris commented 1 year ago

@danbunkr can you possibly share some more about what you mean? That issue plagues us.

What exactly did you debounce? Was it your own code or was it some Slate internal code you patched?

Nantris commented 1 year ago

@BitPhinix would you happen to know what @danbunkr means should be debounced?

Nantris commented 1 year ago

Friendly bump. I wonder if anyone can explain @danbunkr's solution briefly? It would be a very valuable thing to add to the documentation. If someone can help me understand it I'd happily submit a PR with the info.

PibeG commented 1 year ago

I've been trying to find a issue report for this. Not sure if this would be related to #5071 https://github.com/ianstormtaylor/slate/pull/5071#issuecomment-1205129784

It's invoked with a "safe" timing after each composition change, so you could modify the editor state inside it without interfering too much with the IME (I say too much because it still will if the render takes long/you type very fast). Forcing a flush manually by returning true will have these issues as well, so you should use it very sparingly and only where you have no other choice (like with markdown shortcuts).

an example of the debounce would be helpful, my best guess would be onChange, but confirmation would be greatly appreciated @danbunkr @BitPhinix

AdlerJS commented 1 year ago

Hey Everyone : @Slapbox @PibeG @ttkien

Honestly came back here on accident for a slightly different issue and was surprised to see all the missed mentions. I can paste parts of our code below the explains the debouce issue and hopes that helps.

First we have a controlled Slate component in react:

      <Slate editor={editor} value={initialValue} onChange={onChange}>

and a pretty basic onChange function that simply calls an API to save the data on the server:

   const onChange = useCallback(
    (updatedContent) => {
      // Prevent update for cursor clicks
      if (isEqual(updatedContent, initialValue)) return;

      setLastUpdated(Date.now());

      syncNote({
        ...note,
        content: serialize(updatedContent),
      });
    },
    [initialValue, note, syncNote]
  );

And then the debounce that just calls the api with the two second value:

    const onlineDebounceTime = isAndroid ? 2000 : 500;
  // Updating the delay so that useCallback will re-calculate when offline state changes
  const syncNote = useDebounce(
    isOnline ? updateNote : onNoteChange,
    isOnline ? onlineDebounceTime : 499
  );

Not sure if that answers your questions but let me know. I'll make sure to check GH notifications more often

elatif2020 commented 8 months ago

similar issue here

https://github.com/ianstormtaylor/slate/assets/72435714/c936458f-8e90-4d27-bbfd-3b1ad2a6a5b3