wordpress-mobile / gutenberg-mobile

Mobile version of Gutenberg - native iOS and Android
GNU General Public License v2.0
259 stars 58 forks source link

Splitting Block Before Swiped Word Loses Word #2373

Closed mchowning closed 10 months ago

mchowning commented 4 years ago

Describe the bug

On Android, splitting a paragraph block with the cursor immediately before a swiped word causes that word to be lost.

losing_word_on_split mp4

To Reproduce

  1. Add multiple words to a paragraph block by "swiping"
  2. Place the cursor directly in front of one of the words you entered by swiping
  3. Tap Enter to split the block
  4. Observe that the word the cursor was immediately in front of was lost in the split

Expected behavior

Splitting a block should never lose content.

Smartphone

hypest commented 4 years ago

I can repro at hash c2a85e03f167c7fc33e2d79b67bf640e22f9e928, on a physical Pixel 2XL (Android 10).

maxme commented 4 years ago

I was able to reproduce it on Android (wpandroid alpha-223).

guarani commented 4 years ago

TL;DR: I think the Aztec library for Android differentiates between swiped and typed text when handling the Enter key event. This bug isn't reproducible in the Classic editor because unlike react-native-aztec, the Classic editor doesn't use custom Enter key handling AFAIK.


Investigation

I reproduced this on Android v15.2 in Gutenberg. I don't see this bug in the Classic editor which implies the bug might not be in Aztec itself but in either the Aztec wrapper (react-native-aztec) or in Gutenberg itself (e.g. rich-text).

The bug is present in all rich text blocks (Paragraph, Heading, List, Quote, Preformatted, Pull Quote, etc) but not present on the Code and Shortcode block which don't use rich text (and are not powered by Aztec). It's present on blocks that don't split when Enter is tapped (e.g. Quote, Preformatted, Pull Quote, Verse), so this bug isn't related to splitting blocks like I thought previously, but is about how the Enter key event is handled. To reiterate previous comments, it's only present when swiping on the Gboard, not when typing.

Debugging

I debugged using the Verse block instead of the Paragraph block because it a) reproduces the bug and b) avoids the unnecessary complexity of blocks being split in the process (tapping Enter in a Verse block just creates a newline within the same block).

I put a breakpoint in rich-text's handleEnter method, repeated the steps to produce this bug by swiping the text Hello world, and printed this.createRecord() as JSON:

View JSON
{
  "start": 5,
  "end": 5,
  "formats": [ ... ],
  "replacements": [ ... ],
  "text": "Hello"     <-- missing the " world" portion of the text
}

I then did the same steps, but instead of swiping I typed the text Hello world and again printed the return value of this.createRecord() as JSON:

View JSON
{
  "start": 6,
  "end": 6,
  "formats": [ ... ],
  "replacements": [ ... ],
  "text": "Hello world"
}

Going back to the failing case when swiping, why is createRecord returning only "Hello" instead of "Hello world"? I see that the value of this.value is Hello. this.value is populated from RichText's props via its constructor which in-turn comes from the Verse block here.

The Verse block, like any other that uses rich text, embeds an instance of Aztec. There is a React Native wrapper around this native view, called react-native-aztec. It handles sending events, such as key presses from where they originate inside Aztec to the React Native code, where they can have custom handling.

Inside react-native-aztec, ReactAztecEnterEvent handles Enter key presses. Tracing the use of this event back to onEnterKey(Spannable text, boolean firedAfterTextChanged, int selStart, int selEnd), I see that when swiping is used to type, tapping Enter sets the text argument of onEnterKey to just "Hello". In comparison, when typing, text is set to the expected value of "Hello world".

onEnterKey is one of Aztec's key listeners from the OnAztecKeyListener interface — which indicates the root cause of this issue is in the Aztec for Android library itself.

Next steps

I think the key to solving this is to look at Aztec's handleBackspaceAndEnter and understand why text is "Hello" when hitting Enter on swiped text and "Hello world" when hitting Enter on typed text.

guarani commented 4 years ago

To isolate the problem to Aztec and ensure it's not an issue in react-native-aztec or its usage of Aztec, I modified the Aztec demo project for Android to use a AztecText subclass, MyAztecText. This subclass added an OnAztecKeyListener and logged the events to the console.

View code
package org.wordpress.aztec.demo;

import android.content.Context;
import android.text.InputType;
import android.text.Spannable;
import android.util.AttributeSet;

import org.jetbrains.annotations.NotNull;
import org.wordpress.aztec.AztecText;

public class MyAztecText extends AztecText {
    public MyAztecText(@NotNull Context context, @NotNull AttributeSet attrs) {
        super(context, attrs);

        this.setAztecKeyListener(new OnAztecKeyListener() {
            @Override
            public boolean onEnterKey(Spannable text, boolean firedAfterTextChanged, int selStart, int selEnd) {
                System.out.println("Enter pressed");
                return true;
            }
            @Override
            public boolean onBackspaceKey() {
                System.out.println("Backspace pressed");
                return true;
            }
        });

        this.setInputType(InputType.TYPE_CLASS_TEXT | InputType.TYPE_TEXT_FLAG_CAP_SENTENCES | InputType.TYPE_TEXT_FLAG_MULTI_LINE);
    }
}

What I found is that the Enter event only fired when using the default Samsung keyboard of my S10 device, not when using the Google Gboard. These events are added using setOnKeyListener here.

This StackOverflow answer points to the View.OnKeyListener docs which suggest that this listener doesn't work for "soft input methods".

This is only useful for hardware keyboards; a software input method has no obligation to trigger this listener.

The suggested answer is to use a TextWatcher. I see text watchers are used in many places in Aztec and wonder if we could hook into an existing one or add a new one.

Edit: text watchers are used in react-native-aztec here, so digging there could lead to a fix.

mchowning commented 4 years ago

I just ran across a scenario where I lose text when I splitting the swiped word "whenever" in a list block, but doing the same thing on a paragraph block does not cause the same problem. 🤔

list_split_lose_word_comparison mp4

guarani commented 4 years ago

I just ran across a scenario where I lose text when I splitting the swiped word "whenever" in a list block, but doing the same thing on a paragraph block does not cause the same problem. 🤔

Could the bug you're seeing be specific to lists? If so, it could be https://github.com/wordpress-mobile/gutenberg-mobile/issues/2204.

mchowning commented 4 years ago

You're right. 👍 I forgot that #2204 involved more than just the multiple undo states, but also touched on loss of content with the initial split.

hypest commented 4 years ago

I think this ticket is not actually worked on at the moment so, I'll remove the assignee to denote that.

@cameronvoell , if you have any particular progress you want to add to the ticket for context, that'd be helpful for when we'll pick this up again. Thanks!

AmandaRiu commented 3 years ago

FYI: Still an issue as of 1.57.0. Tested on a physical Pixel 4 running Android 11 and Gboard:

https://user-images.githubusercontent.com/5810477/125340835-f099c500-e320-11eb-8999-91aae738bb31.mp4

AmandaRiu commented 3 years ago

I picked up this issue for Groundskeeping and did a bunch of debugging but haven't found a resolution yet. It’s an issue that only seems to happen when swiping out words using Gboard. It doesn’t happen when swiping words out on the Swiftkey keyboard, or tapping words out on any of the keyboards I’ve tested including Gboard. I spent hours debugging different areas of the app by adding logging, stepping through code in the Chrome debugger and in Android Studio trying desperately to figure out where in the code we were erroneously handling something but I had no luck. Here is some of my logging to demonstrate what I mean. The logs are from two different places: Android Logcat and console logging for the React-based code:

Android logs

D  ReactAztecManager.setTextFromJS > text=[]
D  ReactAztecManager.onTextChanged > newText=[One] oldText=[]
D  AztecReactTextChangedEvent > text=[One], keyCode=[]
D  ReactAztecManager.setText > text=[<pre>One</pre>] selection=[null]
D  ReactAztecManager.setText > text=[<pre>One</pre>] selection=[null]
D  ReactAztecManager.onTextChanged > newText=[ ] oldText=[]
D  ReactAztecManager.onTextChanged > newText=[two] oldText=[]
D  AztecReactTextChangedEvent > text=[<pre>One two</pre>], keyCode=[]
D  ReactAztecManager.setText > text=[<pre>One </pre>] selection=[null]
D  ReactAztecManager.setText > text=[<pre>One two</pre>] selection=[null]
D  ReactAztecManager.setText > text=[<pre>One two</pre>] selection=[null]
D  ReactAztecManager.onTextChanged > newText=[ ] oldText=[]
D  ReactAztecManager.onTextChanged > newText=[three] oldText=[]

// CORRECT STRING sent by textChangedEvent and set in the editor
D  AztecReactTextChangedEvent > text=[<pre>One two three</pre>], keyCode=[]
D  ReactAztecManager.setText > text=[<pre>One two </pre>] selection=[null]
D  ReactAztecManager.setText > text=[<pre>One two three</pre>] selection=[null]
D  ReactAztecManager.setText > text=[<pre>One two three</pre>] selection=[null]
D  ReactAztecManager.setText > text=[<pre>One two three</pre>] selection=[null]

// STRING "two" randomly deleted
D  ReactAztecManager.onTextChanged > newText=[] oldText=[two]

// STRING " " randomly deleted, onEnter processes new string "one three"
D  ReactAztecManager.onTextChanged > newText=[] oldText=[ ]
D  onEnterKey > text=One three, firedAfterTextChanged=true, selStart=3, selEnd=3
D  handling enter event!
D  ReactAztecManager.onTextChanged > newText=[
D  ] oldText=[]
D  EnterPressedWatcher.afterTextChanged > text=One
D   three, deleting org.wordpress.mobile.ReactNativeAztec.EnterPressedUnderway@53e1c96

// STRING "two" is magically sent back through the onTextChanged event
D  ReactAztecManager.onTextChanged > newText=[two] oldText=[]

// ENTER EVENT shows bad string of "One three"
D  ReactAztecEnterEvent > text=<pre>One three</pre>, selStart=3, selEnd=3

// CORRECT STRING sent again in this event, but dies here
D  AztecReactTextChangedEvent > text=[<pre>One<br>two three</pre>], keyCode=[]

// BAD STRING is final string set in editor
D  ReactAztecManager.setText > text=[<pre>One three</pre>] selection=[null]
D  ReactAztecManager.setText > text=[<pre>One<br> three</pre>] selection=[{ NativeMap: {"start":4,"end":4} }]
D  ReactAztecManager.setTextFromJS > text=[<pre>One<br> three</pre>]
D  ReactAztecManager.onTextChanged > newText=[One
D   three] oldText=[One
D  two three]
D  AztecReactTextChangedEvent > text=[<pre>One<br> three</pre>], keyCode=[]
D  ReactAztecManager.setText > text=[<pre>One<br> three</pre>] selection=[null]

React logs

LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[], props.value=[], comesFromAztec=[undefined], firedAfterTextChanged=[undefined]
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[], props.value=[], comesFromAztec=[undefined], firedAfterTextChanged=[undefined]
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[One], props.value=[], comesFromAztec=[true], firedAfterTextChanged=[true]
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[One], props.value=[One], comesFromAztec=[true], firedAfterTextChanged=[true]
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[One], props.value=[One], comesFromAztec=[false], firedAfterTextChanged=[false]
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[One ], props.value=[One], comesFromAztec=[true], firedAfterTextChanged=[true]
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[One two], props.value=[One ], comesFromAztec=[true], firedAfterTextChanged=[true]
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[One two], props.value=[One two], comesFromAztec=[true], firedAfterTextChanged=[true]
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[One two ], props.value=[One two], comesFromAztec=[true], firedAfterTextChanged=[true]
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[One two three], props.value=[One two ], comesFromAztec=[true], firedAfterTextChanged=[true]

// CORRECT text is processed "One two three"
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[One two three], props.value=[One two three], comesFromAztec=[true], firedAfterTextChanged=[true]
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[One two three], props.value=[One two three], comesFromAztec=[true], firedAfterTextChanged=[true]

// CORRECT text is REPLACED with "One three"
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[One three], props.value=[One two three], comesFromAztec=[true], firedAfterTextChanged=[true]
LOG  AMANDA-TEST: rich-text.onEnter > text=[One three], start=[3], end=[3]
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[One<br> three], props.value=[One three], comesFromAztec=[false], firedAfterTextChanged=[false]
LOG  AMANDA-TEST: RichText.shouldComponentUpdate > nextProps.value=[One<br> three], props.value=[One<br> three], comesFromAztec=[false], firedAfterTextChanged=[false]

The word "two" is removed from the string before the ReactAztecEnterEvent event happens. TextWatchers, keyListeners, event handlers, none of them seem to be responsible for this rather odd behavior. It could be my lack of understanding around how all these events are passed around and handled, but it’s almost like Android itself is performing the action of deleting the word “two” of its own volition!

I did a bunch of searches online and found a post describing the same behavior while porting a React-based editor library called Slate to be used on Android using React Native. Here’s a little snippet from the post:

Screen Shot 2021-08-04 at 11 02 46 PM

It feels like step #5 in this snippet doesn’t happen for us since that erroneously deleted text doesn’t get added back visually, but if you look at the Android logs I posted above, you do see it get added back:

// STRING "two" is magically sent back through the onTextChanged event
D  ReactAztecManager.onTextChanged > newText=[two] oldText=[]

And there is an event sent with the correct string afterword, but it seems to die at that point, possibly due to a race condition:

// CORRECT STRING sent again in this event, but dies here
D  AztecReactTextChangedEvent > text=[<pre>One<br>two three</pre>], keyCode=[]

The author went on to note that he has not found a good workaround and feels something needs to be fixed in the syncing of selection.

I’m throwing in the towel on this issue for the moment to work on other Groundskeeping issues but wanted to document what I learned during this for future reference. Maybe someone who understands Aztec’s integration with React Native will read this and have an “aha” moment that leads to a fix 🤞

hypest commented 3 years ago

FWIW, issue doesn't happen to me if I manually press SPACE between the swiped words.

fluiddot commented 3 years ago

I continued the investigations on this issue (thanks @AmandaRiu for such detailed debugging, it really helped me out on getting easily the context 🙇 !). In this case, I decided to explore and debug the native side and Aztec-Android , as the React Native side was already covered in previous comments.

As mentioned previously, when swiping out words using Gboard, splitting words produces an odd behavior leading to some of the words being deleted.

For the debug test, I relied on the TextWatcher class (reference) to listen for potential text change events, which for this case I observed several of them. I added breakpoints in the following methods and disable the deleteEnter prop on the Paragraph block (reference) as it could interfer with the test:

Here are the results when pressing the enter key after swiping out words One, two, and three:

  1. One two three => One three (two spaces between words)
  2. One three (two spaces between words) => One three
  3. One three" => "One\n three (NOTE: at this step when the text changes, we determine that the intro key was pressed when using Gboard and trigger onEnterKey event)
  4. One\n three => One\ntwo three

As far as I checked, the issue is related to the way we detect when the intro key has been pressed (step 3) and notify the React Native side before step 4 is executed. If we manage to figure out a way to trigger the onEnterKey event on step 4 instead of 3, we'll probably address the issue.

fluiddot commented 3 years ago

FWIW, issue doesn't happen to me if I manuall press SPACE between the swiped words.

Yep, I experienced the same behavior. I debugged both cases and the main difference is that when adding words by swiping, Android calls the method deleteSurroundingText (reference) when splitting words, which produces all text changes mentioned in https://github.com/wordpress-mobile/gutenberg-mobile/issues/2373#issuecomment-976900269, unlike when pressing SPACE key.

fluiddot commented 3 years ago

If we manage to figure out a way to trigger the onEnterKey event on step 4 instead of 3, we'll probably address the issue.

I tried to figure out a way to address this issue but no luck so far. I'd like to leave here some notes in case we revisit it in the future:

Gboard changes the text in multiple steps

I tested the Gboard on other apps and I noticed that the words added by swiping are treated as a single item when being modified. For example, when pressing the backspace key, instead of deleting one character it deletes the entire word. Similarly, when pressing enter, between words (this behavior is the one related to this issue) it breaks the text into two lines but also removes the blank space introduced when the word was added.

Delete word New line between words

The interesting part about the latter (the new line between words), as described in previous comments, is that instead of modifying the text in one change, it's being done in five actions.

I couldn't find the source code of the Gboard app to know the cause of executing so many steps. One thing I noticed is that this issue doesn't happen if after adding words, another text input is focused and then the text input is focused again. Looks like Gboard only gives words a special treatment while the text input is focused.

Potential workarounds

Prevent Gboard call deleteSurroundingText

We don't have a way to hack Gboard but we could skip some of the modifications done to prevent multiple text modifications. Regarding this, I investigated the option of overriding the InputConnection object by providing a custom implementation (reference). But I discarded this approach as it could introduce undesired side effects, and most likely won't prevent the last step (4 and 5) to be executed what would lead to duplicate part of the text.

Trigger onEnter event after all text modifications are done

The idea of this approach is to identify, by listening to text changes in EnterPressedWatcher, when the user pressed the enter key and the Gboard applied all text modifications.

The conditions for detecting this are:

  1. A word has been deleted next to the current selection in beforeTextChanged.
  2. A blank space before the current selection has been deleted in beforeTextChanged. At this point, we can keep a string with the word that was previously deleted, as we detected this before the text has changed.
  3. A new line character (\n) has been added in onTextChanged. This condition is the common way to detect if the enter key has been pressed, however in our case, we can't notify the onEnter event yet as the text is not complete.
  4. A word is added that matches previously one saved in onTextChanged. Here we detected the last text modification step (step 5), so we can trigger the onEnter event and notify React Native RichText component.

This approach has been my main goal in today's effort, however, although in theory should have worked, I realized two issues:

  1. Although onEnter is sent at proper timing, there's a synchronization problem with React Native side and the final text is not the expected one. As a workaround, I added a new parameter to the onEnter event (reference) to force the RichText component to update the current text with the value passed in the event (reference), which addressed the issue.
  2. Unfortunately, although the approach initially worked, I realized that the outlined previous steps can be reproduced manually leading to new issues.

Conclusion:

I struggled to figure out a reliable way to identify what Gboard does, but I couldn't find anything that tells us when these modifications are being done automatically and hence, infer that are being made by Gboard.

Delay onEnter event

As a last option, I tried to delay some milliseconds the call that triggers the onEnter event (reference). It worked although it breaks the line in an incorrect point, we might revisit this approach and figure out a way to address this issue in the future.

https://user-images.githubusercontent.com/14905380/143295307-b9b7b061-d3e5-4ce2-86f2-c84f9fe6d783.mp4

Conclusion:

This option might not be very reliable as we can't provide a minimum delay time, maybe on older devices, it requires a higher value which would end up on new issues.

Next steps

I would revisit the EnterPressedWatcher class, and provide fresh ideas on how to detect when the enter key has been pressed in Gboard (or other virtual keyboards).

geriux commented 10 months ago

This issue is no longer happening in the latest 24.0 version of Jetpack Android. I tested both the Gboard and Microsoft Keyboard SwiftKey on a Samsung Galaxy A23 5G on Android 14.

Before closing the issue, @fluiddot I was wondering if you could double-check on your device as you have previously investigated this thoroughly. For now, I'll remove the High priority label, thank you!

fluiddot commented 10 months ago

Before closing the issue, @fluiddot I was wondering if you could double-check on your device as you have previously investigated this thoroughly. For now, I'll remove the High priority label, thank you!

Thanks for taking a look @geriux 🙇 !

I confirmed that the issue can't be reproduced in version 24.0. However, when using the Gboard keyboard, I still notice that it modifies the text when splitting. As you can see in the attached video capture, it's removing the space between words. The problem is way less severe than the original issue, as despite removing a character, the words remain unmodified. Hence, I agree with closing the issue.

https://github.com/wordpress-mobile/gutenberg-mobile/assets/14905380/d4080fde-afa2-4ae3-a569-46aa64f1923a

geriux commented 10 months ago

Thanks for checking it!

The problem is way less severe than the original issue, as despite removing a character, the words remain unmodified. Hence, I agree with closing the issue.

Cool! I agree 🚀