-
Notifications
You must be signed in to change notification settings - Fork 369
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
Android Fullbleed IAMs #1481
Android Fullbleed IAMs #1481
Conversation
e24b912
to
580ffdd
Compare
No Margin IAMs now are full screen covering the status bar. This commit does not yet adjust the IAM content to account for this
Note that this doesn't override the App's display cutout window settings meaning that if the App doesn't support content bleeding under the cutout then the IAM will not go under the cutout.
also testing fullscreen windowManager flags in the demo app
Just a snapshot of it working, don't desinged to be merged. Will create another branch to follow up with a commit of only then changes needed.
Other cleanup as well
580ffdd
to
4d1001e
Compare
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.
Reviewed 2 of 7 files at r1, 9 of 9 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @emawby, @Jeasmine, and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/InAppMessageView.java, line 411 at r3 (raw file):
cardView.setClipToPadding(false); cardView.setPreventCornerOverlap(false); cardView.setCardBackgroundColor(Color.TRANSPARENT);
This change was already done in PR #1482
OneSignalSDK/onesignal/src/main/java/com/onesignal/WebViewManager.java, line 242 at r3 (raw file):
private void handleResize() { if (messageContent.isFullBleed()) { updateSafeAreaInsets();
Why is this dependent on a JS resize event? Shouldn't we set this as soon as possible after the JS is ready to receive it?
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @emawby, @Jeasmine, @jkasten2, and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/WebViewManager.java, line 242 at r3 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Why is this dependent on a JS resize event? Shouldn't we set this as soon as possible after the JS is ready to receive it?
This for adjusting the insets when the device orientation changes or something else causes the window to resize. The initial insets are already set in the HTML.
Are you thinking there is a better place to update the insets on orientation change?
webView.evaluateJavascript methods should be run on the main thread
65f65d0
to
b53858c
Compare
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.
Reviewable status: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @Jeasmine, @jkasten2, and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/InAppMessageView.java, line 411 at r3 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
This change was already done in PR #1482
Done.
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.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @emawby, @Jeasmine, and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/WebViewManager.java, line 242 at r3 (raw file):
Previously, emawby (Elliot Mawby) wrote…
This for adjusting the insets when the device orientation changes or something else causes the window to resize. The initial insets are already set in the HTML.
Are you thinking there is a better place to update the insets on orientation change?
Yes, thinking insets should be set when the IAM, Activity is resumed, or an orientation change.
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.
Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @Jeasmine, @jkasten2, and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/WebViewManager.java, line 242 at r3 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Yes, thinking insets should be set when the IAM, Activity is resumed, or an orientation change.
Gotcha I removed it. I was already calling it calculateHeightAndShowWebViewAfterNewActivity
which is called on orientation change and makes sense to call update insets here
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.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Jeasmine and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/WebViewManager.java, line 242 at r3 (raw file):
Previously, emawby (Elliot Mawby) wrote…
Gotcha I removed it. I was already calling it
calculateHeightAndShowWebViewAfterNewActivity
which is called on orientation change and makes sense to call update insets here
👍
* Revert the focusable property of the PopupWindow that displays an IAM view to be false. This was added as part of fullbleed IAM implementation in #1481, but does not appear to be necessary. * This allows banner-style top and bottom IAMs to remain on the screen while the user interacts with the app. * Otherwise, currently, tapping on the background dismisses the banner-style IAM.
Description
One Line Summary
Added support for fullbleed in app messages to the Android SDK.
Details
Fullscreen and Carousel In App Messages that do not have margins will now bleed under the status bar to be completely full screen.
The dashboard has added the ability for fullscreen and Carousel IAMs to be created without margins. When doing so it sets
remove_height_margin
andremove_width_margin
to true instyles
(this was already being read by the SDK). Currently we don't support only width or height removal so I am consolidating these flags into an isFullscreen boolean that controls the fullbleed behavior.In order to handle notched devices we need to tell the Javascript about the device insets so that IAM content doesn't overlap with notches and the status bar. To do this we are modifying the IAMs HTML and calling setSafeAreaInsets with a javascript object of the form
Version specific notes
Fullbleed is accomplished in two different ways. For API < 30 we use
For API >=30 we use
webView.setFitsSystemWindows(false);
Additionally landscape mode behaves differently in API 29 than all other versions. In API 29 the IAM will bleed under notches whereas in all other versions the IAM will not bleed under. This means we only account for left+right notch insets for API 29.
Orientation Change
We also need to call the setSafeAreaInsets() method again if the device is rotated since the side of the notches have changed. To do so we are now responding to the resize javascript event (previously unused).
Misc Changes
In order for the IAM to bleed under the status bar we needed to change the popupWindow's display type to
PANEL
. If the IAM is not fullbleed we will still useattachedDialog
. Additionally we must setclippingEnabled
to false.Motivation
Customer request for full screen IAMs.
Background Context
When we initially discussed removing margins from IAMs we discussed the future ability to remove just width or height margins so we split them into their own booleans. We eventually decided against that approach and to just focus on fullbleed IAMs.
Scope
This change involves coordinating new behavior between the SDK and IAM Javascript. It makes no new API calls and provides no new public APIs in the SDK. This change should not affect non fullscreen/carousel IAMs.
Testing
Unit testing
✅ All tests pass after the below modifications
We don't have infrastructure for testing visual properties of IAMs currently or good ways to test a messages HTML content.
Manual testing
✅ Tested on various Android emulators and phsyical devices for Android API versions 27-31 with and without notches. Test both carousels and fullscreen IAMs. Tested both landscape and portrait.
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is