Enable platform view keyboard input on Android Q #12085

mklim · 2019-09-10T00:10:16Z

Naively embedded platform views on Android were never able to receive
keyboard input, because they were never focusable. So far we've worked
around the limitation by hooking into InputMethodManager and proxying the
InputConnection from a focused window over to the embeded view.

Android Q changed InputMethodManager to be instanced per display instead
of a singleton. Because of this our proxy hook was never being called,
since it was being set up on a different instance of IMM than was being
used in the virtual display.

Update SingleViewPresentation to store the IMM from the focused window
and return it whenever there are any calls to INPUT_METHOD_SERVICE.
This hooks our proxy back into place for the embedded view in the
virtual display. This restores the functionality of our workaround from
previous versions.

Unfortunately there's still a lot of noisy error logs from IMM here. It
can tell that the IMM has a different displayId than what it's expecting
from the window.

This also updates the unit tests to support SDK=27. SDK 16 doesn't have
DisplayManager, so there were NPEs attempting to instantiate the class
under test.

Fixes flutter/flutter#38375.

Naively embedded platform views on Android were never able to receive keyboard input, because they were never focusable. So far we've worked around the limiation by hooking into InputMethodManager and proxying the InputConnection from a focused window over to the embeded view. Android Q changed InputMethodManager to be instanced per display instead of a singleton. Because of this our proxy hook was never being called, since it was being set up on a different instance of IMM than was being used in the virtual display. Update `SingleViewPresentation` to store the IMM from the focused window and return it whenever there are any calls to `INPUT_METHOD_SERVICE`. This hooks our proxy back into place for the embedded view in the virtual display. This restores the functionality of our workaround from previous versions. Unfortunately there's still a lot of noisy error logs from IMM here. It can tell that the IMM has a different displayId than what it's expecting from the window. This also updates the unit tests to support SDK=27. SDK 16 doesn't have DisplayManager, so there were NPEs attempting to instantiate the class under test.

amirh · 2019-09-10T00:26:46Z

shell/platform/android/io/flutter/plugin/platform/SingleViewPresentation.java

        this.startFocused = startFocused;
    }

+    private static Context wrapContext(Context context, @Nullable WindowManagerHandler windowManagerHandler) {


Feels like this can just be done in PresentationContext's constructor.

amirh · 2019-09-10T00:35:59Z

shell/platform/android/io/flutter/plugin/platform/SingleViewPresentation.java


        container = new FrameLayout(getContext());
-        PresentationContext context = new PresentationContext(getContext(), state.windowManagerHandler);
+        Context context = wrapContext(getContext(), state.windowManagerHandler);


I find this somewhat confusing, my thought process was: wrapContext is going to replace the IMM with the one it's getting from the context I'm giving it, but we're giving it the inner context which has the wrong IMM... ohh the innerContext is already wrapped at this point.

The other thing I'm finding not obvious from the code is that getContext() is already a "wrapped context", without digging into Presentation and ContextWrapper it's not obvious why we do this here.

I'm also a little worried about being confused by stack traces with 2 separate instances of PresentationContext.

I guess some of this can be mitigated with comments, alternatively we can implement our own Presentation and have a single context wrapper.

Tried to make this clearer in the last patch.

I added a comment, which is a way to clear this up while reading the code but wouldn't help with a debugger.

I also split the two main wrappers (IMM at construction time, both IMM and WindowManager onCreate) into two separate classes. It's still murky but I think this will make it clearer when going through this with a debugger since they'll show up as different classes. I experimented a little with trying to cast getContext() here into something intelligible, but because Presentation is wrapping it there wasn't anything I could really do.

It occurred to me that I could also extend Presentation and fork the constructor and createPresentationContext methods instead of just outright forking the entire class, which I think would mitigate most of my concerns with maintaining a fork of it.

WDYT?

amirh · 2019-09-10T00:38:47Z

shell/platform/android/io/flutter/plugin/platform/SingleViewPresentation.java

+        private @Nullable
+        final WindowManagerHandler windowManagerHandler;
+        private @NonNull
+        final InputMethodManager inputMethodManager;


Is it safe to cache this here? is it possible that the IMM for FlutterView is changed after we cached it? e.g FlutterView was moved to a different window (I actually won't be surprised if it's common for code that was written prior to Q to assume the IMM doesn't change, so not sure if there's any fail safe that mitigates it already).

Good catch, this would break if FlutterView moved to a different window after we cached it. FlutterView would have a new IMM then and this would still be getting the one from the previous window.

This is tricky. By definition we can't do anything around detecting what window we're "really" in and if it changes here since the whole point is to return the IMM from the "wrong" window. We also couldn't give the responsibility to FlutterView/etc to update this, for many reasons.

The only thing I'm thinking of right now is that we pipe a method reference all the way from FlutterView to here and delegate returning the IMM from that instead of caching anything. I'm not sure if that's worth it though, WDYT? I don't know if changing FlutterView's window at runtime is really supported as-is.

I'm not sure we could extend presentation without calling it's constructor (which uses createPresentationContext).

For the new embedding the plan is to have the PlatformViewsController aware of the current FlutterView (@matthew-carroll is doing this I believe). When that is done it should be easy to use the current FlutterView's IMM. As this thing should land soon let's leave a TODO here for now.

Okay, added a TODO for now.

amirh · 2019-09-11T16:58:31Z

shell/platform/android/io/flutter/plugin/platform/SingleViewPresentation.java

-    static class PresentationContext extends ContextWrapper {
-        private WindowManager windowManager;
-        private final WindowManagerHandler windowManagerHandler;
+    private static class PresentationContext extends ImmContext {


I think I'd find it less confusing if this didn't extend ImmContext (the base context this class is given is already an ImmContext).

amirh · 2019-09-11T23:29:38Z

shell/platform/android/io/flutter/plugin/platform/SingleViewPresentation.java

            if (WINDOW_SERVICE.equals(name)) {
                return getWindowManager();
            }
+            if (INPUT_METHOD_SERVICE.equals(name)) {


Do we need this? (I thought the base context would be based of an ImmContext anyway)

🤦‍♂️ We don't need it, thanks. Removed.

amirh

LGTM

matthew-carroll · 2019-09-12T21:30:17Z

This also updates the unit tests to support SDK=27. SDK 16 doesn't have
DisplayManager, so there were NPEs attempting to instantiate the class
under test.

What happens when Flutter is used on devices running SDK 16?

matthew-carroll · 2019-09-12T21:33:42Z

shell/platform/android/test/io/flutter/plugin/platform/SingleViewPresentationTest.java

+@TargetApi(27)
+public class SingleViewPresentationTest {
+    @Test
+    public void returnsOuterContextInputMethodManager() {


Can we please name all unit tests in a similar fashion to the others in the embedding, it[DoesSomething[WhenSomethingHappens]]? I'd prefer that all unit test names are framed with respect to the object under test so that we do not confuse unit tests with component, UI, and end-to-end tests. I'm working on a doc to formalize some testing practices in the embedding which I will share for comment soon.

I'm fine with that naming convention, but I think if we want to enforce any kind of subjective stylistic decisions at that level of granularity it should be a larger style guide discussion we have and commit to first, then point PR authors to the existing style guide. This particular test is internally consistent, which I think is a reasonable bar for something this minor. I'd like to avoid situations where PRs are reviewed based on how reviewers would personally have written code vs if the code is a net improvement to the code health. Basically, "Don’t block CLs from being submitted based only on personal style preferences.".

In general I'm wary of requiring something as specific and subjective as a specific unit test name style in our code requirements, beyond nitpicking that adjacent tests preferably be consistent. There is no overwhelming benefit or right answer to any particular naming convention, and enforcing any one would leads to PRs being temporarily blocked for no measurable benefit. It's also the kind of requirement we can't really expect PR authors to lint for or predict ahead of time. When the tests are run the names aren't even printed to the console unless they're failing.

Happy to discuss further in a doc. :)

Ok. I do think there is a meaningful role for this particular naming convention, but we can discuss further in the doc.

matthew-carroll · 2019-09-12T21:37:00Z

shell/platform/android/io/flutter/plugin/platform/SingleViewPresentation.java

            OnFocusChangeListener focusChangeListener
    ) {
-        super(outerContext, display);
+        super(new ImmContext(outerContext), display);


What happens to SingleViewPresentation when its associated FlutterEngine is detached from the UI? Does it get destroyed? If so, should we be explicitly cleaning up this ImmContext in any way? Or even more generally, is SingleViewPresentation currently implemented in a way that respects FlutterEngine being detached from the UI?

I'm not sure how this object behaves in an add to app situation. It looks like SingleViewPresentation is instantiated through VirtualDisplayController#create, when createPlatformView is called via the Flutter widget. I'm not familiar enough with the entire flow to know how that would behave if FlutterEngine was attached and detached. @amirh, WDYT?

As far as I can tell mContext itself should be consistent for the SVP, though.

mklim

What happens when Flutter is used on devices running SDK 16?

Platform Views require SDK 20. So if they're used in an app on an earlier SDK there's a runtime error logged saying that Flutter is running on SDK 16 but platform views requires SDK 20, and the platform views widget fails to render.

mklim · 2019-09-12T22:20:29Z

shell/platform/android/test/io/flutter/plugin/platform/SingleViewPresentationTest.java

+@TargetApi(27)
+public class SingleViewPresentationTest {
+    @Test
+    public void returnsOuterContextInputMethodManager() {


I'm fine with that naming convention, but I think if we want to enforce any kind of subjective stylistic decisions at that level of granularity it should be a larger style guide discussion we have and commit to first, then point PR authors to the existing style guide. This particular test is internally consistent, which I think is a reasonable bar for something this minor. I'd like to avoid situations where PRs are reviewed based on how reviewers would personally have written code vs if the code is a net improvement to the code health. Basically, "Don’t block CLs from being submitted based only on personal style preferences.".

In general I'm wary of requiring something as specific and subjective as a specific unit test name style in our code requirements, beyond nitpicking that adjacent tests preferably be consistent. There is no overwhelming benefit or right answer to any particular naming convention, and enforcing any one would leads to PRs being temporarily blocked for no measurable benefit. It's also the kind of requirement we can't really expect PR authors to lint for or predict ahead of time. When the tests are run the names aren't even printed to the console unless they're failing.

Happy to discuss further in a doc. :)

mklim · 2019-09-12T22:43:55Z

shell/platform/android/io/flutter/plugin/platform/SingleViewPresentation.java

            OnFocusChangeListener focusChangeListener
    ) {
-        super(outerContext, display);
+        super(new ImmContext(outerContext), display);


I'm not sure how this object behaves in an add to app situation. It looks like SingleViewPresentation is instantiated through VirtualDisplayController#create, when createPlatformView is called via the Flutter widget. I'm not familiar enough with the entire flow to know how that would behave if FlutterEngine was attached and detached. @amirh, WDYT?

As far as I can tell mContext itself should be consistent for the SVP, though.

mklim requested a review from amirh September 10, 2019 00:10

googlebot added the cla: yes label Sep 10, 2019

Fix lint.

85ec944

amirh reviewed Sep 10, 2019

View reviewed changes

Michael Klimushyn added 2 commits September 10, 2019 12:59

Review feedback, extra test.

2dbacdc

Split out ContextWrapper instances into classes

f77c424

amirh reviewed Sep 11, 2019

View reviewed changes

Review feedback

814783a

amirh reviewed Sep 11, 2019

View reviewed changes

Whoops.

f7487ac

amirh approved these changes Sep 11, 2019

View reviewed changes

mklim 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 Sep 11, 2019

mklim merged commit dfa9498 into flutter:master Sep 12, 2019

mklim deleted the webview_q_fix branch September 12, 2019 16:50

matthew-carroll reviewed Sep 12, 2019

View reviewed changes

mklim commented Sep 12, 2019

View reviewed changes

utzcoz mentioned this pull request Dec 25, 2021

Bump Robolectric to 4.7.3 #30437

Merged

8 tasks

Enable platform view keyboard input on Android Q #12085

Enable platform view keyboard input on Android Q #12085

Uh oh!

Conversation

mklim commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirh left a comment

Choose a reason for hiding this comment

Uh oh!

matthew-carroll commented Sep 12, 2019

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mklim commented Sep 10, 2019 •

edited

Loading