Skip to content
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

(ios) Remove fake status bar with hardcoded height to fix issues in iOS devices with a notch #656

Merged
merged 4 commits into from
Jun 4, 2020

Conversation

PDLMobileApps
Copy link
Contributor

Platforms affected

iOS

Motivation and Context

IAB adds what has been referred as a "fake status bar" that creates issues on devices with a notch, such as iPhone X/XS/XM, etc.
In order to fix such issues, the fake status bar has to be removed.

Fixes: #604, #638, #301

Related to: #546, #265

Description

The fake status bar is removed from the IAB window and both height and the position of the IAB window is modified to account for the (real) iOS status bar. That is needed to correctly display the content of the iOS status bar without overlapping the content in the IAB window in portrait mode.
The color of the iOS status bar is set to transparent to match the background color of the underlying Cordova app.

Testing

Testing was done manually as I am not sure on how to automate it.

  1. Created a new Cordova test app using cordova-plugin-inappbrowser.

  2. Executed:
    cordova.InAppBrowser.open(url, "_blank", "");
    and verified that the iOS status bar is shown around the notch and the web view is displayed just below the iOS status bar (default behavior for iOS apps). The status bar color matches the background color of the underlying Cordova app.

  3. Executed:
    cordova.InAppBrowser.open(url, "_blank", "toolbar=yes,toolbarposition=top");
    and verified that the iOS status bar is shown around the notch, the toolbar is displayed just below the iOS status bar and the web view is displayed just below the toolbar (default behavior for iOS apps). The status bar color matches the background color of the underlying Cordova app.

  4. Executed:
    cordova.InAppBrowser.open(url, "_blank", "toolbar=yes,toolbarposition=top");
    on a device without notch and verified that the iOS status bar is shown on the top of the screen, the toolbar is displayed just below the iOS status bar and the web view is displayed just below the toolbar (default behavior for iOS apps). The status bar color matches the background color of the underlying Cordova app.

The tests above were executed for both portrait and landscape orientation. Note that in landscape mode, no status bar is displayed, which is the default iOS behavior for that mode with any app.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@jcesarmobile
Copy link
Member

what's the point of keeping the bgToolbar if you just set it to clearColor? shouldn't you just remove all the references to it?

@PDLMobileApps
Copy link
Contributor Author

what's the point of keeping the bgToolbar if you just set it to clearColor? shouldn't you just remove all the references to it?

I removed the bgToolbar and retested: everything worked fine. I am going to commit the change. Thanks for pointing it out.

Copy link
Member

@NiklasMerz NiklasMerz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a quick test and the code looks good to me. Thank you for your contribution.

I found a small bug. If you open an InAppBrowser window and turn your iPhone (no notch) from portrait to horizontal the statusbar height is not changed and you can see your app below the IAB window. If you start IAB in horizontal and turn your phone the statusbar is white in my case and you cannot read the text.

Looks like the views need to reposition in this case.

@PDLMobileApps
Copy link
Contributor Author

I did a quick test and the code looks good to me. Thank you for your contribution.

I found a small bug. If you open an InAppBrowser window and turn your iPhone (no notch) from portrait to horizontal the statusbar height is not changed and you can see your app below the IAB window. If you start IAB in horizontal and turn your phone the statusbar is white in my case and you cannot read the text.

Looks like the views need to reposition in this case.

Thanks for finding this issue. It should be fixed in my last commit. I also refactored/simplified the code. In my tests if worked fine with devices with and without notch, with top toolbar enabled and disabled.

@NiklasMerz NiklasMerz linked an issue Apr 3, 2020 that may be closed by this pull request
Copy link
Member

@NiklasMerz NiklasMerz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests on non-notch iPhone and notch Simulator are good for me. Code looks good but someone else should have a look. :-)

@PDLMobileApps PDLMobileApps requested a review from NiklasMerz April 9, 2020 14:24
Copy link
Member

@NiklasMerz NiklasMerz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good from my side 👍

@NiklasMerz
Copy link
Member

NiklasMerz commented Apr 9, 2020

@PDLMobileApps Could you please resolve the conflict? I don't want to break it :-)

I would like to invite others to review before merging it first. Sorry it might take some time for people to find time. I will add some people that understand this better.

@PDLMobileApps
Copy link
Contributor Author

@PDLMobileApps Could you please resolve the conflict? I don't want to break it :-)

I would like to invite others to review before merging it first. Sorry it might take some time for people to find time. I will add some people that understand this better.

Sure, that's fine. The conflict is resolved, by the way.

@mosabab
Copy link
Contributor

mosabab commented Apr 23, 2020

@NiklasMerz

Did you try to build iOS for this pull ?

I try to build iOS using cordova build, but the build failed for this pull.

@PDLMobileApps
Copy link
Contributor Author

@NiklasMerz

Did you try to build iOS for this pull ?

I try to build iOS using cordova build, but the build failed for this pull.

I am using this PR in a prod environment. I build multiple times a day for iOS and got zero issues. You may want to delete your platform folder, plugins folder, package.json and package-lock.json then retry.

@mosabab
Copy link
Contributor

mosabab commented Apr 23, 2020

@PDLMobileApps

My friend, i create a new empty app and add platform and build it

Try many times also by removing the platforms and folders but the build failed.

If anyone can test it from their sides to check the problem.

I am using:

The latest version of
Cordova-cli

Corodva-ios master version

Xcode latest version.

What i remeber about the build error was about something of objective-c .....

@PDLMobileApps
Copy link
Contributor Author

@PDLMobileApps

My friend, i create a new empty app and add platform and build it

Try many times also by removing the platforms and folders but the build failed.

If anyone can test it from their sides to check the problem.

I am using:

The latest version of
Cordova-cli

Corodva-ios master version

Xcode latest version.

What i remeber about the build error was about something of objective-c .....

Just created and tested an iOS build using the bug-hardcodedstatusbar branch where the changes for this PR were made. I confirm that everything works fine.

@mosabab
Copy link
Contributor

mosabab commented Apr 23, 2020

Message from cordova build:


** BUILD FAILED **

The following build commands failed:
CompileC /var/root/Library/Developer/Xcode/DerivedData/testapp-hhqojpudddigfmcqzttvqbyvtkwp/Build/Intermediates.noindex/testapp.build/Debug-iphonesimulator/testapp.build/Objects-normal/x86_64/CDVWKInAppBrowser.o /Users/mosabab/Documents/testapp/platforms/ios/testapp/Plugins/cordova-plugin-inappbrowser/CDVWKInAppBrowser.m normal x86_64 objective-c com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)
xcodebuild: Command failed with exit code 65`

Please note: i am using the latest xcode v11.4.1
Cordova-CLI: 9.0.0 (cordova-lib@9.0.1)
cordova-ios : #master version
cordova-inappbrowser-plugin (bug-hardcodedstatusbar branch)
(https://github.com/PDLMobileApps/cordova-plugin-inappbrowser/tree/bug-hardcodedstatusbar)

@vadimwe
Copy link

vadimwe commented May 6, 2020

Works excellent. Thanx for fix guys.

@HYPE-Thomas
Copy link

Hey guys, any idea when this PR will get merged?

@lukas-mertens
Copy link

+1 this is a blocker for me as well...

@artberri
Copy link

I tested it also and it works ok. Thanks for the fix.

@NiklasMerz
Copy link
Member

@jcesarmobile @dpa99c Do you have time for a quick review? What do you think?

@NiklasMerz NiklasMerz requested a review from erisu June 2, 2020 18:24
@dpa99c
Copy link
Contributor

dpa99c commented Jun 3, 2020

@NiklasMerz sorry started to review this yesterday and got side-tracked.
Will try to review/test today/tommorrow.

Copy link
Contributor

@dpa99c dpa99c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and looks good to me

dpa99c added a commit to dpa99c/cordova-plugin-inappbrowser-popup-bridge that referenced this pull request Jun 4, 2020
@NiklasMerz
Copy link
Member

@dpa99c Thanks for the review

@NiklasMerz NiklasMerz merged commit 2706f34 into apache:master Jun 4, 2020
@HYPE-Thomas
Copy link

Great! Thanks a lot!!

@mosabab
Copy link
Contributor

mosabab commented Jun 4, 2020

Thanks for the review and test, nice job!

@artberri
Copy link

artberri commented Jun 4, 2020

Thank you!

@mosabab
Copy link
Contributor

mosabab commented Jun 5, 2020

@PDLMobileApps @NiklasMerz @dpa99c
Thanks for help with this issue.

I try to test it in my mac
but the build failed

Here the message:

The following build commands failed:
CompileC /var/root/Library/Developer/Xcode/DerivedData/myapp-fwtbwlotgipnwyfynxipdyoqnfgg/Build/Intermediates.noindex/myapp.build/Debug-iphonesimulator/testapp.build/Objects-normal/x86_64/CDVWKInAppBrowser.o /Users/mosab/Documents/myapp/platforms/ios/testapp/Plugins/cordova-plugin-inappbrowser/CDVWKInAppBrowser.m normal x86_64 objective-c com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)
xcodebuild: Command failed with exit code 65


I opened project in xcode to see what the errors,

I see this error:

/Users/mosab/Documents/myapp/platforms/ios/testapp/Plugins/cordova-plugin-inappbrowser/CDVWKInAppBrowser.m:1250:42: No known instance method for selector 'shouldAutorotateToInterfaceOrientation:'

Could you please investigate about this issue ?

Thanks !

@breautek
Copy link
Contributor

breautek commented Jun 5, 2020

@mosabab You should create a new issue with the bug form filled out.

@mosabab
Copy link
Contributor

mosabab commented Jun 5, 2020

@mosabab You should create a new issue with the bug form filled out.

thanks for advice.

@breautek
Copy link
Contributor

breautek commented Jun 5, 2020

@NiklasMerz

Do you know why the following code was re-added?

- (BOOL)shouldAutorotateToInterfaceOrientation:(UIInterfaceOrientation)interfaceOrientation
{
if ((self.orientationDelegate != nil) && [self.orientationDelegate respondsToSelector:@selector(shouldAutorotateToInterfaceOrientation:)]) {
return [self.orientationDelegate shouldAutorotateToInterfaceOrientation:interfaceOrientation];
}
return YES;
}

Appears to be a regression, as pointed out by #714 (comment)

@NiklasMerz
Copy link
Member

@NiklasMerz

Do you know why the following code was re-added?

- (BOOL)shouldAutorotateToInterfaceOrientation:(UIInterfaceOrientation)interfaceOrientation
{
if ((self.orientationDelegate != nil) && [self.orientationDelegate respondsToSelector:@selector(shouldAutorotateToInterfaceOrientation:)]) {
return [self.orientationDelegate shouldAutorotateToInterfaceOrientation:interfaceOrientation];
}
return YES;
}

Appears to be a regression, as pointed out by #714 (comment)

Looks like a merge conflict.I don't really understand it. Preparing a PR to fix it. Code was removed in #666

NiklasMerz added a commit that referenced this pull request Jun 5, 2020
Merging issue from #656

Closes #714
@NiklasMerz NiklasMerz mentioned this pull request Jun 5, 2020
5 tasks
dpa99c added a commit to dpa99c/cordova-plugin-inappbrowser-popup-bridge that referenced this pull request Jun 5, 2020
NiklasMerz added a commit that referenced this pull request Jun 5, 2020
Fix merging issue from #656

Closes #714
@NiklasMerz NiklasMerz mentioned this pull request Jul 9, 2020
5 tasks
@mosabab
Copy link
Contributor

mosabab commented Aug 6, 2020

@NiklasMerz Hello, sorry for the mention, but I have a question please regarding this PR.

After this PR remove the fake statusbar, I face a problem with transparent statusbar after inappbrowser appear.

How we can handle this to make status bar visible when inappbrowser opened?

Does you have a solution for it? because I don't see any information regarding this.

Regards

@PDLMobileApps
Copy link
Contributor Author

@NiklasMerz Hello, sorry for the mention, but I have a question please regarding this PR.

After this PR remove the fake statusbar, I face a problem with transparent statusbar after inappbrowser appear.

How we can handle this to make status bar visible when inappbrowser opened?

Does you have a solution for it? because I don't see any information regarding this.

Regards

You can add the cordova-plugin-status bar and set the status bar color like this:
<preference name="StatusBarBackgroundColor" value="#242021" />

@mosabab
Copy link
Contributor

mosabab commented Aug 11, 2020

@NiklasMerz Hello, sorry for the mention, but I have a question please regarding this PR.
After this PR remove the fake statusbar, I face a problem with transparent statusbar after inappbrowser appear.
How we can handle this to make status bar visible when inappbrowser opened?
Does you have a solution for it? because I don't see any information regarding this.
Regards

You can add the cordova-plugin-status bar and set the status bar color like this:
<preference name="StatusBarBackgroundColor" value="#242021" />

Thanks for information, I try it and work correctly.

But what about if user change the device to dark theme or light theme?

I think the statusbar color work in dark theme and in other ways in light themes.
(if the StatusBarBackgroundColor is dark and the statusbar color in light it work, but if user change to light then the statusbar color not appear)

How we can handle this?

Thanks

@mosabab
Copy link
Contributor

mosabab commented Aug 25, 2020

@PDLMobileApps
Please take a look for this issue:

#728

I face this issue as others.

How we can deal with your pull and dark theme for iOS?

@DevEdward666
Copy link

I Have an issue on IOS specifically on Iphone with notch iphone X to 13.
can someone help me with this problem?
IAB bug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove fake status bar for iOS Issue with status bar on iPhone X when toolbar is at the top (v3.0.0)