Fix DynamicLayout Crash - Round 2

[mkevins]

Fix

This PR is an attempt to address the issue described here: wordpress-mobile/WordPress-Android#8828. This issue is related to #516 and #729 which were addressed with PR #801. This is a workaround to attempt to avoid encountering the underlying Android OS bug described here and patched here.

This workaround utilizes an InputFilter as in #801. The code in that PR covers many scenarios that lead to a crash, however, there are still some that cannot be addressed with that approach. One limitation of the InputFilter approach is that we may only modify the text being inserted during a change, but cannot modify the text being removed. Also, in that PR, strict criteria must be met before the fix is applied. One such criterion is that the selection must be a caret (i.e. no selection). This means certain edge cases will still encounter the crash.

One odd note is that if we set up the Demo App content with text, followed by two images, and then some more text, we can trigger the crash by deleting the text preceding the image only after we have edited that text at least once. Also, even after having edited that text, if we used the undo and redo buttons to attain the same state, we could then delete the text without a crash.

Somehow, setting the content in full (as occurs when the app first loads) will put the app into a "safe" state. I also noticed that when the first change happens above the images, the images move vertically by a very small amount.

In order to make use of this in a new workaround, we can set the text within the InputFilter. This is accomplished by utilizing the fromHtml method, passing in the output from toFormattedHtml. In order to do so with the new state of the content, we must overload the toFormattedHtml method to accept a Spannable, whereas the default behavior is to generate the html from the unedited content (which would merely revert our changes, disallowing modification by the user).

The nature of this PR is to expand the number of cases it addresses by relaxing the criteria for which it is applied. This also means that any side effects that such a workaround may introduce have the potential to affect more users. Since an unrecoverable crash is a fairly severe condition, it holds some weight, but, some consideration should also be given to the volume of users that are currently not experiencing this crash. It would be helpful if we could test these changes as thoroughly as possible so ensure we do not introduce any regressions.

Test

Follow steps here: wordpress-mobile/WordPress-Android#8828 (comment).
Also, here: #516 (comment).
And here: #729 (comment).

Any more tests in those threads would also be good candidates for testing this PR.

Review

@mzorz

[mzorz]

Just thinking aloud here - we discussed this of course but I'm not sure about doing such a radical thing for everyone with Android 8.x just as you already rightfully commented on the PR description:

Also, in that PR, strict criteria must be met before the fix is applied. One such criterion is that the selection must be a caret (i.e. no selection). This means certain edge cases will still encounter the crash.

We may want to narrow down the cases so this doesn't introduce a regression for other users who maybe weren't experiencing the problem so, thinking in that direction it seems to me it'd be nice if we kept the "case detection logic" (such as the big if clause that was up there) and add / remove more constraints as we see fit. I'm not really convinced on one way or the other, so please take this comment just as to keep the conversation going and see what we can get from it rather than an indication / suggestion of "what seems right" in code 👍

OTOH it may be the only way to cover "all" the possibilities where the issue may appear. I believe we're still spot on in that Images are the ones internally using DynamicLayout so it seems right to do the search for it (as in, the broader case).
I'll do some more smoke tests, another thing that comes to mind is maybe we also need to try and keep the caret position as well? Though we may want to address that as a separate PR if it starts to get convoluted

[mkevins]

and add / remove more constraints as we see fit

I totally agree with this. So far, these are the conditions that must be met:

1. Android API version must be either 26 or 27.
2. The crash preventer must not already be "in progress" (i.e. no recursion).
3. At least one character must follow the selection / caret.
4. The character immediately following the selection / caret must have at least one AztecImageSpan (i.e. it is an image).

Conditions 2 and 3 are set in the outer-most conditional of the filter. Condition 4 requires querying of the Spannable, and thus lives inside that outer conditional. If there is a way to determine whether we are on a version of Oreo which has been patched, we could narrow condition 1 further, but I'm not sure this is currently possible.

If you have other conditions that come to mind, we can add them. Note: I intentionally left out the condition regarding the new line character because this approach was different enough, I thought it best to test the behavior separately. Do you know if the current state reproduces (regresses) the issue that condition was added for?

[mkevins]

another thing that comes to mind is maybe we also need to try and keep the caret position as well? Though we may want to address that as a separate PR if it starts to get convoluted

I noticed this on develop as well, so I think a separate PR would be better, with an issue to track it.

[mzorz]

Note: I intentionally left out the condition regarding the new line character because this approach was different enough, I thought it best to test the behavior separately. Do you know if the current state reproduces (regresses) the issue that condition was added for?

I think you're right - just tested that the conditions this PR here #809 was fixing are not met with this PR anymore and wasn't able to reproduce the crash so, I think we're good 👍

[mzorz]

hey @mkevins I had to re-introduce the check in f77e576 (hope you don't mind!) to fix the connected tests that were failing for this PR 👍

[mzorz]

I've been testing this on WPAndroid with the following PRs:
wordpress-mobile/WordPress-Android#9773
wordpress-mobile/gutenberg-mobile#947

With the content in this report fabric-5a76a4fa8cb3c2fa634286c6, session fabric-5CC8BCF7015E00017DBC2EE3F6DE56FF_DNE_0_v2 (the logs show a Post with several images in it), and can observe there's a lag when positioning the cursor typing fast, with entered sequence being lost in there. After giving it time, you can re-type (if not right before an Image, that is) and it works without lag. Fortunately the caret position is kept so, it's no big deal - of course much better than crashing so, I think this is bearable.
Here's a video of it https://cloudup.com/coQJdVLvDXA

I've tried with other long posts and can see the lag Aztec already had before so, not a pointer that this solution in particular may be problematic (or should I say, be more problematic than it was before).

I think we should test a bit more but, generally, looks good as to try it!

Let's fix the lint issues and we can try ship a new version of Aztec at next cycle, sounds good @mkevins ?

[mkevins]

I think we should test a bit more but, generally, looks good as to try it!

Let's fix the lint issues and we can try ship a new version of Aztec at next cycle, sounds good @mkevins ?

Sure, I've pushed a commit to fix the lint errors. Thanks for setting up the related PRs @mzorz !

[mzorz]

We had @daniloercoli's eyes on this one as well and approved merging so, merging

[daniloercoli]

Tested and worked as expected. Also tried tests on this PR 🎉