Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Flutter focus to interop with Android view hierarchies #26602

Merged
merged 8 commits into from Jun 10, 2021

Conversation

blasten
Copy link

@blasten blasten commented Jun 5, 2021

When platform views are used in the Flutter widget tree, the keyboard may be dismissed or shown incorrectly.

The issue was present in hybrid composition too. When HC is used, this issue can be solved by checking
if any descendant view of the current platform view has been focused.

Fixes flutter/flutter#81029
Fixes flutter/flutter#34169 for HC.

This PR will also close flutter/flutter#34169 since using HC is the solution.

screen-20210605-120831_2.mp4

VD_PLATFORM_VIEW,
// InputConnection is managed by an embedded platform view that is embeded in the Android view
// hierarchy.
HC_PLATFORM_VIEW,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is HC meant to stand for "hierarchy"? Not sure if this is a standard abbreviation, but it was not intuitive for me. It may be clearer to just write it out.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I clarified it in the comment above.

Copy link
Contributor

@GaryQian GaryQian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % a few nits.

Emmanuel Garcia added 2 commits June 10, 2021 15:01
@blasten blasten 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 Jun 10, 2021
@fluttergithubbot fluttergithubbot merged commit 5c59a1d into flutter:master Jun 10, 2021
@math1man
Copy link
Contributor

FYI, we believe this change is causing thousands of crashes on our app. You cannot register a ViewTreeObserver.OnGlobalFocusChangeListener without also unregistering it when you're done with it. ViewTreeObserver listeners are global to the view hierarchy and unless unregistered will persist even after the view that registered them is destroyed. At best, this leaks the view, and at worst, causes crashes like this:

java.lang.NullPointerException
	at io.flutter.plugin.platform.PlatformViewsController.lambda$initializePlatformViewIfNeeded$0$PlatformViewsController(PlatformViewsController.java:746)
	at io.flutter.plugin.platform.PlatformViewsController$$ExternalSyntheticLambda0.onFocusChange(D8$$SyntheticClass)
	at io.flutter.embedding.engine.mutatorsstack.FlutterMutatorView$1.onGlobalFocusChanged(FlutterMutatorView.java:91)
	at android.view.ViewTreeObserver.dispatchOnGlobalFocusChange(ViewTreeObserver.java:960)
	at android.view.View.handleFocusGainInternal(View.java:6695)
	at android.view.ViewGroup.handleFocusGainInternal(ViewGroup.java:720)
	at android.view.View.requestFocusNoSearch(View.java:9920)
	at android.view.View.requestFocus(View.java:9899)
	at android.view.ViewGroup.requestFocus(ViewGroup.java:3048)
	at android.view.View.requestFocus(View.java:9866)
	at android.view.View.requestFocus(View.java:9845)
	at android.view.ViewRootImpl.focusableViewAvailable(ViewRootImpl.java:3724)
	at android.view.ViewGroup.focusableViewAvailable(ViewGroup.java:763)
	at android.view.ViewGroup.focusableViewAvailable(ViewGroup.java:763)
	at android.view.ViewGroup.focusableViewAvailable(ViewGroup.java:763)
	at android.view.ViewGroup.focusableViewAvailable(ViewGroup.java:763)
	at android.view.ViewGroup.focusableViewAvailable(ViewGroup.java:763)
	at android.view.ViewGroup.focusableViewAvailable(ViewGroup.java:763)
	at android.view.ViewGroup.focusableViewAvailable(ViewGroup.java:763)
	at android.view.ViewGroup.focusableViewAvailable(ViewGroup.java:763)
	at android.view.ViewGroup.focusableViewAvailable(ViewGroup.java:763)
	at android.view.View.setFlags(View.java:12775)
	at android.view.View.setVisibility(View.java:8814)
	<14 redacted stack trace lines>
	at android.os.Handler.handleCallback(Handler.java:751)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:154)
	at android.app.ActivityThread.main(ActivityThread.java:6816)
	at java.lang.reflect.Method.invoke(Method.java:-2)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1563)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1451)

We have a short-term mitigation, but the issue needs to be fixed ASAP.

@blasten
Copy link
Author

blasten commented Jul 22, 2021

@math1man thanks. working on the fix right now

@math1man
Copy link
Contributor

Sweet

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes platform-android waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
4 participants