Mapbox-gl-native: Compass underlaps top bar

[1ec5]

In a standard view controller with a top bar and automaticallyAdjustsScrollViewInsets set to YES, the compass underlaps the top bar:

This is almost certainly a regression from iOS SDK v3.3.x. Relevant PRs from this release cycle: #6216 #6313 #6566.

/cc @boundsj @incanus

[1ec5]

This is causing KIF tests to fail:

Test: testBottomLayoutGuide in MapViewTests failed on iPhone 7
Assertions: ((CGRectIntersectsRect(logoBugFrame, toolbarFrame)) is false) failed - logo bug should not be under toolbar
File: MapViewTests.m:386

Test: testTopLayoutGuide in MapViewTests failed on iPhone 7
Assertions: ((CGRectIntersectsRect(compassFrame, navigationBarFrame)) is false) failed - compass should not be under navigation bar
File: MapViewTests.m:359

[frederoni]

I think this occurs because we don't handle device rotation properly or there's a bug in the topLayoutGuide.

The initial height of the topLayoutGuide is 64, rotate to landscape and it's 44, rotate back to portrait and we have 52. Additional rotations from here will switch between 44 and 52.

Here's two workarounds:

1. Respond to "device rotation" in the view controller

// MBXViewController.m
- (void)viewDidLayoutSubviews
{
    [super viewDidLayoutSubviews];
    [self.mapView setNeedsLayout];
}

2. Generate device orientation notifications in MGLMapView

// MGLMapView.m
[[UIDevice currentDevice] beginGeneratingDeviceOrientationNotifications];
[[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(deviceOrientationDidChange:) name:UIDeviceOrientationDidChangeNotification object:nil];

- (void)deviceOrientationDidChange:(NSNotification)notification
{
    [self setNeedsLayout];
}

I'd prefer none of the above so I'll dig a bit more. 🐰 🕳

[frederoni]

Decided to go with the latter because it works well and it's a fix in the SDK unlike the first workaround. Implemented in #7004

[1ec5]

I think this may be a duplicate of #6755.

In #6776, @friedbunny noted that the compass positioning is likely due to a UIKit bug in iOS 10 rather than a regression on our end. However, his proposed fix had some problems too. @frederoni, can you reconcile the two PRs?

[frederoni]

Fixed in #6776

[1ec5]

6776 doesn’t fix #1327 and actually causes another KIF test to fail:

Test: testBottomLayoutGuide in MapViewTests failed on iPhone 7
Assertions: ((CGRectIntersectsRect(logoBugFrame, toolbarFrame)) is false) failed - logo bug should not be under toolbar
File: MapViewTests.m:386

Test: testInsetMapView in MapViewTests failed on iPhone 7
Assertions: ((CGRectIntersectsRect(compassFrame, mapViewFrame)) is true) failed - compass should lie inside shrunken map view
File: MapViewTests.m:423

Test: testTopLayoutGuide in MapViewTests failed on iPhone 7
Assertions: ((CGRectIntersectsRect(compassFrame, navigationBarFrame)) is false) failed - compass should not be under navigation bar
File: MapViewTests.m:359

I disagree with @frederoni’s claim in https://github.com/mapbox/mapbox-gl-native/pull/6776#issuecomment-260626664 that the developer needs to explicitly set automaticallyAdjustsScrollViewInsets to NO if they intend to make MGLMapView not extend to the edges of the screen. This isn’t how MKMapView works.

[frederoni]

Fixed in #7084

[1ec5]

As of 13db36c12387d7236a47fdd7da2693635fc3beca in the iPhone 7 simulator:

Test Case '-[MapViewTests testBottomLayoutGuide]' started.
/Users/mxn/hub/mapbox-gl-native-2/platform/ios/uitest/MapViewTests.m:386: error: -[MapViewTests testBottomLayoutGuide] : ((CGRectIntersectsRect(logoBugFrame, toolbarFrame)) is false) failed - logo bug should not be under toolbar
Test Case '-[MapViewTests testBottomLayoutGuide]' failed (0.031 seconds).
Test Case '-[MapViewTests testTopLayoutGuide]' started.
/Users/mxn/hub/mapbox-gl-native-2/platform/ios/uitest/MapViewTests.m:359: error: -[MapViewTests testTopLayoutGuide] : ((CGRectIntersectsRect(compassFrame, navigationBarFrame)) is false) failed - compass should not be under navigation bar
Test Case '-[MapViewTests testTopLayoutGuide]' failed (0.028 seconds).

The view hierarchy at the point of failure:

[1ec5]

If the navigation bar is present by the time MGLMapView lays itself out, the compass shifts downward to accommodate the navigation bar. However, if you make the navigation bar visible after the map view lays itself out, it fails to update the compass’ constraints to reflect the change.

[incanus]

That's weird; shouldn't the topLayoutGuide itself be updated automatically, carrying the constraint along with it?

[1ec5]

7084 removed the code that conditionally constrains the ornaments to the layout guides. Instead, the ornaments are always constrained only to MGLMapView itself. This is exactly the bug this test was designed to catch.

[frederoni]

This regressed in https://github.com/mapbox/mapbox-gl-native/commit/ede1e713d8b66be713d96141653b83078443cbea but we can't just revert it because:
1: The components should respect the contentInset.
2: viewController.view is the developers responsibility and we should not modify its constraints.

[incanus]

2: viewController.view is the developers responsibility and we should not modify its constraints.

Coming into this a little cold, but we've long (years) had to touch the view controller view constraints for the compass to solve issues like this. Because we don't want to force users to use a view controller that we mandate, we've searched the responder chain to find the map view's first view controller ancestor in order to solve compass underlap problems.

[1ec5]

I think @frederoni’s point is that, even though we may need to access the view controller’s constraints, we shouldn’t be manipulating them; instead, we should manually move the ornaments around based on the layout guides’ current values. This makes me wonder whether we should ultimately be constraining the ornaments to the map view’s margins instead of its actual edges.

In any case, since the remaining issue regressed in v3.3.1, it sounds like we should postpone #7716 until v3.4.1 to avoid any last-minute bugs around KVO.

[1ec5]

Postponing to v3.5.0 because we’ve apparently shipped imperfect behavior around this scenario even before v3.4.0 and the fix would be risky enough to require a beta release.

[1ec5]

Fixed in #7716 on the release-ios-v3.6.0-android-v5.1.0 branch.