[Android] Fix incorrect viewInsets during keyboard animation with EdgeToEdge #39391
Conversation
…insets in some cases
JonathanPeterCole
changed the title
|
@reidbaker Please route the appropriate reviewer. |
|
@gmackall can you give this a first pass it looks related to an issue you are working on. I will give it a second pass when it is ready. |
|
From Triage: Ping @gmackall. |
shell/platform/android/io/flutter/plugin/editing/ImeSyncDeferringInsetsCallback.java
Outdated
Show resolved
Hide resolved
|
@gmackall can you provide some next steps for this pr. |
|
Sure! The issue of excluding all systembars vs only navbars is the only comment I’m looking to resolve, once we can resolve that I can give an approval and @reidbaker will do a final review. Let me know when you’ve had a chance to make/test the change! Thanks for the contribution and especially for including video of the issue/resolution, it makes reviewing much easier 🙂 |
|
Should be resolved now, I've switched to navigationBar insets and tested on-device to make sure it's still working as expected 👍 |
shell/platform/android/io/flutter/plugin/editing/ImeSyncDeferringInsetsCallback.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for making that change and testing it out! This looks good to me, passing to @reidbaker for final review.
Additional context for reading the PR:
The flags set by the different UI modes are defined here - https://github.com/flutter/engine/blob/main/shell/platform/android/io/flutter/plugin/platform/PlatformPlugin.java#L302
shell/platform/android/io/flutter/plugin/editing/ImeSyncDeferringInsetsCallback.java
Show resolved
Hide resolved
shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java
Show resolved
Hide resolved
|
Hey, just following up to see if you are still planning to address Reids comments. |
|
Sorry I've been meaning to get back to this for a while, I'll try to address the comments in the next few days. |
|
Any updates? If this is still WIP, please convert it to a draft so triagers don't bother you every week :) |
|
FYI, #40867 is on its way. After both PRs will land, flutter bottom inset animation on keyboard opening will work as expected on both platforms. |
|
I'm currently exploring possible solutions that don't depend on deprecated APIs per @reidbaker's comment, I'll set this as a draft for now whilst I continue to investigate. |
|
@reidbaker if the non-deprecated path is already mastered by you, would it be possible for you to try to make time and help integrating it? That would speed thing up |
|
I think that is a question for @gmackall. |
|
Thx @reidbaker if you notice this might take more time than expected, can it be considered to merge PR just to mitigate clients pain and at same time move the pending part to remove deprecated code to a new PR? Asking it because issue is severe and also Lucky said me that #40867 is almost ready, he is just testing it a bit. |
|
@JonathanPeterCole yes that is the issue that would still require a fix. A similar work has been done for ios with #40867 If possible, the idea is to have this merged with a note about the deprecated api, so that we can keep debugging on master. Issue is painful and since landing an ultimate fix might take some time, apps would meantime benefit from a not-perfect-yet-decent keyboard animation. The fix for ios took 1yr, we are confident that the android side would be easier but we are not sure. |
|
@reidbaker I suggested to merge this PR so that we can keep debugging issue on master. Another PR will address deprecated api and the other issue mentioned above when it is ready |
@reidbaker I’ve misunderstood this. It is perfect 👍 If you also want to file an issue, we will refer to it for next PR. @JonathanPeterCole can you please move this out of draft so it can be merged? |
|
CC: @CoolDude53 |
|
@reidbaker @gmackall Sorry for the delays on this one, it should now be ready for a re-review |
|
First pass on the re review looks good. @gmackall can you checkout this code and run it on a real device before approving. |
Will try testing on a device this afternoon and update with results |
|
Tested on a samsung galaxy s21 on both master and with this patch, and was able to both recreate the issue on master and confirm that this patch fixes it. The changes to tests since my last approval also look good, so re-adding my approval. |
| // out behind the navigation bar aren't present. | ||
| int excludedInsets = 0; | ||
| int systemUiFlags = view.getWindowSystemUiVisibility(); | ||
| if ((systemUiFlags & View.SYSTEM_UI_FLAG_LAYOUT_HIDE_NAVIGATION) == 0 |
There was a problem hiding this comment.
Without committing you to building it do you think we need to do a similar evaluation for status bar and the top insets?
There was a problem hiding this comment.
I don't think that'll be necessary as the status bar visibility and top insets didn't affect the keyboard animation in my testing.
There was a problem hiding this comment.
But it would impact any animation coming from the top like keyboards impact animations coming from the bottom. Right? If for example the status bar was present our code would behave differently than if status bar was overlayed. I am thinking of flutter/flutter#118761
There was a problem hiding this comment.
Ah interesting, I'm not sure on this one but WindowInsetsController.BEHAVIOR_SHOW_TRANSIENT_BARS_BY_SWIPE is one of the new APIs introduced in API 30, so I wonder if it's a conflict with the deprecated system UI flags currently in use, rather than an issue with keyboard animation.
reidbaker
added
the
autosubmit
…126309) flutter/engine@8d3a816...824cd09 2023-05-09 [email protected] Increase timeout of orchestrator. (flutter/engine#41839) 2023-05-09 [email protected] [fuchsia] Stop calling FIDL from Dart in Flutter integration tests (flutter/engine#41669) 2023-05-09 [email protected] [tests] Remove unused fuchsia.sys protocol reference (flutter/engine#41826) 2023-05-09 [email protected] Roll Skia from 7736fbaf84f0 to 485cd3d0f9ca (6 revisions) (flutter/engine#41840) 2023-05-09 [email protected] [Impeller] introduces DeviceHolder to avoid accessing a dead Device (flutter/engine#41748) 2023-05-08 [email protected] Get rid of "outrageous" default text styles for HTML renderer. (flutter/engine#41822) 2023-05-08 [email protected] Adjust DL filter bounds tests to not rely on exact Skia results (flutter/engine#41792) 2023-05-08 [email protected] Roll Dart SDK from a8b6687327d6 to 498cfa57165b (1 revision) (flutter/engine#41823) 2023-05-08 [email protected] [Android] Fix incorrect viewInsets during keyboard animation with EdgeToEdge (flutter/engine#39391) 2023-05-08 [email protected] Roll Skia from 951eb9653163 to 7736fbaf84f0 (1 revision) (flutter/engine#41821) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
5 participants
Currently during the keyboard animation, the navigation bar insets are subtracted from the keyboard insets. This is correct when the app isn't laid out behind the navigation bar, but results in incorrect viewInsets when the app's running in edge-to-edge or fullscreen.
This change checks if the app is being laid out behind the navigation bar and adjusts the bottom insets accordingly during the keyboard animation.
Fixes flutter/flutter#89914
Tested on Android 13 (Pixel 7) using the code sample here: flutter/flutter#109623
Before
Before.mp4
After
After.mp4
Pre-launch Checklist
///).