Keyboard support for embedded Android views. #9203
Conversation
Generally what this PR is doing is setting up a delegation mechanism
for Android's onCreateInputConnection.
It works by letting the framework know when an embedded view gets loses
focus(within the virtual display), the framework maintains a focus node
for each Android view that is kept in sync with the focus state of the
embedded view.
The TextInputPlugin is extended to allow for 2 type of text clients a
"framework client"(what we had before) and a "platform view client".
When the AndroidView's focus node in the framework is focused the
framework sets a "platform view text client" for the TextInputPlugin,
which will result in the TextInputPlugin delegating
createInputConnection to the platform view.
When a platform view is resized, we are detaching it from a virtual
display and attaching it to a new one, as a side affect a platform view
might lose an active input connection, to workaround that we "lock" the
connection when resizing(by caching it and forcing the cached copy until
the resize is done).
Additional things worth calling out in this PR:
* To properly answer which views are allowed for input connection
proxying we compare a candidate view's root view to the set of root
views of all virtual displays.
* We also preserve a view's focus state across resizes.
Note that this PR only wires text for the io.flutter.view.FlutterView
For the new Android embedding some additional plumbing is neccessary.
|
I'm happy to split this PR into smaller parts if reviewers prefer. |
| /*** | ||
| * Use the current platform view input connection until unlockPlatformViewInputConnection is called. | ||
| * | ||
| * The current input connection instance is cached and any following call to @{link createInputConnection}. |
There was a problem hiding this comment.
Looks like there might be a typo or something here? Should the sentence end with an action description?
| PLATFORM_VIEW | ||
| } | ||
|
|
||
| public InputTarget(Type type, int id) { |
There was a problem hiding this comment.
Can you add a nullability annotation to type?
| accessibilityEventsDelegate.setAccessibilityBridge(null); | ||
| } | ||
|
|
||
| public void attachTextInputPlugin(TextInputPlugin textInputPlugin) { |
There was a problem hiding this comment.
Can you add javadocs for all public methods? And in those docs, for these attach/detach methods, can you describe when attach and detach should be called?
|
|
||
| import android.view.View; | ||
|
|
||
| public interface PlatformViewsResolver { |
There was a problem hiding this comment.
I think the style guide requires class level javadocs, too.
There was a problem hiding this comment.
deleted this interface
| mTextInputPlugin = new TextInputPlugin(this, dartExecutor, platformViewsResolver); | ||
| androidKeyProcessor = new AndroidKeyProcessor(keyEventChannel, mTextInputPlugin); | ||
| androidTouchProcessor = new AndroidTouchProcessor(flutterRenderer); | ||
| mNativeView.getPluginRegistry().getPlatformViewsController().attachTextInputPlugin(mTextInputPlugin); |
There was a problem hiding this comment.
I'm a bit concerned about this relationship between PlatformViewsController and TextInputPlugin. While TextInputPlugin technically requires a PlatformViewsResolver in its constructor, we know that really its the PlatformViewsController that we're passing. So effectively TextInputPlugin requires a PlatformViewsController at construction time, and then the PlatformViewsController requires someone to instruct it to attach to the TextInputPlugin.
Why not fold the relationship between TextInputPlugin and PlatformViewsResolver into the required relationship between TextInputPlugin and PlatformViewsController, i.e., just use the attached PlatformViewsController as the PlatformViewsResolver? Are there any cases where a PlatformViewsResolver would be required by the TextInputPlugin when that TextInputPlugin is not attached to a PlatformViewsController?
There was a problem hiding this comment.
Deleted PlatformViewsResolver, and passed a PlatformViewsController to TextInputPlugin.
The TextInputPlugin attaches itself to the PlatformViewsController.
I'm expecting that in the new embedding the TextInputPlugin will be detached from the PlatformViewsController in detachFromFlutterEngine(I guess when we do that we should probably add a dispose method or something like that to TextInputPlugin).
| } | ||
|
|
||
| @Override | ||
| public boolean checkInputConnectionProxy(View view) { |
There was a problem hiding this comment.
Can you add a javadoc to this method?
There was a problem hiding this comment.
This is an Android method we override, there's documentation in View#checkInputConnectionProxy
| } | ||
| }); | ||
| this.platformViewsController = platformViewsController; | ||
| platformViewsController.attachTextInputPlugin(this); |
There was a problem hiding this comment.
As you mentioned, I think we need symmetrical behavior with attachTextInputPlugin(). It's fine to wait for me to invoke that behavior in teh new embedding, but for historical reference purposes I would recommend adding that capacity here. At the moment we're introducing an impossibility to clean up that attach relationship.
I'm not sure what the naming convention is elsewhere in the embedding, it might be destroy, but we should probably go ahead and introduce such a method here for symmetry.
There was a problem hiding this comment.
In the current embedding the TextInputPlugin is created with a FlutterView and is never explicitly destroyed so I'm not sure where I'd add that. In the new embedding I believe there's a natural place to do that.
There was a problem hiding this comment.
I think what we'd want is a destroy() method beneath the constructor in TextInputPlugin and that method invokes platformViewsController.detachTextInputPlugin(). That works, right?
There was a problem hiding this comment.
Yes, the only caveat is that right now it seems like we're "destroying" the TextInputPlugin by restarting the input connection here:
I guess we'd want to move the equivalent logic into this new destroy method, probably not as-is(seems like the mechanism to kill the input connection is baked into FlutterView's onCreateInputConnection, wouldn't we want the TextInputPlugin to contain all the logic for managing an input connection?)
amirh
force-pushed
the
platform_view_keyboard
branch
from
54fa9c9 to
f0470f1
Compare
When an AndroidView gains focus we invoke the(newly introduced) 'TextInput.setPlatformViewClient' text_input system channel method which sets the platform view as the text input target. When the AndroidView loses focus we send a clearFocus message to platform views system channel(so the engine can clear the focus from the platform view). This PR is going to land before the engine implementation is rolled to the framework, we swallow MissingPluginException for the newly introduced method channel methods so this is a no-op before the engine is ready(after the engine is rolled with the corresponding change I'll remove the logic to swallow the exceptions). The engine counterpart is in: flutter/engine#9203
flutter/engine@0602dbb...06dbe28 git log 0602dbb..06dbe28 --no-merges --oneline 06dbe28 Fix instantiateImageCodec api diff with web (stub) (flutter/engine#9234) 99240b7 Remove unnecessary whitelisted flags for --dart-flags (flutter/engine#9233) 8eaddd6 Add web integration test to build_and_test_host (flutter/engine#9218) a7fb7da Roll src/third_party/dart 9f2f5adb64..6d608fb52b (5 commits) 041c791 Roll src/third_party/skia fe18de506097..14c8ca93db18 (11 commits) (flutter/engine#9231) 086b5a4 move webOnlyScheduleFrameCallback off of window (flutter/engine#9222) 8a6bad6 Roll src/third_party/dart 40ef0c6d9f..9f2f5adb64 (4 commits) 91ee780 Roll src/third_party/skia 6faf8d662af8..fe18de506097 (1 commits) (flutter/engine#9228) 8281919 Roll src/third_party/dart 6e0d978505..40ef0c6d9f (7 commits) 9dafb40 Roll src/third_party/skia 0e8362655a66..6faf8d662af8 (1 commits) (flutter/engine#9225) e3b8b61 Roll src/third_party/skia 3431d9d1dcc6..0e8362655a66 (2 commits) (flutter/engine#9223) e80df36 Keyboard support for embedded Android views. (flutter/engine#9203) 2ec6dad Roll src/third_party/skia 6e4fee8c812e..3431d9d1dcc6 (17 commits) (flutter/engine#9221) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.
This reverts commit e80df36.
This reverts commit ddd36e8.
#9203 broke the keyboard_resize integration test(see more details in flutter/flutter#34085 (comment)). This re-lands @9203 and fixes the issue the integration test uncovered by always allowing to hide the keyboard. The difference from the original change is 07d2598
Generally what this PR is doing is setting up a delegation mechanism for Android's onCreateInputConnection. It works by letting the framework know when an embedded view gets loses focus(within the virtual display), the framework maintains a focus node for each Android view that is kept in sync with the focus state of the embedded view. The TextInputPlugin is extended to allow for 2 type of text clients a "framework client"(what we had before) and a "platform view client". When the AndroidView's focus node in the framework is focused the framework sets a "platform view text client" for the TextInputPlugin, which will result in the TextInputPlugin delegating createInputConnection to the platform view. When a platform view is resized, we are detaching it from a virtual display and attaching it to a new one, as a side affect a platform view might lose an active input connection, to workaround that we "lock" the connection when resizing(by caching it and forcing the cached copy until the resize is done). Additional things worth calling out in this PR: To properly answer which views are allowed for input connection proxying we compare a candidate view's root view to the set of root views of all virtual displays. We also preserve a view's focus state across resizes. Note that this PR only wires text for the io.flutter.view.FlutterView For the new Android embedding some additional plumbing is necessary. Corresponding framework PR: flutter/flutter#33901 flutter/flutter#19718
…lutter#9257) flutter#9203 broke the keyboard_resize integration test(see more details in flutter/flutter#34085 (comment)). This re-lands @9203 and fixes the issue the integration test uncovered by always allowing to hide the keyboard. The difference from the original change is 07d2598
3 participants
Generally what this PR is doing is setting up a delegation mechanism
for Android's onCreateInputConnection.
It works by letting the framework know when an embedded view gets loses
focus(within the virtual display), the framework maintains a focus node
for each Android view that is kept in sync with the focus state of the
embedded view.
The TextInputPlugin is extended to allow for 2 type of text clients a
"framework client"(what we had before) and a "platform view client".
When the AndroidView's focus node in the framework is focused the
framework sets a "platform view text client" for the TextInputPlugin,
which will result in the TextInputPlugin delegating
createInputConnection to the platform view.
When a platform view is resized, we are detaching it from a virtual
display and attaching it to a new one, as a side affect a platform view
might lose an active input connection, to workaround that we "lock" the
connection when resizing(by caching it and forcing the cached copy until
the resize is done).
Additional things worth calling out in this PR:
proxying we compare a candidate view's root view to the set of root
views of all virtual displays.
Note that this PR only wires text for the io.flutter.view.FlutterView
For the new Android embedding some additional plumbing is necessary.
Corresponding framework PR: flutter/flutter#33901
flutter/flutter#19718