Multiple iPhone X & iOs11 Fixes

[tennist]

The following changes address some concerns I had as mentioned in issue #186 and issue #218 . This also solves the problem @joe-scotto is having in issue #228 .

I got rid of the iOS 7 hack altogether. This really wasn't an iOS 7 hack, but rather a hack to make this plugin work with a feature of the statusbar plugin. As I mentioned before, it is apples intention to have your app flood below the status bar. It is very easy to add a div to the top of your webview or padding to the top of your header that moves your content below the statusbar. See this link for instructions: https://blog.phonegap.com/displaying-a-phonegap-app-correctly-on-the-iphone-x-c4a85664c493

I was able to create an addition sub view called _safeAreaBackgroundView that only gets initialized, sized, and positioned if the device is running iOS 11 or greater, the ad banner is at the bottom, and the ad banner doesn't overlay the webview. I set the background color of this view to black. We might be able to work on this in the future and allow the user to set a preference for the background color somewhere in their own code. For now, I think black will be compatible with most people's devices.

None of my apps use banner ads at the top or banner ads that overlay. There may need to be some additional work to make these types of ads position properly on the iPhoneX. From what I can see now, all ads (overlay and not) at the top might be beneath the statusbar. Also, ads at the bottom that overlay might need to be shifted up on the iPhoneX as well.

[joe-scotto]

How would I control the color of the background using this?

[tennist]

Right now there is no way to control the color. The last update switched the superview background color from the default of white to black. They did this because of the new space created for the safe area at the bottom of the iPhone X. I commented that line of code and instead used a small sub view with a black background to fill this area. In doing this the default white background of the superview can remain so that things are back to normal if the superview shows through in other places of your app. It would be nice to create a banner preference to set the background color of this new view to whatever jives with a particular app, however I don’t have tune to dive into that right now and black seems to work great in all apps I have since it blends with the edges of the screen.

[MBuchalik]

In my opinion, your PR is a good idea. However, there are a few things to do. (Please note that I am not an ObjC developer, so I might be mistaken at some parts.)

White bars at left and right

Have a look at this screenshot: Here, you can see that the bottom part is black (as desired). However, the left and right part is white. I think this is the superview's background color. If I saw it correctly, the safeAreaBackgroundView is not a superview of the actual banner. Maybe, it would be possible to place the banner inside this black background view? Or, you could stretch the black background to have the same height as (banner + safeAreaInsets.bottom)?

Removing the banner

Currently, when removing a banner, the black background stays at the bottom. I don't know if a CGRect has some sort of "hide me temporarily" method. If not, maybe setting its height and width to 0 could help.

What do you think about these two problems?

[tennist]

@MBuchalik the white bars you see left and right are in fact the superview showing through due to the ad width being smaller than the screen width in landscape mode on the iPhone X. This width is narrowed on purpose here to keep the ad out of the safe zones. We could creates some additional views to fill these areas with a particular color, but I don't think we can come up with a good color to set here since the ad colors will always be different. When a white ad pops up, the white bars blend nicely, but when a blue ad pops up both the black and white bars look bad. Perhaps there is a way to determine the ad's background color and then use that to set the background color of these bars, but that is above my experience level in ObjC. For now, I think this is just something people will have to live with with unless they overlay their banner ads.

I got rid of the ternary operator and believe I coded a solution to removing the safeAreaBackground when a banner is removed. I do not have any apps in which the banner gets removed to test this in however. Please test this in your app. If everything tests out I think it would be good to get these changes released in a new version of the plugin.

[ratson]

@tennist Thank you for creating the PR, I will testing out the changes as soon I got time, probably this weekend.

@MBuchalik Do you think this is better than the current one? Or do we need to keep the old behavior available in case someone need it?

[MBuchalik]

Thanks for your changes!

My older comment that is not valid anymore

The ternary operator is actually this one: ``` ObjC self.bannerOverlap ? self.webView : [self.webView superview]; ``` You can replace the whole line ``` ObjC UIView* parentView = self.bannerOverlap ? self.webView : [self.webView superview]; ``` with ``` ObjC UIView* parentView = [self.webView superview]; ``` The if statement ( ``` if (!bannerOverlap && ! bannerAtTop){ ...} ``` ) actually has to be kept there. Sorry if I wasn't clear enough in my comment. The whole part could look like this: ``` ObjC if (!bannerOverlap && ! bannerAtTop){ if (@available(iOS 11.0, *)) { UIView* parentView = [self.webView superview]; // The rest of the code here. ```

Edit: Oh dear, I have missed something. Look at where "initializeSafeAreaBackground" is called. Here, we don't have the information whether the banner is actually overlapping the view or not. We only get this information after a call from the JS side is made. So you can ignore the collapsed comment above.

How about this? You create the black background in initializeSafeAreaBackgroundView but set the visibility to hidden by default. When a new banner is requested, it will be shown when it is necessary (it is a bottom banner and not overlapping). That's what you are already doing in resizeViews. The only thing that you need to do now: If it is not an overlapping banner or not a bottom banner, hide the black view in resizeViews.

So:

• Set the visibility of the view in initializeSafeAreaBackgroundView to hidden by default. You only have to change this and nothing else here.
• Set the black view to hidden in resizeViews. Only if it is a non-overlapping bottom banner, set it to visible.

I think (and hope) this can solve the problem. I hope you understand my comment - if not, please don't hesitate to ask! I have definitely missed this part. Sorry about that.

Today, I cannot make any tests. I will do so in the next days and report what I found out. :smiley:

I am still not sure if the white area is a good idea. In my solution in https://github.com/ratson/cordova-plugin-admob-free/pull/186 , I used a black area for the bottom and left/right part. In my opinion, this looks "cleaner" (no matter which kind of ad you see) because it is one solid, connected "piece". When using a white background, there are now two colors. But this is probably only my opinion... @ratson what do you think about this?

@ratson: In general, this solution looks better to me since it doesn't set the superview's background color. Because of this, it doesn't interfere with other plugins. I don't think we need to keep the old behavior. My only concern is the white area (like I said above).

[tennist]

@MBuchalik & @ratson I have incorporated all of the reviews thus far into my PR. The only issue remaining is white vs. black background behind the ads. This really comes to a preference here. I personally prefer white because my apps have white backgrounds and it makes it feel like the ad is part of my app vs part of the phone. I can see where black could be preferred by some however so that it blends with the safe area background I created below it.

Again maybe we could turn this into a preference somehow or have it pull the background color from the ad or app. I am confident I could easily code an additional view that pulls the background color from the webview, but I do not know about pulling the color from the ad. The complication with ads is that some of them are pictures instead of text ads so the background color isn't applicable.

[MBuchalik]

I have now tested your changes with my (limited) configuration. Unfortunately, hiding the black background doesn't work. My approach to fix this (at least it works in my case) is the following. You only have to look at resizeViews.

1. Remove lines 721-724

   //If the ad is not showing turn the safeArea Background off
   if(self.bannerView.hidden && ! _safeAreaBackgroundView.hidden){
   _safeAreaBackgroundView.hidden = true;
   }

2. In line 707 the following if statement starts:

if( self.bannerView ) {

Find the end of this statement (I think it is in line 779) and add an else statement that hides the black background.

if( self.bannerView ) {
    // The whole code from line 708 to 778)
} else {
    _safeAreaBackgroundView.hidden = true;
}

3. Now we have to do something pretty similar to 2. In line 730, the following if statement can be found:

   if( adIsShowing ) {

   Find the end of this statement (I think it is in line 778) and add an else statement that hides the black background.

if( adIsShowing  ) {
    // The whole code from line 731 to 777)
} else {
    _safeAreaBackgroundView.hidden = true;
}

I hope this solves the issue. (And I hope that I haven't messes up the line numbers and brackets and so on :smile:)

Regarding the white background

I still think that a black background on the left and right would look better. But this is, as I said, just my personal opinion. So if @ratson descides that he also likes the white one, I am ok with that and will use this in my apps for now. I dont't know any way to fetch the backround color of an ad. So the only thing we could do here is, as you said, to add an option that sets the background color (either of the superview, or of an additional "banner container"). I am not sure if we should do that in this PR or just leave it like it is. @ratson what do you think?

[ratson]

@tennist @MBuchalik I think force the current users to change the background color is not nice, it will better to make configurable for people to opt-in, so let's keep the background not changed for this PR. Leave future improvement for adding configurable background color if there is a need for it.

[joe-scotto]

@ratson I haven't been following this too closely so correct me if I'm wrong, but I believe allowing us to change the background color of the adspot would be great. It looks horrible with the white background default on iOS (iPhone X). I would like it to match the overall theme of my app.

[tennist]

@MBuchalik I incorporated your suggested changes for hiding the new view.

@ratson I agree that we should leave the background color at default for now and then add functionality for setting this color later on down the road. Lets go ahead and commit this PR for now and I will start a new one when I get around to playing with background colors.

If everyone is agreeable I can create another view behind the banner ad the pulls the background color from the web view. In this way it would be white if people are just using the default or it would pull whatever color they are using in their app. I think this is what @joe-scotto would prefer.

Another thought is to just make the view I created taller so that it extends behind the ad and then set its background color from the web view. This would make it so that the same app color is carried across the screen and provide the app-feel that Apple is looking for. I don't have a personal preference here as my apps use a white background and any color looks ok behind the safe area and ads. I do prefer white behind the ads though. I am sure I could figure out how to code this as a configurable preference as well so that it pulls the webview color by default, but can be configured to something else when creating the banner.

[MBuchalik]

Thank you for adding the suggested changes. I would like to wait for @ratson do some tests - I have only tested a really limited scenario. In my opinion, it is better to do more, detailed tests just to be sure that no errors occur with other parts of the plugin.

Regarding the background: In my opinion, stretching the (currently black) background so that the whole banner is covered by it would be a good idea. I think that an additional setting for the background color is a good solution. In this way, every developer is able to design the view as he likes.

[lone-cloud]

@tennist did you get a chance to test interstitial ads when working on this PR? Did you test with both iOS webviews? Based on my testing the page does not get displayed correctly (page overflows to the ad banner area) after an interstitial ad is shown and the cordova statusbar plugin is installed.

[tennist]

There are conflicts between this plugin and the statusbar plugin on ios. Particularly if you are using with the feature of the statusbar plugin to not overlay the webview. The issue is that both plugins are trying to resize the webview in order to fit the statusbar or resize the web view to try and fit the ad. Depending on which plugin fires last is what the webview will end up showing. If you are using the status bar not overlaying the webview feature of the status bar plugin, I recommend turning that off. Just add a div at the top of your webview that is the color you want and is the height of the statusbar.

This feature in the statusbar plugin should be deprecated as it is too easy to implement what it does with some basic html and css.

https://blog.phonegap.com/displaying-a-phonegap-app-correctly-on-the-iphone-x-c4a85664c493

[lone-cloud]

What you say sounds reasonable, but unfortunately it does not work for me in practice. When debugging the simulator webview via Safari, in a build that has cordova-plugin-statusbar installed and a bottom ad banner from this plugin, I see that the <html> element height actually increases by 50px (height of the banner I'm guessing), but ONLY after the interstitial ad is displayed. This causes the overlay bug and makes it impossible to work around using pure HTML+CSS. I do have <preference name="StatusBarOverlaysWebView" value="true" /> set up for the statusbar.

The only real workaround that I've found around this issue is to manually call admob.banner.show() which causes the page to re-render correctly, but unfortunately it cannot be called automatically since the "admob.interstitial.events.CLOSE" event seems to never fire in iOS.

[tennist]

@hoboman313 I think I may have a solution, but do not have interstitials setup to test with. In the file /src/ios/CDVAdMob.m please add the following code just after line 865 (inside the if statement):

[self resizeViews];

You will have to remove the ios platform from your app and then add it back and rebuild for the change to propagate. Let me know if this solves the issue you are having.

[lone-cloud]

Unfortunately it did not work. I debugged in xcode and the core issue here seems to be that interstitialDidDismissScreen() never gets called (also tested with interstitialWillDismissScreen()) so your proposed solution never got run. This is very weird because the other callbacks around it do get called (ex. interstitialWillPresentScreen()). This is my first time looking at Objective C so I could not figure it out. Will keep digging. Perhaps one of the previous iOS Admob SDK updates that @ratson ran broke something? Dunno.

[tennist]

It may be that google changed some things in the latest version of the sdk. I think I may have found the issue based on google's documentation. In the same file as before change the lines in the code as follows:

Line 838 to: - (void)interstitialWillLeaveApplication:(GADInterstitial *)ad {

Line 844 to: - (void)interstitialDidReceiveAd:(GADInterstitial *)ad {

Line 854 to: - (void)interstitialWillPresentScreen:(GADInterstitial *)ad {

Line 860 to: - (void)interstitialDidDismissScreen:(GADInterstitial *)ad {

If changing these lines doesn't fix also try adding my last suggestion in combination with these changes.

[lone-cloud]

I got to the bottom of my statusbar issues tonight.

1.) Preparing an interstitial while one was already being displayed prevented the CLOSE event from ever being fired. Otherwise the events fire just as well without the changes described in the previous comment.

2.) The [self resizeViews]; fix actually works quite well and fixes the overlay issue without having to rely on calling admob.banner.show() in the CLOSE callback.

[tennist]

I will add this line of code in my next pull request. I have been working on a solution to some other display issues for banners at the top as well.