-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[WIP] Add top notch detection #546
Conversation
@dpa99c Sorry for the ping and thanks for your reviews on my PRs, but I would like to add you as a reviewer. Probably someone should create a CODEROWNERS file with you, because you review PRs in this repo anyway :-D |
Will try to get around to it shortly |
@NiklasMerz Sorry for the delay. I've tested your branch used my test harness project. It works fine if the IAB is opened while the device is in portrait orientation: But if you orientate the device into landscape orientation before opening the IAB, the status bar height is incorrect in the IAB when it is opened. Instead of displaying no status bar, there's grey bar: This then persists if you rotate the device back to portrait orientation: |
I did start to make an attempt to detect the orientation change using the - (void)viewWillAppear:(BOOL)animated
{
if (IsAtLeastiOSVersion(@"7.0") && !viewRenderedAtLeastOnce) {
viewRenderedAtLeastOnce = TRUE;
[self setHeight];
[[UIApplication sharedApplication] setStatusBarStyle:[self preferredStatusBarStyle]];
}
[self rePositionViews];
[super viewWillAppear:animated];
}
-(void)viewWillTransitionToSize:(CGSize)size withTransitionCoordinator:(id<UIViewControllerTransitionCoordinator>)coordinator
{
[super viewWillTransitionToSize:size withTransitionCoordinator:coordinator];
[self setHeight];
}
-(void) setHeight
{
CGRect mainStatusBarFrame = [UIApplication sharedApplication].statusBarFrame;
CGFloat mainStatusBarFrameHeight = mainStatusBarFrame.size.height;
CGRect statusBarFrame = [self getStatusBarFrame];
statusBarFrame.size.height = mainStatusBarFrameHeight;
}
- (CGRect) getStatusBarFrame
{
return [self invertFrameIfNeeded:[UIApplication sharedApplication].statusBarFrame];
} I will try to make some time to look again but maybe this is some use to you in the meantime? |
Thanks for your review. I never use portrait mode So forgot to test this.
I will get to this tomorrow and test your code.
Am 15. Okt. 2019, 22:18, um 22:18, Dave Alden <notifications@github.com> schrieb:
…I did start to make an attempt to detect the orientation change using
the [`viewWillTransitionToSize`
delegate](https://developer.apple.com/documentation/uikit/uicontentcontainer/1621466-viewwilltransitiontosize)
but didn't get much further than this initial attempt which didn't
work:
```objectivec
- (void)viewWillAppear:(BOOL)animated
{
if (IsAtLeastiOSVersion(@"7.0") && !viewRenderedAtLeastOnce) {
viewRenderedAtLeastOnce = TRUE;
[self setHeight];
[[UIApplication sharedApplication] setStatusBarStyle:[self
preferredStatusBarStyle]];
}
[self rePositionViews];
[super viewWillAppear:animated];
}
-(void)viewWillTransitionToSize:(CGSize)size
withTransitionCoordinator:(id<UIViewControllerTransitionCoordinator>)coordinator
{
[super viewWillTransitionToSize:size
withTransitionCoordinator:coordinator];
[self setHeight];
}
-(void) setHeight
{
CGRect mainStatusBarFrame = [UIApplication
sharedApplication].statusBarFrame;
CGFloat mainStatusBarFrameHeight = mainStatusBarFrame.size.height;
CGRect statusBarFrame = [self getStatusBarFrame];
statusBarFrame.size.height = mainStatusBarFrameHeight;
}
- (CGRect) getStatusBarFrame
{
return [self invertFrameIfNeeded:[UIApplication
sharedApplication].statusBarFrame];
}
```
I will try to make some time to look again but maybe this is some use
to you in the meantime?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#546 (comment)
|
This is not a proper fix, it just hides the fake status bar for iPhone X and other devices with notches. The proper fix should make the fake statusbar the size of the notch to be consistent, or just remove the fake statusbar for all devices. |
You are right. What about removing the statusbar for all devices? I think that would make sense. Does this mean we need an option to for showing/hiding the statusbar? |
The last two commits show my attempts to fix the statusbar heigth for devices with notch. But there is room for improvement. The statusbar looks kind of ugly on all devices and on devices in landscape mode, where it's empty. On devices with notch it is even bigger. |
I personally vote for removing it, it’s an ugly “hack” that doesn’t work well |
I agree. Considering the effort required to make the statusbar work properly in the IAB for devices with a notch, removing it entirely is much easier and since the IAB is a secondary UI to the main Cordova app UI, not displaying the statusbar while it's open seems like a not-too-big inconvenience. We could at least start by removing it entirely and if someone has time to make a PR which fixes display of the statusbar on all devices (notch and non-notch) in all orientations, with the additional option to display or not display it, then we can consider re-adding it at that point in time. |
Making it configurable is going to be troublesome as the toolbar is created in the CDVInAppBrowserNavigationController, where the InAppBrowser options are not available |
we are talking about the toolbar that is used as a fake status bar, not the real status bar, the status bar won't be hidden, but apple removed what used to be the solid status bar on iOS 7, so trying to keep that appearance of the iOS <= 6 status bar makes no sense anymore |
I understand, looks like you don’t. So you seem to agree with us |
Please, don’t try to rush things, we are all volunteers and we do what we can when we can and for free. Feel free to send the pull request yourself if you need it so much. |
I'm with @jcesarmobile . You need to be patient or do the fix yourself. That's how I do things. That's how it works in Open Source. We cannot expect anyone to do the work just because we feel the need for us and others. I am currently happy with the solution as it is in this PR. I may do the complete removal of the statusbar some time in the future if I have the spare time, but let's see. @mosababubakr Just try doing this change yourself. It is not that hard as you might expect. I never worked on native iOS projects before, but I did some contributions to iOS Cordova plugins just by trying stuff out and googling. You may find a solution and can get feedback on a PR to get it working as you like. |
Hi @NiklasMerz I took the liberty to add "[WIP]" to the title since this PR seems to be not ready for us to review and merge right now. (I would really favor I would also wonder if this might be solved by https://github.com/EddyVerbruggen/cordova-plugin-safariviewcontroller or by the https://github.com/toontoet/cordova-plugin-themeablebrowser fork that was mentioned in #301 (comment)? I would also favor getting rid of the status bar, let other forks do something more elaborate if needed. |
That's fine. I can try to do the removal of the fake statusbar, if I find time anytime soon, to update this PR. Removing the statusbar seems logical. |
You are right, I test your fork modified plugin, and work well in portrait mode, but in landscape mode the empty grey bar appear, I think removing it could be better. |
@NiklasMerz I think it could be better to merge your fix to the 3.2.0 release make good sense. |
No this one is not ready. It works for me and might work for you but it is not a proper fix and won't be in 3.2.0 Like suggested by jcesarmobile a proper fix would be to remove the "fake statusbar" completely in the next major release. I looked at this briefly and I am afraid I don't know enough about Objective-C to do this in the time I have available. I created #604 for that and others might pick this up. |
@jcesarmobile I'm using Ionic 4 and Capacitor (I know this is specific to in-app-browser). However, is there anyway I can use the in-app-browser Cordova plugin with my own custom changes in Capacitor? Can't find any way to do that. |
@justjoeyuk and others please feel free to fork and adapt. |
I did little change in plugin to detect and Resize InAppBrowser based on Notched devices. It adjusts its dimensions to fit safe area with orientation change as well . PS: I am not a swift or Pro apple coder, but took lot of time to get it fixed for my use case. Any contributor is free to modify below code, might be ugly. Have not tested this with that Done button pane for IAB, for my use case of hybird app which uses full screen no URL bar, this worked. Change in CDVInAppBrowserNavigationController.m
Change in CDVWKInAppBrowser.m declare -
add these 2 methods-
Modify ViewWillAppear to
Modify to supportedInterfaceOrientations method to
|
Should be solved with #604 which is the proper solution |
Notch detection PR apache#546 - Pull Request apache#546 for notch detection - https://github.com/apache/cordova-plugin-inappbrowser/pull/546/commits
Platforms affected
iOS
Motivation and Context
This fixes an issue with iPhone that have a notch.
Closes #301
Description
Detect if phone has notch and adjust statusbar.
Testing
Using with iPhones with notch and without. Deployed to production app for some time.
Checklist
(platform)
if this change only applies to one platform (e.g.(android)
)