ianstormtaylor / slate

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

Selecting text and moving the cursor is completely broken on Android #3470

Open RudeySH opened 4 years ago

RudeySH commented 4 years ago

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

Bug.

What's the current behavior?

Selecting text on Android automatically copy-pastes some text into the cursor position (to be more specific, the first position the cursor was at after focusing the editor).

Here's a GIF.

I'm not really sure why text is being inserted, how it decides what text to insert, or even at what location the text gets inserted.

I can reproduce the issue using the official examples: https://www.slatejs.org/examples/richtext

Slate: Whatever version is used at https://www.slatejs.org/examples/richtext Browser: Chrome 79.0.3945.136 OS: Android

What's the expected behavior?

Slate should just allow me to move the cursor and select text like normal.


I noticed there have been a bunch of issues about mobile/Android issues, but they were all closed/merged.

Coincidentally, I noticed very similar behavior with the CodeMirror editor. The issue seems to be gone in CodeMirror 6.

RudeySH commented 4 years ago

I noticed Discord uses a fork of slate. I'm not sure what the differences between an Android app and Chrome for Android would be, but their editor works fine on Android.

Scrolling through their commits, I found commits like these: https://github.com/discordapp/slate/commit/a1f64f5d8d553bca3bea6cf1c7aca0d0f08397e7 I'm not sure if this commit specifically fixes the issue at hand, but it shows that there are definitely some things that need fixing.

thesunny commented 4 years ago

FYI, current versions of Slate do not support Android. You have to go back to the 0.47x version to get Android back.

baljeet-aulakh commented 4 years ago

FYI, current versions of Slate do not support Android. You have to go back to the 0.47x version to get Android back.

Hi Sunny, I am using slate 47 and it's working perfectly on desktop and iOS devices but onChange is not triggering on Android devices. I would greatly appreciate any suggestion. Thanks.

ajorkowski commented 4 years ago

Hi,

To anyone that is having issues with Android... This is my attempt to fix the issue in slate 58+ and so far it seems to working pretty well.

The problem with the composition in Android is that the insertCompositionText before input event fires, but the insertFromComposition before input event does not. This really breaks slate because it relies on this event to be able to replace the text after a composition. Chrome correctly fires the onCompositionEnd event, but the problem is that this event does not have a selection associated with it - you might have edited multiple pieces of text etc and clicked around before this eventually fires.

Another problem on Android is that when text is completely removed from a line the slate text element can get completely removed and replaced with a <br /> tag. So I had to fix this as well.

The essential idea with this fix is that we store the insertCompositionText selections and values and elements etc, and then when we do eventually hit the onCompositionEvent we re-run these transformations on our slate data, so that we align the 'virtual' data with what is actually in the dom. We also fix up any element that has turned into a <br /> node.

Here is the code in any case (sorry I don't use typescript - this is es6):

const editor = useEditor();
const lastComposeData = useRef(null);

const onDOMBeforeInput = useCallback(e => {
  if (IS_DEBUG) { console.log('Before Input', e.inputType, e); }
  switch (e.inputType) {
    case 'insertCompositionText': {
      // We need to push each composition event so we can apply it when finished
      if (!lastComposeData.current) { lastComposeData.current = []; }
      try {
        // We need to convert the selection right away, as the window.getSelection() will change with the window
        const selection = window.getSelection();
        const sel = ReactEditor.toSlateRange(editor, selection);

        // The offsets may not match because the composition text might be far ahead etc...
        sel.anchor.offset = selection.anchorOffset;
        sel.focus.offset = selection.focusOffset;

        lastComposeData.current.push({
          selection: sel,
          value: e.data,
          node: selection?.anchorNode,
          elementNode: selection?.anchorNode?.parentElement.closest('[data-slate-node="element"]')
        });
      } catch {
        lastComposeData.current = null;
      }
      break;
    }
    case 'insertFromComposition':
    case 'deleteByComposition':
      // If we get this event we don't need to apply any sort of fix, this is the correct event to handle things
      lastComposeData.current = null;
      break;
  }
}, [ editor ]);

const onCompositionEnd = useCallback(e => {
  if (IS_DEBUG) { console.log('Composition End', e); }
  const { current } = lastComposeData;
  if (current) {
    // Store the current selection so we can move back once we have finished applying queued changes
    // Convert to slate range straight away as the selection can get messed up due to element fixing code
    const { anchorNode, anchorOffset, focusNode, focusOffset, isCollapsed } = window.getSelection();

    // Apply each of the changes
    for (const c of current) {
      const { selection, value, node, elementNode } = c;
      Transforms.select(editor, selection);
      SlateEditor.insertText(editor, value);

      // If the text was completely removed it can clear the slate element
      // which will cause a slate crash (Chrome)
      if (!value) {
        const textNode = node.parentElement.closest('[data-slate-node="text"]');
        const stringNode = node.parentElement;
        if (elementNode && textNode && stringNode && elementNode.children.length === 1 && elementNode.children[0].nodeName === 'BR') {
          stringNode.innerText = '';
          elementNode.replaceChild(textNode, elementNode.children[0]);
        }
      }
    }

    lastComposeData.current = null;

    // Move back to existing selection
    try {
      const ss = ReactEditor.toSlateRange(editor, {
        startContainer: anchorNode,
        startOffset: anchorOffset,
        endContainer: focusNode,
        endOffset: focusOffset,
        collapsed: isCollapsed
      });
      // Small fixup again
      ss.anchor.offset = anchorOffset;
      ss.focus.offset = focusOffset;
      Transforms.select(editor, ss);

      // Reset the dom selection as well... this can get out of alignment
      const sel = window.getSelection();
      sel.removeAllRanges();
      const newDomRange = ReactEditor.toDOMRange(editor, ss);
      if (newDomRange) {
        sel.addRange(newDomRange);
      }
    } catch {}
  }

  // Prevent slate from doing anything
  // This prevents the hack that is currently in onCompositionEnd that doesn't work
  e.data = null;
}, [ editor ]);

return <Editable
  spellCheck
  onCompositionEnd={onCompositionEnd}
  onDOMBeforeInput={onDOMBeforeInput}
/>;
kyrisu commented 4 years ago

@ajorkowski I was trying to test your solution and it seems to work OK until you use autocorrect / swipe on Android device.

I may be missing something, so I've created a codesandbox - could you take a look to confirm this is what you had in mind?

Edit:

I did some more testing and discovered something that may be interesting:

  1. If I put cursor after 'ipsum', press Space, swipe word 'hello' and confirm (with space or enter) - everything works
  2. If I put cursor after 'ipsum', press Enter, swipe word 'hello' and confirm - app crashes.

Conclusion would be - if there's something already in the line before the cursor, everything seems to work, but if you try to enter first word on the line (it may be understood as the beginning of the Slate node / paragraph) - it breaks.

P.S. I'm testing using standard Android Gboard (Google Keyboard)

ajorkowski commented 4 years ago

@kyrisu Good find... it looks like another hack is required for a new line and if you start a composing because slate has a specific structure for an empty text node vs a filled in text node. I have updated the code and cleaned it up a bit with this in consideration. Here is the codesandbox for my new fork:

https://codesandbox.io/s/slate-android-test-47z0s

Here is the code for posterity

import React, { useRef, useCallback } from 'react'
import { Transforms, Editor as SlateEditor } from 'slate'
import { Editable, ReactEditor, useEditor } from 'slate-react'

const IS_DEBUG = true

const Editor = () => {
  const editor = useEditor()
  const lastComposeData = useRef(null)

  const onDOMBeforeInput = useCallback(
    e => {
      if (IS_DEBUG) {
        console.log('Before Input', e.inputType, e)
      }
      switch (e.inputType) {
        case 'insertCompositionText': {
          // We need to push each composition event so we can apply it when finished
          if (!lastComposeData.current) {
            lastComposeData.current = []
          }
          try {
            // We need to convert the selection right away, as the window.getSelection() will change with the window
            const selection = window.getSelection()
            const sel = ReactEditor.toSlateRange(editor, selection)

            // The offsets may not match because the composition text might be far ahead etc...
            sel.anchor.offset = selection.anchorOffset
            sel.focus.offset = selection.focusOffset

            lastComposeData.current.push({
              selection: sel,
              value: e.data,
              node: selection?.anchorNode,
              elementNode: selection?.anchorNode?.parentElement.closest(
                '[data-slate-node="element"]'
              )
            })
          } catch {
            lastComposeData.current = null
          }
          break
        }
        case 'insertFromComposition':
        case 'deleteByComposition':
          // If we get this event we don't need to apply any sort of fix, this is the correct event to handle things
          lastComposeData.current = null
          break
        default:
          break
      }
    },
    [editor]
  )

  const onCompositionEnd = useCallback(
    e => {
      if (IS_DEBUG) {
        console.log('Composition End', e)
      }

      const { current } = lastComposeData
      if (current) {
        // Store the current selection so we can move back once we have finished applying queued changes
        // Convert to slate range straight away as the selection can get messed up due to element fixing code
        const {
          anchorNode,
          anchorOffset,
          focusNode,
          focusOffset,
          isCollapsed
        } = window.getSelection()

        // Apply each of the changes
        for (const c of current) {
          const { selection, value, node, elementNode } = c
          Transforms.select(editor, selection)
          SlateEditor.insertText(editor, value)

          if (value) {
            // HACK #1 - when a new line is created slate creates a zero-width
            // but actually it will be filled in, this causes slate crashes
            // Have to recreate a full element instead of an empty element in slate
            const el = node.parentElement
            if (el && el.hasAttribute('data-slate-zero-width')) {
              el.removeAttribute('data-slate-length')
              el.removeAttribute('data-slate-zero-width')
              el.setAttribute('data-slate-string', 'true')
              el.innerText = value
              const { path, offset } = editor.selection.anchor
              const p = { path, offset: offset - 1 }
              Transforms.select(editor, { anchor: p, focus: p })
            }
          } else {
            // HACK #2 - when an element is made empty during compose
            // the element is removed from dom, need to add it back
            // as a zero-width element to match
            const textNode = node.parentElement.closest(
              '[data-slate-node="text"]'
            )
            const el = node.parentElement
            if (
              elementNode &&
              textNode &&
              el &&
              elementNode.children.length === 1 &&
              elementNode.children[0].nodeName === 'BR'
            ) {
              el.innerHTML = '&#65279;'
              el.setAttribute('data-slate-length', '0')
              el.setAttribute('data-slate-zero-width', 'n')
              el.removeAttribute('data-slate-string')
              elementNode.replaceChild(textNode, elementNode.children[0])
            }
          }
        }

        lastComposeData.current = null

        // Move back to existing selection
        try {
          const ss = ReactEditor.toSlateRange(editor, {
            startContainer: anchorNode,
            startOffset: anchorOffset,
            endContainer: focusNode,
            endOffset: focusOffset,
            collapsed: isCollapsed
          })
          // Small fixup again
          ss.anchor.offset = anchorOffset
          ss.focus.offset = focusOffset
          Transforms.select(editor, ss)

          // Reset the dom selection as well... this can get out of alignment
          const sel = window.getSelection()
          sel.removeAllRanges()
          const newDomRange = ReactEditor.toDOMRange(editor, ss)
          if (newDomRange) {
            sel.addRange(newDomRange)
          }
        } catch {}
      }

      // Prevent slate from doing anything
      // This prevents the hack that is currently in onCompositionEnd that doesn't work
      e.data = null
    },
    [editor]
  )

  return (
    <Editable
      spellCheck
      onCompositionEnd={onCompositionEnd}
      onDOMBeforeInput={onDOMBeforeInput}
    />
  )
}

export default Editor
ajorkowski commented 4 years ago

One thing I did notice which I will work on tomorrow is if you highlight in the middle of the text and then press backspace. It actually removes two characters. The problem here seems to be that the composition is removing one character and the backspace is removing the second character.

ajorkowski commented 4 years ago

Yeah - I cannot work this one out at all. It seems like when you have a composition selected and press the backspace key when you are in the middle of a piece of text that everything is working correctly. First the onCompsitionEnd event is called, then the onBeforeInput with deleteContentBackward, and this removes the character in the right position. I can even see that React fires at this point to reconcile the views (which is already reconciled).

And then, after all that happens, for some inexplicable reason the same operation seems to happen again - since the original character is already removed, what happens is that the character right after the correct character is also removed. I'm thinking this must be Chrome applying the backspace even though the beforeinput event was explicitly prevented.

The problem is that this causes a mismatch in the Slate model - the slate model correctly has only one character removed but the dom no longer matches. The problem I guess is that React is not firing and fixing up the difference... I'm not sure if we could force this.

skogsmaskin commented 4 years ago

@ajorkowski - I think you're really on to something here! :+1:

Using the SwiftKey keyboard on Android duplicates a word whenever you press space to write the next one. With your workaround it doesn't!

One thing I've discovered is that when using another keyboard, like Hacker's Keyboard, then the word duplication isn't happening when pressing space.

Logging out the onDOMBeforeInput events from the different keyboards gives very different results. This is the log of writing one character then pressing space:

Hacker's Keyboard (working):

insertCompositionText
insertCompositionText
insertText

Swiftkey (duplicates character):

insertText
insertCompositionText
insertText

I've been experimenting with an even simpler workaround for the text duplication on space issue with Swi:

  const onDOMBeforeInput = e => {
    if (e.inputType === 'insertCompositionText') {
      const selection = window.getSelection()
      if (selection && selection.toString() === e.data) {
        Transforms.deselect(editor)
      }
    }
  }

This avoid duplicating the text when pressing space after a word, but still keeps inputting word suggestions from the keyboard working.

EDIT: this is of course not a real suggestion for a fix, but it demonstrates that this is related to selections somehow.

ajorkowski commented 4 years ago

@skogsmaskin

It looks like this work around is basically just removing the selection from the editor during the insert composition, which is basically essentially removing the tracking around this completely - this is covered essentially by the e.data = null; line in the current code. It would probably only work in the special case when you are editing just one composition (and not making a new line).

I should point out that the current version of the code seems to handle these situations in Android:

The only thing right now that doesn't seem to be working is the following:

Cheers

skogsmaskin commented 4 years ago

Awesome @ajorkowski - I wonder how we can move forward with this? Implement it directly into slate-react or have some kind of plugin?

Any thoughts?

ajorkowski commented 4 years ago

@skogsmaskin Ideally it would be great to get this added directly to slate-react since typing on Android should just work.

However this solution just feels so hacky and is very specific to android, because all the other browsers (even Chrome on desktop sigh) seem to implement insertFromComposition properly (or don't support it at all for older browsers, but nothing can be done about that...). I also feel like this solution is incomplete because there doesn't seem to be an easy way to fix the double backspace issue in the middle of a composition right now. Then again, how it is coded right now is written in a generalized way - if insertFromComposition is supported then the hacks are basically ignored.

Saying that - I have seen a few issues that have been coming up with typing say Chinese characters that I think this solution also solves, because the compositionEnd event is broken on Chrome desktop I think right now and you will get the multiple insert issue because of it.

I will try making a PR for these changes over the next week or two directly into slate-react and we can see how it goes from there.

thesunny commented 4 years ago

Here is a proposal for completing an Android plugin as an open source project via sponsorship https://github.com/ianstormtaylor/slate/issues/3786