[webview_flutter_android] Adds opt out of displayCutout and systemBars Android inset changes#11192
[webview_flutter_android] Adds opt out of displayCutout and systemBars Android inset changes#11192bparrishMines wants to merge 13 commits intoflutter:mainfrom
displayCutout and systemBars Android inset changes#11192Conversation
displayCutout and systemBars Android inset changes
|
@flutter/android-reviewers This adds an opt out to newly supported insets on chromium. My assumption for this PR is that these insets are handled by Flutter (similar to iOS), but wanted to ensure this is fine with Android team members. |
There was a problem hiding this comment.
Code Review
This pull request introduces an opt-out mechanism for displayCutout and systemBars Android inset changes in webview_flutter_android. The changes involve updating Pigeon files to add a new method for setting an inset listener, implementing this logic on the Android native side, and calling it from the Dart AndroidWebViewController. The tests have been updated accordingly.
My main feedback concerns the implementation of the inset listener on the Android side, where padding is being applied to the View. This seems to contradict the goal of letting Flutter handle safe areas, as stated in the comments and PR description. I've left a specific comment with a suggestion to address this.
Note: Security Review did not run due to the size of the PR.
| final Insets allInsets = windowInsets.getInsets(finalTypeMask); | ||
| pigeon_instance.setPadding( | ||
| allInsets.left, allInsets.top, allInsets.right, allInsets.bottom); | ||
| return new WindowInsetsCompat.Builder(windowInsets) | ||
| .setInsets(finalTypeMask, Insets.NONE) | ||
| .build(); |
There was a problem hiding this comment.
The comment in android_webview_controller.dart states that "Flutter handles these internally", and the PR description mentions that the desired behavior is similar to iOS's contentInsetAdjustmentBehavior = .never. Both of these suggest that the web content should extend into the safe areas (i.e., underlap system bars and cutouts).
However, applying padding to the native View here will shrink the web content area, preventing it from drawing into the safe areas. This seems to contradict the intended goal of letting Flutter manage the safe area.
If the intention is to let Flutter handle safe areas, you should probably only consume the insets to prevent the WebView from applying them, but not apply any padding to the View itself. This would allow the web content to render edge-to-edge.
Could you clarify if this padding is intended? If not, these lines should be removed, and the corresponding test in ViewTest.java should be updated.
| final Insets allInsets = windowInsets.getInsets(finalTypeMask); | |
| pigeon_instance.setPadding( | |
| allInsets.left, allInsets.top, allInsets.right, allInsets.bottom); | |
| return new WindowInsetsCompat.Builder(windowInsets) | |
| .setInsets(finalTypeMask, Insets.NONE) | |
| .build(); | |
| return new WindowInsetsCompat.Builder(windowInsets) | |
| .setInsets(finalTypeMask, Insets.NONE) | |
| .build(); |
There was a problem hiding this comment.
Confirmed that an edge-edge WebView still works.
There was a problem hiding this comment.
Can you confirm the behavior of a full screen flutter web view on a device with a simulated or actual tall cutout.
You can test on any physical device (or maybe emulator) by running adb shell cmd overlay enable com.android.internal.display.cutout.emulation.tall
reidbaker
left a comment
There was a problem hiding this comment.
At a high level I like that we are opting out of chromes auto system bars instead of creating a path to ignore our own insets logic when showing in chrome. I need to get this a more through review but directionally I am for this change.
reidbaker
left a comment
There was a problem hiding this comment.
If you have screenshots of the before and after behavior could you include them in this pr? If you dont and it is cumbersome to get them then feel free to ignore this request.
| public void setInsetListenerToSetInsetsToZero( | ||
| @NonNull View pigeon_instance, @NonNull List<? extends WindowInsets> insets) { | ||
| int insetsTypeMask = 0; | ||
| for (WindowInsets inset : insets) { |
There was a problem hiding this comment.
Can you add comment here that indicates why SYTEM_BARS and DISPLAY_CUTOUT where chosen and how to know when a new item should be added to this list.
Additionally why use the all caps ints instead of WindowInsets.Type.systemBars()?
| ViewCompat.setOnApplyWindowInsetsListener( | ||
| pigeon_instance, | ||
| (view, windowInsets) -> { | ||
| if (finalTypeMask == 0) { |
There was a problem hiding this comment.
Can you add a comment explaining that this is checking to see if we set any bitwise values to make the code easier to skim without having someone need to understand why we are comparing to zero.
| } | ||
|
|
||
| final Insets allInsets = windowInsets.getInsets(finalTypeMask); | ||
| pigeon_instance.setPadding( |
There was a problem hiding this comment.
What does setting the padding here on the pigeon_instance do?
| return windowInsets; | ||
| } | ||
|
|
||
| final Insets allInsets = windowInsets.getInsets(finalTypeMask); |
There was a problem hiding this comment.
I allInsets here is actually only the set of insets we care about ignoring. Can you help me understand how it is different from windowInsets at this line and also how it is more "all" than just the window insets?
| return new WindowInsetsCompat.Builder(windowInsets) | ||
| .setInsets(finalTypeMask, Insets.NONE) |
There was a problem hiding this comment.
I think this line is saying to copy the existing windowInsets then to set all the ones we care about "consuming" to Insets.NONE.
If that is correct i think this line could be made more readable with the following changes.
| return new WindowInsetsCompat.Builder(windowInsets) | |
| .setInsets(finalTypeMask, Insets.NONE) | |
| // Consume insects that are already handled by flutter. | |
| return new WindowInsetsCompat.Builder(windowInsets) | |
| .setInsets(activeInsetsHandledByFlutter, Insets.NONE) |
There was a problem hiding this comment.
Additionally if we cant define a list to import in both locations I think the code that is "already" handling the insets should be cross linked. As a result the case statements probably become another loop.
| final Insets allInsets = windowInsets.getInsets(finalTypeMask); | ||
| pigeon_instance.setPadding( | ||
| allInsets.left, allInsets.top, allInsets.right, allInsets.bottom); | ||
| return new WindowInsetsCompat.Builder(windowInsets) | ||
| .setInsets(finalTypeMask, Insets.NONE) | ||
| .build(); |
There was a problem hiding this comment.
Can you confirm the behavior of a full screen flutter web view on a device with a simulated or actual tall cutout.
You can test on any physical device (or maybe emulator) by running adb shell cmd overlay enable com.android.internal.display.cutout.emulation.tall
For WebView versions >=144, support has been added for displayCutout() insets and systemBars() insets. This is causing WebViews to incorrectly report that it is obscured by a system bar or display cutout as demonstrated in this issue.
This adds the opt out for inset changes as explained in this chromium doc. It seems Flutter handles safe areas for platform views, so the
WebViewcan zero out inset changes to the View.iOS does something similar. And it also sets UIScrollView.contentInsetAdjustmentBehavior to
Never. My assumption was that this was never done for Android, becauseWebViews didn't receive these inset changes until this version.Fixes flutter/flutter#182208
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2