LeoNatan / LNPopupController

A framework for presenting view controllers as popups of other view controllers, much like the Apple Music and Podcasts apps.
MIT License
3.04k stars 340 forks source link

Layout margins and safe area insets are incorrect when presenting the map scene in landscape #291

Closed iDevelopper closed 5 years ago

iDevelopper commented 6 years ago

Description

When presenting a custom popup bar with a tab bar controller or a navigation controller, the popup bar subviews are not lay correctly.

Steps to Reproduce

From your example, in MapScene.storyboard, embed the MapViewController in a UITabBarController.

Be sure the new tab bar controller is the initial view controller.

In MapViewController.swift, present the custom popup bar with the tab bar controller:

    override func viewWillAppear(_ animated: Bool) {
        super.viewWillAppear(animated)

        let customMapBar = storyboard!.instantiateViewController(withIdentifier: "CustomMapBarViewController") as! CustomMapBarViewController
        customMapBar.view.backgroundColor = .clear
        customMapBar.searchBar.delegate = self

        self.tabBarController?.popupBar.customBarViewController = customMapBar
        popupContentView.popupCloseButtonStyle = .none
        popupInteractionStyle = .snap

        popupContentVC = storyboard!.instantiateViewController(withIdentifier: "PopupContentController") as? LocationsController
        popupContentVC.tableView.backgroundView = UIVisualEffectView(effect: UIBlurEffect(style: .extraLight))

        DispatchQueue.main.async {
            self.tabBarController?.presentPopupBar(withContentViewController: self.popupContentVC, animated: false, completion: nil)
        }
    }

With Xcode 9.4.1 when presenting the custom popup bar, the width of the search bar is equal to 395, (that corresponds to iPhone X width plus the leading and the trailing spaces???).

With Xcode 10.0 beta 6, the width of the search bar is equal to 379 (???)

And when you rotate the screen, the search bar does not take into account the unsafe area and is displayed over it:

capture d ecran 2018-09-07 a 17 50 11

Device, OS and Xcode Versions

iPhone X, Xcode 9.4.1, iOS 11.4 or Xcode 10.0 beta 6, iOS 12

LeoNatan commented 6 years ago

Well, I was wondering when iOS 12 will break my stuff 😂

Thanks! I am on vacation now, but will look into this when I am back.

iDevelopper commented 6 years ago

No as I mentioned above, the issue is the same on iOS 11 as well.

LeoNatan commented 6 years ago

I seem to be misunderstanding something. I will run the example project with your changes so I can see it when I am back.

iDevelopper commented 6 years ago

Hi @LeoNatan , Good holidays!

I'm still not very comfortable with PR!

But after some investigations, setting preservesSuperviewLayoutMargins to YES to the customBarViewController's view solves the problem (In LNPopupBar.m):

- (void)setCustomBarViewController:(LNPopupCustomBarViewController*)customBarViewController
{
    if(_customBarViewController != customBarViewController)
    {
        if (customBarViewController.containingPopupBar)
        {
            //Cleanly move the custom bar view controller from the previos popup bar.
            customBarViewController.containingPopupBar.customBarViewController = nil;
        }

        _customBarViewController.containingPopupBar = nil;
        [_customBarViewController.view removeFromSuperview];
        [_customBarViewController removeObserver:self forKeyPath:@"preferredContentSize"];

        _customBarViewController = customBarViewController;
        _customBarViewController.containingPopupBar = self;
        [_customBarViewController addObserver:self forKeyPath:@"preferredContentSize" options:NSKeyValueObservingOptionNew context:NULL];

                _customBarViewController.view.preservesSuperviewLayoutMargins = YES; // <----- here
        _customBarViewController.view.translatesAutoresizingMaskIntoConstraints = NO;
        [self addSubview:_customBarViewController.view];
        [NSLayoutConstraint activateConstraints:@[[self.topAnchor constraintEqualToAnchor:_customBarViewController.view.topAnchor],
                                                  [self.leftAnchor constraintEqualToAnchor:_customBarViewController.view.leftAnchor],
                                                  [self.rightAnchor constraintEqualToAnchor:_customBarViewController.view.rightAnchor],
                                                  [self.bottomAnchor constraintEqualToAnchor:_customBarViewController.view.bottomAnchor]]];

        [self _updateViewsAfterCustomBarViewControllerUpdate];

        [self setBarStyle:LNPopupBarStyleCustom];
    }
}
LeoNatan commented 6 years ago

If this is the solution, this is something that users should set for themselves, not the framework override for them.

LeoNatan commented 6 years ago

Also, layout margins are something completely different from safe area. In the maps example, the text field should be constrained against the safe area, not layout margins. Or perhaps both, not sure now.

iDevelopper commented 6 years ago

If you don't want to set this property here, you could set it in LNPopupCustomViewController, the super class, at least with the default value, this could help users to think about it.

I can easily understand that you don't have to modify the properties of an user view controller!

Also, constraining the field against the safe area does not solve the problem when you rotate the screen to landscape even with this property set to YES. And even with insetsLayoutMarginsFromSafeArea set to YES, but this is the default.

LeoNatan commented 6 years ago

I think this is what should be looked at, but I agree about the default. Will look I to it once I’m back. Thanks!

LeoNatan commented 6 years ago

I see an issue here. Will investigate when I have some time.