[Android Text Input] restart when framework changes composing region#25180
Conversation
4a8a185 to
bbee466
Compare
justinmc
left a comment
There was a problem hiding this comment.
This is really great. Your explanation of the IME not expecting a composing region to change programmatically sounds consistent with what I've experienced of these bugs.
So help me understand what will happen with this PR in practice:
- Type some composing text in a composing keyboard, e.g. "nihao" with a Chinese pinyin keyboard.
- By tapping/clicking the screen, move the cursor: "ni|hao".
- Type something else: "nin|hao"
What happens on each platform:
- Flutter on master
- The cursor jumps and the input happens at the wrong spot: "nihaon|".
- Seems the same on all platforms.
- This is obviously a very bad experience.
- Native Mac TextEdit
- Composing ends when the cursor is moved and starts again at the new location: "nin|hao" where only the second "n" is composing.
- Native iOS Safari
- The entire text is still part of the same composing region. So it's "nin|hao" and 您好 is the first choice.
- This seems to be the best experience, even better than native Mac. I'm surprised that native Mac didn't seem capable of doing this.
- Flutter with this PR
- My understanding is that Flutter will now behave like native Mac did above, is that right?
| // to reset their internal states. | ||
| mRestartInputPending = composingChanged(mLastKnownFrameworkTextEditingState, state); | ||
| if (mRestartInputPending) { | ||
| Log.w(TAG, "Changing the content within the the composing region is discouraged."); |
There was a problem hiding this comment.
Nit: Could we link to an issue like flutter/flutter#78827 where this is explained in more detail? If it's ok to put a link in a warning message. Just a thought.
There was a problem hiding this comment.
Done. Does the Samsung bug repro with this patch on your S6?
|
@justinmc The problem this PR is addressing is this, when I tap to move the selection out of the composing region (or any other way that commits the composing text), the IME always adds the composing text back: example.mp4It only restarts the IME in the event that the current composing region is changed by the framework or the developer, so it resets its internal states. |
|
This pull request is not suitable for automatic merging in its current state.
|
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍
This does not reproduce on my S6, the behavior seems identical on my S6 with this branch and with master. It restarts the composing region every time I enter a character, so the composing region is only ever 1 character in size.
On my Pixel 2, which shouldn't have ever had the bug, the composing region resets only when I move the cursor, as usual. So on a non-affected device, Flutter's composing region behaves like TextEdit on Mac. I also realized that Gboard's Chinese input doesn't insert composing characters into the field at all, so the example I gave in #25180 (review) doesn't apply for Chinese (though it does apply for Korean).
I didn't try programmatically changing the composing region or anything but we'll have to follow up on the "maybe fixes" issues and see.
flutter/flutter#78827: there're multiple android duplicate input issues involving the framework committing a non-empty composing region (the Samsung input bug could be a manifest of this too, since in framework when done is pressed we call
clearComposing).Fixes flutter/flutter#73433
Fixes flutter/flutter#72980
maybe fixes flutter/flutter#78066
maybe fixes flutter/flutter#61097
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.