Content insets and edge padding should be additive, not mutually exclusive

[1ec5]

By design, a camera’s edge padding and the map’s content insets are supposed to be additive: every camera change operation is relative to the content insets, and the edge padding can further inset the camera.

In #12818, the -[MGLMapView setCamera:withDuration:animationTimingFunction:edgePadding:completionHandler:] method incorrectly treated the edge padding and content insets as mutually exclusive. If the developer left the edge padding unspecified when changing the camera, then MGLMapView would correctly consider just the content insets. But if the developer passes in an edge padding, MGLMapView would incorrectly ignore the content insets in favor of the edge padding alone. Clients (including the navigation SDK) worked around the bug by explicitly adding the content insets to the edge padding.

#14813 fixed this issue for v5.1.0, but in the same release, #14664 regressed it in mbgl and expanded the issue to flight animations as well:

mapbox-gl-native/src/mbgl/map/transform.cpp

Line 85 in b6dd995

const EdgeInsets& padding = camera.padding.value_or(state.edgeInsets);

mapbox-gl-native/src/mbgl/map/transform.cpp

Line 165 in b6dd995

const EdgeInsets& padding = camera.padding.value_or(state.edgeInsets);

/cc @astojilj @mapbox/maps-ios @mapbox/navigation-ios @d-prukop

[1ec5]

Additionally, if a padding is explicitly passed into either easeTo() and flyTo(), mbgl persists that padding as the content insets: #15233. Because mbgl conflates the two properties, it isn’t possible even to work around the original issue by manually passing in the content inset plus a padding – now the developer has to reset the content inset in the camera change operation’s completion handler.

[astojilj]

@1ec5 ,

We discussed this concern at #12818 (comment) and I should continue work on this.

By design, a camera’s edge padding and the map’s content insets are supposed to be additive: every camera change operation is relative to the content insets, and the edge padding can further inset the camera.

As mentioned there, didn't see it defined in documentation and thought it is intuitive to make it behave additive for case of automaticallyAdjustsScrollViewInsets, otherwise no.

This is related to discussion in #15233 and we should clarify the behavior in documentation. I don't see it requires API changes.

[1ec5]

I don’t think API changes are strictly required, but I do think we need to do more than clarify the documentation. If content insets and edge padding are redundant to each other and one affects the other, then there was no point in fixing #12818, and the situation is especially confusing for someone used to UIKit and MapKit controls.

The discussion in #12818 (comment) was focused on whether we could get away with making the change without bumping the major version, as required by semantic versioning. A loophole in the documentation meant that we could do so, but it was still necessary to make the various methods consistent.

automaticallyAdjustsScrollViewInsets is only a convenience for the developer, but the developer can set the insets for exactly the same reason that it would be set automatically. In that case, they should get the same camera behavior as if the map view adjusted the insets automatically.

[astojilj]

Offline agreement on this is that it is confusing to have both content insets and padding since they both use the same implementation to implement Android and iOS code and terms content insets, padding, edge insets are used interchangeably.

We will reconsider anchor design later.

@1ec5

By design, a camera’s edge padding and the map’s content insets are supposed to be additive: every camera change operation is relative to the content insets, and the edge padding can further inset the camera.

This is left to be covered. I don't see that our documentation covers this. Is the design implied by adopting similar functionality from other APIs/frameworks?

[astojilj]

@1ec5 , @fabian-guerra

I'm going again through the discussion to split the issues list from the description and title (part of it is related to implementation detail of pre and after #14664.

The regression tag added here - is it related to #14813 fixing #12818?

We can close the issue as, from perspective of iOS SDK API, they are additive - not mutually exclusive - all the APIs add content inset to specified padding:

mapbox-gl-native/platform/ios/src/MGLMapView.mm

Line 3529 in cd12961

edgePadding = MGLEdgeInsetsInsetEdgeInset(edgePadding, self.contentInset);

mapbox-gl-native/platform/ios/src/MGLMapView.mm

Line 3359 in cd12961

padding += MGLEdgeInsetsFromNSEdgeInsets(self.contentInset);

Content inset is stored as separate property of MGLMapView and added to specified padding later - so MGLMapView doesn't ignore it.

[astojilj]

Closing according to #15232 (comment)