Samsung duplication bug hack-fix #16547

GaryQian · 2020-02-11T21:37:55Z

This fixes flutter/flutter#31512 barring one edge case which this does not seem to cover.

The hack tells the samsung keyboard that the selection region is -1. This update also passes the composing region bounds, which we are able to set to -1. This apparently tricks the keyboard into resetting their internally cached composing region, which prevents the duplication. The selection region is then immediately restored in the updateEditingState call, so the user never sees the invalid selection.

In addition, I have gated this hack to only happen on Samsung devices in order to reduce the number of strange IMM calls that are being applied on non-relevant keyboards. This is unlikely to cause a regression on non-samsung keyboards, but the gating further reduces the chances.

Tests coming soon.

cbracken · 2020-02-12T01:03:35Z

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java

+    if (mIsSamsung) {
+      if (Build.VERSION.SDK_INT >= 21) {
+        // Update the keyboard with a reset/empty composing region. Critical on
+        // Samsung keyboards to prevent punctuation duplication.


Maybe lead with "Samsung keyboards don't clear the composing region on finishComposingText. [... current text ...]

cbracken · 2020-02-12T01:06:27Z

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java

+        || Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP
+        || !Build.MANUFACTURER.equals("samsung")) {
+      return false;
+    }


I assume if they fix this bug in a future release, we'll need to change this logic. Is there no way we can narrow this down further?

Overall this hack should be fully compatible with other keyboards. Anything we do here is not 1: telling the keyboard something that it should already know, or 2: performing an action that we are not guaranteed to immediately revert.

It seems unlikely to me that Samsung will release a fix for all versions as well as distribute to the hundreds of millions of devices in the wild.

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java

cbracken

Generally looks good -- just a couple questions.

mklim

🎉

mklim · 2020-02-12T20:35:56Z

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java

@@ -30,6 +33,9 @@
  private InputMethodManager mImm;
  private final Layout mLayout;

+  // Used to determine if Samsung-specific hacks should be applied.
+  private final boolean mIsSamsung;


nit: We actually don't want variables prefixed with m, it goes against Google Java style. The other members in the class already have it but I'd avoid matching it going forward. Ideally somebody should come back here later and clean the other names up so they fit the style guide.

mklim · 2020-02-12T20:42:32Z

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java

+        // Update the keyboard with a reset/empty composing region. Critical on
+        // Samsung keyboards to prevent punctuation duplication.
+        CursorAnchorInfo.Builder builder = new CursorAnchorInfo.Builder();
+        builder.setComposingText(-1, "");


optional/subjective nit here and throughout: It makes API calls like this easier to read if the primitives are annotated so you can tell what they're supposed to be controlling.

Suggested change

builder.setComposingText(-1, "");

builder.setComposingText(/*composingTextStart=*/-1, /*composingText=*/"");

good idea, much clearer what this is doing!

mklim · 2020-02-12T20:43:05Z

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java

+        CursorAnchorInfo anchorInfo = builder.build();
+        mImm.updateCursorAnchorInfo(mFlutterView, anchorInfo);
+      }
+      // TODO(garyq): There is still a duplication case that comes from hiding+showing the keyboard.


Hiding and showing the keyboard can restart the IMM. May be related?

Don't have a solid answer here, but in my experimentation, restarting IMM did not seem to have any noticeable effect on this behavior.

fluttergithubbot · 2020-02-12T23:08:02Z

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

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

…tter/engine#16547)

* f49a8b6 Roll src/third_party/skia c03e6982f96f..465864cad5d2 (14 commits) (flutter/engine#16524) * c477c06 Enable verbose logging for shell unittests on Fuchsia (flutter/engine#16526) * a662579 Clear frame references at the end of every CanvasKit frame (flutter/engine#16525) * 3f31ea3 Roll src/third_party/skia 465864cad5d2..21f382c19d76 (6 commits) (flutter/engine#16528) * 38fb6b1 Roll fuchsia/sdk/core/linux-amd64 from 8L7NY... to Bmq1m... (flutter/engine#16529) * 9c0168a Roll fuchsia/sdk/core/mac-amd64 from PMcw3... to 7JkB7... (flutter/engine#16530) * e8a888d Roll src/third_party/skia 21f382c19d76..f83d0346c06a (2 commits) (flutter/engine#16532) * 1e8b331 Roll src/third_party/dart 5244d99a5d4e..5fc031ebc1d7 (42 commits) (flutter/engine#16533) * c4e3ae6 Roll src/third_party/skia f83d0346c06a..88c3793a4eaa (1 commits) (flutter/engine#16534) * 6cdb14e Roll src/third_party/skia 88c3793a4eaa..abefc9c170c9 (1 commits) (flutter/engine#16535) * 975acd8 Roll src/third_party/skia abefc9c170c9..4fe89b4d871d (2 commits) (flutter/engine#16536) * b7424d0 Roll src/third_party/dart 5fc031ebc1d7..30151a654151 (2 commits) (flutter/engine#16537) * 25e8127 Roll src/third_party/skia 4fe89b4d871d..dc2782c380f6 (1 commits) (flutter/engine#16538) * 74fa10c Roll src/third_party/dart 30151a654151..76b18c455e2c (1 commits) (flutter/engine#16539) * 91b8e40 Roll src/third_party/skia dc2782c380f6..cdf2491afa04 (1 commits) (flutter/engine#16540) * 5acf9b1 Roll src/third_party/skia cdf2491afa04..50a490a1a4fb (2 commits) (flutter/engine#16541) * 9897777 Roll src/third_party/skia 50a490a1a4fb..c3b67eb988c8 (4 commits) (flutter/engine#16542) * 78a8909 Use os_log instead of syslog on Apple platforms (flutter/engine#13487) * ea56ad2 libtxt: use a fixture in the benchmarks (flutter/engine#16531) * a61dbf2 Revert "Use os_log instead of syslog on Apple platforms (#13487)" (flutter/engine#16546) * 539f64f [fuchsia] Disable retained layers (flutter/engine#16548) * c3b5072 Expose DPI helper functions for Runner apps to use (flutter/engine#16313) * 5041ff1 support endless trace buffer (flutter/engine#16520) * 6aacf5e Re-land: Use os_log instead of syslog on Apple platforms (flutter/engine#16549) * a5736b8 Roll src/third_party/skia c3b67eb988c8..b1525c721ea6 (4 commits) (flutter/engine#16543) * 49a370f Roll src/third_party/dart 76b18c455e2c..e4c39721c473 (6 commits) (flutter/engine#16544) * 270421c Fix ensureInitializationCompleteAsync callback when already initialized. (#39675) (flutter/engine#16503) * ca02b91 Prevent long flash when switching to Flutter app. (#47903) (flutter/engine#16527) * 44e80fd skiping tests in Safari. LUCI recipe for Mac is ready. this is the only step left for stopping us running unit tests in Safari (flutter/engine#16550) * 5fb0116 iOS platform view gesture blocking policy. (flutter/engine#15940) * e0ebaea Revert "Re-land: Use os_log instead of syslog on Apple platforms (#16549)" (flutter/engine#16558) * 8a6b949 [Fuchsia] Dump syslog output after tests have run (flutter/engine#16561) * bca879c Roll src/third_party/dart e4c39721c473..0299903f3e78 (31 commits) (flutter/engine#16553) * cd11d7a Roll fuchsia/sdk/core/mac-amd64 from 7JkB7... to t4kck... (flutter/engine#16555) * 99a265b [web] Fix edge cases in Paragraph.getPositionForOffset to match Flutter (flutter/engine#16557) * 8f8af1f Update felt documentation (flutter/engine#16559) * 13dce50 Roll src/third_party/skia b1525c721ea6..67da665c27ff (32 commits) (flutter/engine#16562) * 7c67573 Fix multiline Javadoc code blocks (flutter/engine#16565) * aece5ad Move log_listener call into the reboot trap (flutter/engine#16564) * 42f18d9 Roll src/third_party/skia 67da665c27ff..886e8500a9f2 (3 commits) (flutter/engine#16566) * c4c6ef6 Samsung keyboard duplication workaround: updateSelection (flutter/engine#16547) * 15062ca Revert "Re-arm timer as necessary in MessageLoopFuchsia" (flutter/engine#16568) * 8802a1d Roll src/third_party/skia 886e8500a9f2..9102c86a81ad (1 commits) (flutter/engine#16570) * dbdcae4 Roll src/third_party/skia 9102c86a81ad..6029cbd560b7 (2 commits) (flutter/engine#16575) * f39bc73 Exposes FlutterSurfaceView, and FlutterTextureView to FlutterActivity and FlutterFragment. (#41984, #47557) (flutter/engine#16552) * db030ec Roll src/third_party/skia 6029cbd560b7..1a733b5b760a (1 commits) (flutter/engine#16577) * 050d29d Roll src/third_party/skia 1a733b5b760a..1d1333fcedf8 (3 commits) (flutter/engine#16578) * 97fd898 Roll fuchsia/sdk/core/mac-amd64 from t4kck... to oHa-O... (flutter/engine#16581) * 2e67866 Roll src/third_party/skia 1d1333fcedf8..3bf3b92dfab0 (1 commits) (flutter/engine#16584)

…tter#16547)" This reverts commit df3f267.

tvolkert · 2020-03-29T20:40:18Z

This PR has been implicated as having introduced a regression in flutter/flutter#53086 and flutter/flutter#53052, both of which are marked as TODAY.

I'll send a revert.

tvolkert · 2020-03-29T20:41:06Z

Hmm, can't be cleanly reverted. @GaryQian please advise.

GaryQian · 2020-03-29T20:43:22Z

I can manually disable the workarounds for now.

GaryQian · 2020-03-29T20:45:25Z

This is one of those situations where disabling the workaround would re-introduce a number of editing bugs on a subset of samsung devices. Hard to say which set of bugs is "better".

Samsung duplication bug hack-fix

403d32d

GaryQian added platform-android affects: text input Work in progress (WIP) Not ready (yet) for review! labels Feb 11, 2020

auto-assign bot requested a review from cbracken February 11, 2020 21:37

googlebot added cla: yes labels Feb 11, 2020

GaryQian mentioned this pull request Feb 11, 2020

Samsung Keyboard duplicates text when restoring composing regions (restart text input, punctuation) flutter/flutter#31512

Closed

Fix formatting

014ff7b

cbracken reviewed Feb 12, 2020

View reviewed changes

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java Show resolved Hide resolved

cbracken reviewed Feb 12, 2020

View reviewed changes

Add test

465a8a2

GaryQian removed the Work in progress (WIP) Not ready (yet) for review! label Feb 12, 2020

COmments

b0ad38f

GaryQian requested a review from mklim February 12, 2020 20:32

GaryQian added 2 commits February 12, 2020 12:34

Formatting

11d88fd

Formatting

5dce049

mklim approved these changes Feb 12, 2020

View reviewed changes

Nits

cbfed0e

GaryQian added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 12, 2020

GaryQian added 2 commits February 12, 2020 13:50

Formatting

f01c985

Typo

541ebee

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 12, 2020

FOrmatting again

c447211

GaryQian added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 13, 2020

GaryQian merged commit c4c6ef6 into flutter:master Feb 13, 2020

engine-flutter-autoroll mentioned this pull request Feb 14, 2020

Roll engine 0e8ed2715a35..2e6786621f7b (49 commits) flutter/flutter#50803

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 14, 2020

c4c6ef6 Samsung keyboard duplication workaround: updateSelection (flu…

d4b946c

…tter/engine#16547)

NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

Samsung keyboard duplication workaround: updateSelection (flutter#16547)

df3f267

NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020

Revert "Samsung keyboard duplication workaround: updateSelection (flu…

1ba506f

…tter#16547)" This reverts commit df3f267.

GaryQian mentioned this pull request Mar 3, 2020

Samsung keyboard composing region duplication on subset of devices flutter/flutter#51893

Closed

goderbauer mentioned this pull request Mar 25, 2020

Text disappear while typing on Samsung keyboard flutter/flutter#53086

Closed

GaryQian mentioned this pull request Mar 29, 2020

Partially revert Samsung keyboard force-update workaround #17391

Merged

	builder.setComposingText(-1, "");
	builder.setComposingText(/composingTextStart=/-1, /composingText=/"");

Samsung duplication bug hack-fix #16547

Samsung duplication bug hack-fix #16547

Uh oh!

Conversation

GaryQian commented Feb 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Feb 12, 2020

Uh oh!

tvolkert commented Mar 29, 2020

Uh oh!

tvolkert commented Mar 29, 2020

Uh oh!

GaryQian commented Mar 29, 2020

Uh oh!

GaryQian commented Mar 29, 2020

Uh oh!

Uh oh!