[Android] Fix using emojis inside composing regions

[jmartinesp]

I found a crash when testing the RTE that happened when I added an emoji in the middle of a word, the composer suddenly got an invalid index in the middle of the emoji and it crashes.

It turns out it comes from replaceAll doing editable.replace(0, editable.length, someOtherValue). To test this, I:

1. Wrote Test in the composer.
2. Set the cursor in the middle of the word (Te|st).
3. Added an emoji.
4. Realised that, before calling editable.replace(0, editable.length, ...) inside replaceAll, the selection is at 2, but after that it's automatically moved to 3 by no code of ours, something inside Android must be doing it.

The behavior of replaceAll was done like that to prevent the suggestion from flashing when typing, but I just now tried to do:

editable.clear()
editable.append(newValue)

And the issue is fixed and the suggestions still work as expected. Moreover, we're not sending invalid indexes anymore to the composer, which seems like an unexpected win.

I had to change the order of the expected results of dumpSpans() since the new spans are not added at the end now, but at position 1 .

[jmartinesp]

FWIW, I've also tested this with CJK languages and it works as expected, so I'm not sure why the previous behaviour was decided long ago.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (320f5c1) 87.44% compared to head (9d1e403) 87.28%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #883      +/-   ##
============================================
- Coverage     87.44%   87.28%   -0.16%     
  Complexity      407      407              
============================================
  Files           165      133      -32     
  Lines         18964    17364    -1600     
  Branches       1048      405     -643     
============================================
- Hits          16583    15157    -1426     
+ Misses         2086     1936     -150     
+ Partials        295      271      -24     

Flag	Coverage Δ
uitests	65.63% <100.00%> (-5.58%)	⬇️
uitests-android	65.63% <100.00%> (-0.09%)	⬇️
uitests-ios	?
unittests	86.64% <ø> (+1.11%)	⬆️
unittests-android	45.56% <ø> (ø)
unittests-ios	?
unittests-rust	89.39% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jonnyandrew]

The exception is from Rust:

Caused by: uniffi.wysiwyg_composer.InternalException: assertion failed: self.is_char_boundary(idx)

Do you think it could be an issue with indexing? Perhaps something similar to #824 i.e. not taking account of code points vs code units?

[jmartinesp]

The exception is from Rust:

Caused by: uniffi.wysiwyg_composer.InternalException: assertion failed: self.is_char_boundary(idx)

Do you think it could be an issue with indexing? Perhaps something similar to #824 i.e. not taking account of code points vs code units?

The error comes from Rust, but it's because the onSelectionChanged function of the EditText gets called when the editable.replace(0, editable.length, text) happens, with an invalid index. You can probably test this by adding Timber.d(getCurrentDomState().dump()) to fun ComposerModelInterface.log(), you'll see the selection index suddenly changes at that point.

I think it's normal that Rust throws an error in this case, that index shouldn't have been sent to it in the first place.

[jmartinesp]

We could try to solve this in Rust instead, or add some extra protection against it. But assuming the same example as above with Te😃st, having indexes 0-6 and valid indexes [0, 1, 2, 4, 5, 6], if Android sent a 3, should we just ignore it, or move it to 2 or 4?

[jonnyandrew]

I see what you mean... it looks like the EditText is working in code units as we see by manually moving the cursor from "Te|😃st" (position 2) to "Te😃|st" (position 4), the correct indices are sent to onSelectionChanged .

But if we insert "😃" into "Te|st" (position 2) then at some point during commitText(), onSelectionChanged is called with index 3.

I'm wondering if there is a missing a piece inside our input connection which is causing onSelectionChanged to be called with a selection based on out-of-date text content. Looking at the BaseInputConnection I can see some cursor positioning logic right around the point of replacing text. Perhaps we need to do something similar?

[jmartinesp]

I was talking about Editable.replace, since that's what we are using at the moment, but I see the BaseInputConnection also has this replaceText method.

The new position seems to be calculated there as:

newCursorPosition = newCursorPosition + start

In our case, we always pass newCursorPosition = 1 && start = 0 at that point, so technically we could use newCursorPosition to set the right index, although that means setting the selection twice for some reason according to that code , and only if the underlying baseInputConnection is actually of type BaseInputConnection (right now we assume it's InputConnection, which has a different implementation).

After looking at the code again, it seems to be setting whatever is on newCursorPosition is doing different logic when it's positive, negative or 0, but this is designed thinking about what would happen if you added a few characters or removed a few ones from the current position if I'm not mistaken, not the whole text (i.e., when it's 0 and start == end, it'll use an auto-calculated offset to move the selection forward for as many characters as were added).

However, I don't think doing this is needed at all. We're actually replacing the whole contents of the Editable with each change in the RTE, so doing clear and then repopulating the contents makes more sense to me than calling that replaceText function, which will try to preserve selection and spans.

[jmartinesp]

I think it's this code that's causing the selection change, the 2nd if, probably: https://cs.android.com/android/platform/superproject/+/master:frameworks/base/core/java/android/text/SpannableStringBuilder.java;l=552?q=SpannableStringBuilder

[jonnyandrew]

I tested and there is no issue with the flickering suggestion indicator (when using a soft keyboard). And given the text watcher issue fixed in #248 has not resurfaced, I don't see any issues with this change.

Thanks for fixing the issue!

[jonnyandrew]

Is this still necessary now we are clearing the editable?

[jmartinesp]

Probably not, I'll double check just in case.

[jmartinesp]

It is needed, it seems like clear() only removes the text and selection:

Here I started a list and just wrote "Let". The list item span was added 3 extra times, one for each new character.