Skip to content

Commit

Permalink
[NavigationDrawer] Update cacheLayoutCalculationsWithAddedContentHeig…
Browse files Browse the repository at this point in the history
…ht recursion check to factor out floating point rounding issues (#7654)

The `_contentHeaderTopInset > _contentHeightSurplus` check in `MDCBottomDrawerContainerViewController`'s `cacheLayoutCalculationsWithAddedContentHeight:` function was erroneously evaluating to `true` when presenting content:

- in full screen
- with no header
- with the content's height being equal to the available height

Because of how `_contentHeightSurplus` is calculated, it was almost (but not quite) equal to `_contentHeaderTopInset`, which was causing the function to infinitely recurse.

This PR also adds an example that demonstrates the issue. To reproduce:

1. Open MDCDragons
1. Navigate to the "Bottom Drawer No Header Less Content" example
1. Rotate the phone/simulator to landscape
1. Show the bottom navigation drawer by tapping on the menu icon

Expected:

The bottom drawer is displayed.

Actual:

The `cacheLayoutCalculationsWithAddedContentHeight:` infinitely recurses and the app crashes due to stack overflow.

Closes #7633
  • Loading branch information
bryanoltman committed Jun 19, 2019
1 parent b9ec1b9 commit b243d5e
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 3 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
// Copyright 2018-present the Material Components for iOS authors. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import UIKit
import MaterialComponents.MaterialBottomAppBar
import MaterialComponents.MaterialBottomAppBar_ColorThemer
import MaterialComponents.MaterialColorScheme
import MaterialComponents.MaterialNavigationDrawer

class BottomDrawerNoHeaderLessContentExample: UIViewController {
@objc var colorScheme = MDCSemanticColorScheme()
let bottomAppBar = MDCBottomAppBarView()

let contentViewController = DrawerContentViewController()
let bottomDrawerTransitionController = MDCBottomDrawerTransitionController()

override func viewDidLoad() {
super.viewDidLoad()

contentViewController.preferredHeight = UIScreen.main.bounds.size.height

view.backgroundColor = colorScheme.backgroundColor
contentViewController.view.backgroundColor = colorScheme.primaryColor

bottomAppBar.isFloatingButtonHidden = true
let barButtonLeadingItem = UIBarButtonItem()
let menuImage = UIImage(named:"Menu")?.withRenderingMode(.alwaysTemplate)
barButtonLeadingItem.image = menuImage
barButtonLeadingItem.target = self
barButtonLeadingItem.action = #selector(presentNavigationDrawer)
bottomAppBar.leadingBarButtonItems = [ barButtonLeadingItem ]
MDCBottomAppBarColorThemer.applySurfaceVariant(withSemanticColorScheme: colorScheme,
to: bottomAppBar)
view.addSubview(bottomAppBar)
}

override func viewDidLayoutSubviews() {
super.viewDidLayoutSubviews()

layoutBottomAppBar()
}

private func layoutBottomAppBar() {
let size = bottomAppBar.sizeThatFits(view.bounds.size)
var bottomBarViewFrame = CGRect(x: 0,
y: view.bounds.size.height - size.height,
width: size.width,
height: size.height)
if #available(iOS 11.0, *) {
bottomBarViewFrame.size.height += view.safeAreaInsets.bottom
bottomBarViewFrame.origin.y -= view.safeAreaInsets.bottom
}
bottomAppBar.frame = bottomBarViewFrame
}

@objc func presentNavigationDrawer() {
// This shows that it is possible to present the content view controller directly without
// the need of the MDCBottomDrawerViewController wrapper. To present the view controller
// inside the drawer, both the transition controller and the custom presentation controller
// of the drawer need to be set.
contentViewController.transitioningDelegate = bottomDrawerTransitionController
contentViewController.modalPresentationStyle = .custom
present(contentViewController, animated: true, completion: nil)
}
}

extension BottomDrawerNoHeaderLessContentExample {

@objc class func catalogMetadata() -> [String: Any] {
return [
"breadcrumbs": ["Navigation Drawer", "Bottom Drawer No Header Less Content"],
"primaryDemo": false,
"presentable": false,
]
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -786,7 +786,7 @@ - (void)cacheLayoutCalculationsWithAddedContentHeight:(CGFloat)addedContentHeigh
}
}

CGFloat scrollingDistance = _contentHeaderTopInset + contentHeaderHeight + contentHeight;
CGFloat scrollingDistance = _contentHeaderTopInset + totalHeight;
_contentHeightSurplus = scrollingDistance - containerHeight;
if ([self shouldPresentFullScreen]) {
self.drawerState = MDCBottomDrawerStateFullScreen;
Expand All @@ -795,7 +795,7 @@ - (void)cacheLayoutCalculationsWithAddedContentHeight:(CGFloat)addedContentHeigh
} else {
self.drawerState = MDCBottomDrawerStateCollapsed;
}
if (addedContentHeight < kEpsilon && (_contentHeaderTopInset > _contentHeightSurplus) &&
if (addedContentHeight < kEpsilon && containerHeight > totalHeight &&
(_contentHeaderTopInset - _contentHeightSurplus < self.addedContentHeightThreshold)) {
CGFloat addedContentheight = _contentHeaderTopInset - _contentHeightSurplus;
[self cacheLayoutCalculationsWithAddedContentHeight:addedContentheight];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ - (void)testContentHeightSurplus {
XCTAssertEqualWithAccuracy(self.fakeBottomDrawer.contentHeightSurplus, 0, 0.001);
}

- (void)testContentHeightSurplusWithScrollabelContent {
- (void)testContentHeightSurplusWithScrollableContent {
// Given
CGSize fakePreferredContentSize = CGSizeMake(200, 1000);
[self setupHeaderWithPreferredContentSize:fakePreferredContentSize];
Expand Down Expand Up @@ -751,4 +751,21 @@ - (void)testSettingShouldIncludeSafeAreaInContentHeight {
.shouldIncludeSafeAreaInContentHeight);
}

- (void)testFullScreenContentLayoutCalculationsComplete {
// Given
UIViewController *fakeViewController = [[UIViewController alloc] init];
fakeViewController.view.frame = CGRectMake(0, 0, 200, UIScreen.mainScreen.bounds.size.height);
self.drawerViewController.contentViewController = fakeViewController;
[self.drawerViewController expandToFullscreenWithDuration:0 completion:nil];

// When
MDCBottomDrawerPresentationController *presentationController =
(MDCBottomDrawerPresentationController *)self.drawerViewController.presentationController;
[presentationController.bottomDrawerContainerViewController cacheLayoutCalculations];

// Then
// This test was put in place to validate that cacheLayoutCalculations doesn't infinitely recurse
// It has no visible side effects, so as long as this test finishes, it passes
}

@end

0 comments on commit b243d5e

Please sign in to comment.