Fix inline predictive text and keyboard suggestion toolbar use cases.

[langleyd]

This PR aims to fix a number of issues with predictive suggestions(both inline and in the toolbar) causing in consistent state between RTE model and text view content. Uses cases tested include:

• tapping on the suggestion to accept it
• using spacebar to accept it
• using backspace when on the boundary of a suggestion
• Replacing a selected word with one from the keyboard suggestion toolbar

It should help us address the problems in: https://github.com/element-hq/element-x-ios/issues/2345

This issues arise due to the old reconciliate solution.

The Problem:

In the simpler editor use cases one can assume keystokes and selection events are reflected in both the text view an the internal RTE representation and match. This is not always the case though, fancy features like inline predictive suggestions, text dictation and keyboards for non-latin based languages often dump "pending" text updates into the text views text storage(an attributed string). This mismatch in content makes reconciling the rte and text view content and the selection indices difficult.

The old solution (reconciliate with string differ):

• The text view is the source of truth
• When it is different from our RTE content, we calculate the difference between them and try and insert it.
• This only works if there was a simple difference located in one place(which is generally the case for some of the use cases mentioned above). Any more complex differences meant it it errors out and tries to restore to a previous state.
• This solution works for some of he main autocomplete use cases but was a bit prone to bugs and didn't cover all cases.

The new solution:

• One fortunate behaviour of the iOS text view apis is that the shouldChangeTextIn delegate function is called when text is committed to the text view. So for example when the predictive text is still grey, it will be reflected in textVIew.attributedText, but crucially shouldChangeTextIn will not be called until the suggestion is actually accepted(say by tapping on it).
• The new solution creates a new text storage in the form of committedAttributedText which represents the text that has been committed. So in addition to applying mutations to text view content to be reflected in the UI, we also apply them to committedAttributedText.
• In committedAttributedText we have a way to determine when there is text in the editor that has not yet been committed by the user(e.g in line predictive text).
• This combination of data allows us to better address some of the edge cases on the uses cases mentioned at the top.

Edit: We determined using this committedAttributedText works well for latin character languages which make sensible use of the shouldChangeTextIn api for committed text. Some other languages like Japanese Kana however do not, and will substitute characters without sensibly notifying via the apis. We therefore we have kept the reconciliate function and fall back to it if the content of the editor changes beyond simple latin character mutations.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 62.50000% with 57 lines in your changes missing coverage. Please review.

Project coverage is 89.24%. Comparing base (7488319) to head (3fc7e64). Report is 3 commits behind head on main.

Files	Patch %	Lines
...WysiwygComposerView/WysiwygComposerViewModel.swift	57.62%	50 Missing :warning:
...ents/WysiwygComposerView/WysiwygComposerView.swift	0.00%	7 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #890 +/- ## ========================================== - Coverage 90.32% 89.24% -1.08% ========================================== Files 177 160 -17 Lines 21173 20725 -448 Branches 280 280 ========================================== - Hits 19124 18496 -628 - Misses 2046 2226 +180 Partials 3 3 ``` | [Flag](https://app.codecov.io/gh/matrix-org/matrix-rich-text-editor/pull/890/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=matrix-org) | Coverage Δ | | |---|---|---| | [uitests](https://app.codecov.io/gh/matrix-org/matrix-rich-text-editor/pull/890/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=matrix-org) | `?` | | | [uitests-ios](https://app.codecov.io/gh/matrix-org/matrix-rich-text-editor/pull/890/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=matrix-org) | `?` | | | [unittests](https://app.codecov.io/gh/matrix-org/matrix-rich-text-editor/pull/890/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=matrix-org) | `89.24% <62.50%> (-0.19%)` | :arrow_down: | | [unittests-ios](https://app.codecov.io/gh/matrix-org/matrix-rich-text-editor/pull/890/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=matrix-org) | `87.32% <62.50%> (-1.37%)` | :arrow_down: | | [unittests-react](https://app.codecov.io/gh/matrix-org/matrix-rich-text-editor/pull/890/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=matrix-org) | `88.66% <ø> (ø)` | | | [unittests-rust](https://app.codecov.io/gh/matrix-org/matrix-rich-text-editor/pull/890/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=matrix-org) | `89.67% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=matrix-org#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[nimau]

@langleyd, the "." shortcut seems problematic as it doesn't call func textView(_ textView: UITextView, shouldChangeTextIn range: NSRange, replacementText text: String) -> Bool. So the '.' is present in the textview but disappears once you delete something.

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[aringenbach]

@langleyd, the "." shortcut seems problematic as it doesn't call func textView(_ textView: UITextView, shouldChangeTextIn range: NSRange, replacementText text: String) -> Bool. So the '.' is present in the textview but disappears once you delete something.

AFAIK any language that magically replace latin characters with symbols, have the exact same behaviour as the "." shortcut.

[langleyd]

I did test a number of CJK keyboards and from my initial tests I couldn't break it.

On testing again though, I can see that although shouldChangeTextIn does seem to be called (unlike the "." shortcut), the index in some cases is not correct. For example of you use the Koran 10 key keyboard and cycle through the characters by pressing 1 key multiple times, it replaces the character in the editor but adds each one in the RTE state as the index returned does not really seem correct. :/

[aringenbach]

@langleyd I'm not sure anymore about Korean keyboards behaviour, they might have less issues than some of the others. I think you can try with Japanese Romaji Qwerty for example, this one had lots of issues, and most system events will not get triggered, just magically replaces the text.

[Velin92]

I also found another bug with CJK keyboards. If you type a word for example "kudesai" with a romanji keyboard, before accepting any suggestion, if you tap backspace to delete the last character we get, happens that we either get a html range error (which prevents the deletion to happen at all), or we duplicate the text and append it at the end. I tested on main, and this seems to be a CJK regression since it's not happening. @langleyd

I am trying to write a test for it to reproduce it through it

[langleyd]

Interesting thanks @Velin92 will test it tomorrow. Sounds a bit like the containsLatinAndCommonCharactersOnly might be handling some cases incorrectly :/

[Velin92]

Interesting thanks @Velin92 will test it tomorrow. Sounds a bit like the containsLatinAndCommonCharactersOnly might be handling some cases incorrectly :/

can actually be reproduced even more easily by simply writing with the romanji keyboard any japanese character for example when writing "ku" which is converted into "く" , and then tapping backspace before accepting the suggestion makes throws a range error and prevents deletion

[langleyd]

Ive added a fix @Velin92

  if range != attributedContent.selection {
            select(range: range)
        }

needed to moved above the logic that used attributedContent.selection otherwise the selection of the textview does not match that of attributedContent.selection and you get the index issue.

I have found another though, I'll try create a test case for it.

[langleyd]

When you accept an inline predictive text suggestion with spacebar, then tap .. It inserts the dot before the space, next to the last work, but the shouldChangeTextIn gives you the index as if it's just inserted it at the cursor (after the space). 🫠

It would have to be another special case for inline predictive text. (unless we want to go back to blanket reconciliate magic, or build the proper tree based reconcile).

[langleyd]

The moving back of the dot after accepting inline text prediction was an optimisation brought in in 17.5. I was able to verify outside of out app(e.g. in the reminders app). This is why it was failing in the CI(17.2) but not for me locally(17.5).

I've added the version check, so should be all good now.

[sonarcloud[bot]]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud