-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[webview_flutter_android] Adds support to opt out of Android inset changes #11192
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
Changes from 13 commits
55c4ce0
6bb5a99
8e10a01
3b3d6c6
32856f9
ce2a9b3
bc04474
57ca57c
49a3867
74bfe0f
cfdf061
07c8699
31fa299
a8a415b
b367d63
77b40da
1bb6433
4d02edb
bb20265
f42bb7b
651a4f8
204e405
681eb73
6a6b394
c93e0e6
4e1915d
819f6d1
bdf08ae
5c89d64
c1511d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,6 +6,10 @@ | |||||||||||||||||||
|
|
||||||||||||||||||||
| import android.view.View; | ||||||||||||||||||||
| import androidx.annotation.NonNull; | ||||||||||||||||||||
| import androidx.core.graphics.Insets; | ||||||||||||||||||||
| import androidx.core.view.ViewCompat; | ||||||||||||||||||||
| import androidx.core.view.WindowInsetsCompat; | ||||||||||||||||||||
| import java.util.List; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Flutter API implementation for `View`. | ||||||||||||||||||||
|
|
@@ -67,4 +71,36 @@ public void setOverScrollMode(@NonNull View pigeon_instance, @NonNull OverScroll | |||||||||||||||||||
| throw getPigeonRegistrar().createUnknownEnumException(OverScrollMode.UNKNOWN); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @Override | ||||||||||||||||||||
| public void setInsetListenerToSetInsetsToZero( | ||||||||||||||||||||
| @NonNull View pigeon_instance, @NonNull List<? extends WindowInsets> insets) { | ||||||||||||||||||||
| int insetsTypeMask = 0; | ||||||||||||||||||||
| for (WindowInsets inset : insets) { | ||||||||||||||||||||
| switch (inset) { | ||||||||||||||||||||
| case SYSTEM_BARS: | ||||||||||||||||||||
| insetsTypeMask |= WindowInsetsCompat.Type.systemBars(); | ||||||||||||||||||||
| break; | ||||||||||||||||||||
| case DISPLAY_CUTOUT: | ||||||||||||||||||||
| insetsTypeMask |= WindowInsetsCompat.Type.displayCutout(); | ||||||||||||||||||||
| break; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| final int finalTypeMask = insetsTypeMask; | ||||||||||||||||||||
| ViewCompat.setOnApplyWindowInsetsListener( | ||||||||||||||||||||
| pigeon_instance, | ||||||||||||||||||||
| (view, windowInsets) -> { | ||||||||||||||||||||
| if (finalTypeMask == 0) { | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed the logic to not need to make this check since it is bug prone. Now the code checks if the list is empty and handles it at the top. |
||||||||||||||||||||
| return windowInsets; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| final Insets allInsets = windowInsets.getInsets(finalTypeMask); | ||||||||||||||||||||
|
bparrishMines marked this conversation as resolved.
Outdated
|
||||||||||||||||||||
| pigeon_instance.setPadding( | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does setting the padding here on the pigeon_instance do?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I reread the opt out doc and my interpretation is that the padding used in that doc were just an example of how to manually handle the insets. So I removed the padding added in this PR since these seem to be handled by Flutter. The fix still works without it. |
||||||||||||||||||||
| allInsets.left, allInsets.top, allInsets.right, allInsets.bottom); | ||||||||||||||||||||
| return new WindowInsetsCompat.Builder(windowInsets) | ||||||||||||||||||||
| .setInsets(finalTypeMask, Insets.NONE) | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this line is saying to copy the existing windowInsets then to set all the ones we care about "consuming" to If that is correct i think this line could be made more readable with the following changes.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code isn't intended to indicate what is handled by Flutter. I added comments about the selected inset types in My original idea was to expose this setting to the user, so they can have control over this. But I changed it to only handle this internally in the plugin instead for now since this may be a general change that we need to make for every |
||||||||||||||||||||
| .build(); | ||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment in However, applying padding to the native 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 Could you clarify if this padding is intended? If not, these lines should be removed, and the corresponding test in
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed that an edge-edge WebView still works.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you confirm the behavior of a full screen flutter web view on a device with a simulated or actual tall cutout. |
||||||||||||||||||||
| }); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only added those two because they were the only ones that we are interested in using for this opt out. I went ahead and added the remaining values for the enum. The caps for
SYSTEM_BARS,DISPLAY_CUTOUT, etc. are the generated names for theWindowInsetsenum created by pigeon.I just changed them to
WindowInsetsTypeto make it more accurate though.