Fix bug in handling delete key event for android

[DeMonkeyCoder]

When typing RTL and LTR characters together in TextField, deleting a single character might cause it's previous characters from the same direction to be deleted.
Also, you can't delete a new line when typing RTL text.

This PR is for fixing these problems.

[justinmc]

Thanks for the PR! Here are just some quick comments about things we need to look at, but we should get @GaryQian to take a look as well.

One other thing is that you should include tests for this. One for each of the two problems you mention above would be perfect if possible. Look in InputConnectionAdaptorTest.java. You can run this test file locally like this:

./flutter/testing/run_tests.py --type java --java-filter io.flutter.plugin.editing.InputConnectionAdaptorTest

[DeMonkeyCoder]

There was also another problem in the main code before my pull request
boolean isRtl = mLayout.isRtlCharAt(mLayout.getLineForOffset(selStart));
should be:
boolean isRtl = mLayout.isRtlCharAt(mLayout.getLineStart(mLayout.getLineForOffset(selStart)))
for getting the first character in the line.

[DeMonkeyCoder]

I wrote a test for the problems I mentioned above.
I also changed my code and simply used android QwertyKeyListener which solved all the problems.
Please review my PR again.
Thanks.

[justinmc]

@alimahdiyar Looks like there is a conflict, could you merge master?

[DeMonkeyCoder]

@alimahdiyar Looks like there is a conflict, could you merge master?

@justinmc Master is merged and all checks are passed successfully.

[DeMonkeyCoder]

This is also a better fix for #9734 that handles emojis @GaryQian

[GaryQian]

Thanks for the changes, give me a moment to verify this is valid for all the different kinds of input.

[GaryQian]

It looks like there is a listener called TextKeyListener which may be a more generic (non-QWERTY specific) class to use.

If TextKeyListener does the same job as QwertyKeyListener, I would prefer if we used TextKeyListener instead, as it should be less likely to run into internationalization issues.

[GaryQian]

According to the API docs for QwertyKeyListener, QwertyKeyListener should not be instantiated directly. Instead, a TextKeyListener should be used which will multiplex and instantiate multiple keyboard-specific listeners.

[GaryQian]

See above

[DeMonkeyCoder]

Thank you for reviewing my PR.

I made the needed changes to my code and TextKeyListener class is being used now.

[DeMonkeyCoder]

I worked on the problem and tried to find a better solution and finally mixed my solution with the old one.
These are things that are fixed by this PR:

1. Fix detecting RTL line logic
2. Fix the bug that newlines couldn't be deleted after an RTL line.
3. Fix the bug that multiple characters were deleted when mixing RTL and LTR characters on pressing backspace. (Although you reported problems with complex emojis, it fixed the other bug which for me as an RTL user is more important)
4. Handle simple emojis on Chinese devices having problems with extendRight or extendLeft. (This is separately done in the last commit so tell me if you didn't like the fix).

P.S: extendLeft didn't work correctly on tests. I'm not sure but I think it needed a physical device. Anyway, I deleted the tests but confirmed everything works as good as possible on real devices.

Please review again. @GaryQian @justinmc
Thanks for your attention.

[DeMonkeyCoder]

@GaryQian Can you please review again?
I'm working on some other issues in TextAlign and Direction too and some people might be waiting for them to use their apps for production.
like this one flutter/flutter#53057

[GaryQian]

So I tested with flag/complex emojis and it looks like the new version still does not handle them properly. Let me see if I can wiggle it around to handle the emojis properly. I agree with the RTL handling changes, but I would not want to regress the emoji backspace behavior. Simple emojis should be no problem as they are represented by single codepoints.

[DeMonkeyCoder]

So I tested with flag/complex emojis and it looks like the new version still does not handle them properly. Let me see if I can wiggle it around to handle the emojis properly. I agree with the RTL handling changes, but I would not want to regress the emoji backspace behavior. Simple emojis should be no problem as they are represented by single codepoints.

When not mixing RTL and LTR text and writing simple one direction text, my code is using old extendRight and extendLeft functions. So if the bug exists in this scenarios the bug origin is not my code.

If the problem is when you mix RTL and LTR text (which is handled by the new code), it has the benefit of fixing more problems.

Thanks again for your review and working on the problem
I love flutter and I'm happy if I could make it grow better.

[GaryQian]

Hang in there, I am experiencing a strange issue where complex emojis are not being handled even with a clean, unmodified engine build. It seems that the debug local engine is somehow causing improper emoji handling?

[DeMonkeyCoder]

I didn't have devices that you reported the issue on and didn't experience the issue so I couldn't test myself. But as I said I think the problem is with bringing the caret in the wrong offset when re-placing.
I have a suggestion.
Log the caret offset when tapping the TextField and reassigning the caret place to see if the offset is between glyphs or not.

[GaryQian]

It's not that, your code might have been correct. When I run everything in release mode, it seems to start working. Still trying to isolate why this is happening. I haven't seen debug vs release differences like this before.

[GaryQian]

Ok, so I have confirmed that the version using just the TextKeyListener directly is producing correct results when built as a release engine. If you roll it back to that version (with the tests too!), I can approve it. I'm not sure how there can be differences in the debug vs release though, the only possible causes are either a timing issue (the keyboard API can get race-y due to the jumps between dart, engine, and embedder) or a dart compiler bug where there is some tiny difference between the two builds.

The GetGlyphPositionAtCoordinate() method in LibTxt is simply not being called, resulting in the incorrect caret positioning as you pointed out. Since this bug is unrelated to your fix, we can handle them separately.

[DeMonkeyCoder]

Done
Thanks again

[GaryQian]

LGTM, Thanks for your patience!

[DeMonkeyCoder]

@GaryQian Thanks.
I'm not familiar with this repository's merge process. Your approval means it will be merged automatically?

[GaryQian]

I will take care of merging it :). I'll let you know if anything needs to be fixed to pass presubmit.

[fluttergithubbot]

This pull request is not suitable for automatic merging in its current state.

• The status or check suite Mac iOS Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

[GaryQian]

@alimahdiyar Can you rebase to upstream master? There was a change made recently that is needed for the Mac iOS Engine test to pass.

git pull upstream master --rebase and a force push should do it. Thanks!

[DeMonkeyCoder]

@GaryQian Done :)

[GaryQian]

Landing manually, Fuchsia LUCI failure seems to be a flake.