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

False Window inset is set when dismissing keyboard preventing apps from using the full screen. #118761

Closed
xster opened this issue Jan 19, 2023 · 12 comments · Fixed by flutter/engine#42700
Assignees
Labels
a: fidelity Matching the OEM platforms better a: text input Entering text in a text field or keyboard related problems customer: google Various Google teams P1 High-priority issues at the top of the work list platform-android Android applications specifically

Comments

@xster
Copy link
Member

xster commented Jan 19, 2023

This issue appears on android api 30 and above. (pre api 30 takes a different path and uses a heuristic to set window metrics).

When setting that the app wants full screen behavior by using WindowInsetsController.BEHAVIOR_SHOW_TRANSIENT_BARS_BY_SWIPE
The app will go fullscreen but then when keyboard is opened and closed the app incorrectly sets a window inset the size of the navigation bars even though there is no navigation bar.

Example screenshot showing the content in white and the background in red.

Screenshot 2023-01-24 at 10 58 09 AM

Original bug content "Extracting generic issue from b/258263557 where incorrect viewInsets were transiently being reported as the soft keyboard is being shown. "

@xster xster added a: text input Entering text in a text field or keyboard related problems platform-android Android applications specifically a: fidelity Matching the OEM platforms better customer: google Various Google teams P1 High-priority issues at the top of the work list labels Jan 19, 2023
@reidbaker reidbaker self-assigned this Jan 30, 2023
@reidbaker reidbaker added P2 and removed P1 High-priority issues at the top of the work list labels Jan 30, 2023
@reidbaker reidbaker changed the title Evaluate if WindowMetricsCalculator is more accurate than WindowInfoTracker at tracking Android window metrics False Window inset is set when dismissing keyboard preventing apps from using the full screen. Jan 30, 2023
@reidbaker
Copy link
Contributor

On api 30+ the red bar from the screenshot is a transient and causes content to shift at the end of the keyboard close animation. On earlier than api30 this condition can be suck.

@reidbaker
Copy link
Contributor

If you come across this issue as an app developer the quick fix is to depend on the androidx library for setting fullscreen.

Using WindowInsetsControllerCompat you avoid an edge condition in the flutter engine and get the expected behavior.

https://developer.android.com/reference/androidx/core/view/WindowInsetsControllerCompat#BEHAVIOR_SHOW_BARS_BY_SWIPE()

@reidbaker
Copy link
Contributor

To fix this in the engine we need to modify
(FlutterView#onApplyWindowInsets][https://github.com/flutter/engine/blob/main/shell/platform/android/io/flutter/embedding/android/FlutterView.java#L711] in the android R+ codepath. We need to rely on WindowInsetsController(Compat) to determine based on the values present if navigationBars or system bars are actually overlaying the flutter content and therefor need an inset. The most durable way to ensure our code works as expected is to use the Compat version of WindowInsets available in androidx.

Technical description of issue and fix:

On window there are bits you set for behavior. The newer one a customer was setting is BEHAVIOR_SHOW_TRANSIENT_BARS_BY_SWIPE.
What that does is say "Be full screen but if a user swipes in from the edge Overlay nav bars" Those nav bars do not trigger a view rebuild so that the animation is not janky and any legacy code is not triggered until the end of the animation. source

FlutterView is calling a deprecated (in api 30) but still working method getWindowSystemUiVisibility and was checking for the bit SYSTEM_UI_FLAG_HIDE_NAVIGATION. Which best I can tell was making our code believe that there were nav bars and then we set the inset correctly assuming those nav bars were permanent. Then the close animation triggered and we dropped the bottom inset.

Bottom line: We can override navigationBarVisible to false in the situations where nav bars are possible to be shown but they overlay instead of taking up an inset. The better productionized version of this code would/should store window as a variable and would check the other behavior types found in WindowInsetsControllerCompat

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) {
      int mask = 0;
      WindowInsetsControllerCompat controller =
          WindowCompat.getInsetsController(((Activity) getContext()).getWindow(), ((Activity) getContext()).getWindow().getDecorView());
      navigationBarVisible = (controller.getSystemBarsBehavior() & WindowInsetsControllerCompat.BEHAVIOR_SHOW_TRANSIENT_BARS_BY_SWIPE) == 0 ;

Note Googlers can see a longer more winding discovery here https://buganizer.corp.google.com/issues/258263557#comment73

@reidbaker
Copy link
Contributor

Suggested Task Order:

  1. Build publicly available reproduction case (reachout to reidbaker@ if you need help)
  2. Evaluate the variables defined starting in android api 30 that control window behavior and document the current vs expected behavior (BEHAVIOR_SHOW_TRANSIENT_BARS_BY_SWIPE, BEHAVIOR_DEFAULT, BEHAVIOR_SHOW_TRANSIENT_BARS_BY_TOUCH etc)
  3. Add productionized version of the snippet in the previous comment converting the post android R (api 30) codepath to use WindowInsetsController/WindowInsetsControllerCompat as needed.
  4. Add Unit tests to verify new behavior ensuring that the new tests fail under the old codepaths
  5. Check the behavior on an oder than android R device to ensure no regressions.

@reidbaker reidbaker added P1 High-priority issues at the top of the work list and removed P2 labels Jan 30, 2023
@reidbaker
Copy link
Contributor

Dropped to a p3 since its being worked on this quarter and no longer blocking a google customer with a work around.

@gmackall
Copy link
Member

Sorry this has gone so long without update! Had other things take priority and this went too long without extra info.

I'm currently working on a PR to fix this, am just finishing up writing the testing. I plan to have it up in review by tomorrow at the latest.

@gmackall
Copy link
Member

Still finishing up local testing of the other WindowInsetController behaviors, so I still have the pr in draft - will get the testing finished tomorrow and mark ready for review

@gmackall
Copy link
Member

I've spent a while testing this change, and I'm no longer convinced that the current fix is the right one - In particular, I dont believe we can override navigationBarVisible to false when BEHAVIOR_SHOW_TRANSIENT_BARS_BY_SWIPE is set, because my understanding is that this flag only defines a behavior that is taken when the navbar is hidden (it does not by itself define the navbar as hidden). The navbar can still be visible when this flag is set, if we simply haven't called WindowInsetsController.hide.

I currently believe that the issue is that the fact that we called WindowInsetsController.hide is somehow being overridden by the insets that we save and then apply in ImeSyncDeferringInsetsCallback. I'm currently trying to confirm if this is the case by running a sample app reproduction of this issue with custom engine builds.

@gmackall
Copy link
Member

Updates from more investigation:
The particular bad inset that is getting set is the one that we save before the start of the keyboard dismissal animation and then apply at the end of the keyboard dismissal animation. It is already wrong when it is passed into ImeSyncDeferringInsetsCallback.onApplyWindowInsets, so it can't be fixed by overriding navigationBarVisibile.

navigationBars=Insets{left=0, top=0, right=0, bottom=144} max=Insets{left=0, top=0, right=0, bottom=144} vis=true

I've tried overriding the OnSystemUiVisibilityChangeListener in FlutterView and can see that dismissing the keyboard should be setting the SYSTEM_UI_FLAG_HIDE_NAVIGATION, but it isn't set when it is checked in FlutterView.onApplyWindowInsets. I'm wondering if the difference is because the listener reports global changes, and these changes are somehow not being applied to the fragment, but haven't confirmed this.

cc'ing @Piinks as I can see you've done a lot of work on android fullscreen related code - if you have any thoughts ways in which an add-to-app use case using a FlutterFragment and manipulating system ui component visibility directly could clash with how flutter deals with system ui component visibility, I'd appreciate it! Understand if you're not the right person to ask though :)

@gmackall
Copy link
Member

gmackall commented May 31, 2023

Other things of note from testing:

  1. This false window inset does not only appear when using BEHAVIOR_SHOW_TRANSIENT_BARS_BY_SWIPE. A flutter fragment which hides the navbar using
WindowInsetsController controller = window.getInsetsController();
controller.hide(WindowInsets.Type.navigationBars());
controller.setSystemBarsBehavior(WindowInsetsController.BEHAVIOR_DEFAULT);

Will also get a false window inset on dismissal of the keyboard.

  1. When we dismiss the keyboard, a sequence of things happens in ImeSyncDeferringInsetsCallback:
    onPrepare is called
    onApplyWindowInsets is called with insets representing the final (post animation) state
    onStart is called
    onProgress is called multiple times, once for each step of the animation.

From printing out the insets that are passed to onApplyWindowInsets, I can see that the "final state" that is passed in (and that we save, and apply in onEnd) has visible, full height navbars:

navigationBars=Insets{left=0, top=0, right=0, bottom=144} max=Insets{left=0, top=0, right=0, bottom=144} vis=true

but insets that are passed to onProgress are animating towards 0:

navigationBars=Insets{left=0, top=0, right=0, bottom=144} max=Insets{left=0, top=0, right=0, bottom=144} vis=true
navigationBars=Insets{left=0, top=0, right=0, bottom=142} max=Insets{left=0, top=0, right=0, bottom=144} vis=true
...

which means that somehow the animation is receiving insets progressing towards a state that is different from the "final state" passed to onApplyWindowInsets. I'm still trying to find out why this is the case

@gmackall
Copy link
Member

gmackall commented Jun 2, 2023

It looks like this problem is coming from the fact that when we dismiss the keyboard, we have two different animations: one corresponding to the keyboard, and one corresponding to the navigation bar. The key is that they run almost at the same time, but the keyboard animation starts a millisecond before the navbar animation.

For context, the ImeSyncDeferringInsetsCallback works like this: for each animation that starts, we get a call to onPrepare with the animation, a call to onApplyWindowInsets with insets representing the post-animation state, and then calls to onStart, (multiple) onProgress, onEnd.

So what happens in this case is that:

  1. The keyboard animation hits onPrepare, sets needsSave and animating to true
  2. onApplyWindowInsets is called with post-animation state of hiding the keyboard (but not the navbar). We save the final state and set needsSave to false.
  3. The navbar animation hits onPrepare, where the typemask isn't of keyboard type, so we don't set needsSave (it stays false).
  4. onApplyWindowInsets is called with the post-animation state of both the keyboard and the navbar, but we don't save it. Instead we mark these insets as consumed and ignore them.
  5. A bunch of calls to onProgress happen
  6. We call onEnd for our keyboard animation and apply our saved insets from when the keyboard animation started. This is where the false inset comes from, as the navbar is indicated to be fully visible because these insets come from before the navbar hiding animation. We also set animating to false. This means that we don't make any more changes for the last couple calls to onProgress and the final call to onEnd for the navbar animation.

auto-submit bot pushed a commit to flutter/engine that referenced this issue Jun 13, 2023
…ck (#42700)

Fixes flutter/flutter#118761.

For some context: ImeSyncDeferringInsetsCallback extends [WindowInsetsAnimation.Callback](https://developer.android.com/reference/android/view/WindowInsetsAnimation.Callback) specifically to handle insets related to keyboard animations.

When a keyboard animation happens, we get a call to onPrepare with the animation, and then to onApplyWindowInsets with [WindowInsets](https://developer.android.com/reference/android/view/WindowInsets) that represent the post animation state. For example, for hiding the keyboard, we would get WindowInsets with height of 0 for insets of type WindowInsets.Type.ime() (input method editor). We save these post-animation insets. 

We then get calls to onProgress for each frame of the animation, and use the WindowInsets passed to onProgress to build new WindowInsets based on the saved post-animation insets, but with the keyboard insets overridden by the WindowInsets passed to onProgress.

Finally, we get a call to onEnd, where we apply the saved insets.

However, there can be multiple animation running at the same time. For example, dismissing the keyboard can result in an animation associated solely with the keyboard happening, as well as an animation associated only with the navigation bar happening. And they can start at slightly different times. This can result in the situation described in this comment: flutter/flutter#118761 (comment)

Re-setting needsSave to false ensures we don't ignore any updates to the final state, ensuring correct insets in onProgress and onEnd.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: fidelity Matching the OEM platforms better a: text input Entering text in a text field or keyboard related problems customer: google Various Google teams P1 High-priority issues at the top of the work list platform-android Android applications specifically
Projects
None yet
3 participants