Don't Skip Remeasurement On First Layout Pass (#3031)

* Don't Skip Remeasurement if Initial Bounds are Zero

* Remove misleading unit test

Author: Adlai Holler

AsyncDisplayKit/ASCollectionView.mm

@@ -89,7 +89,6 @@ @interface ASCollectionView () <ASRangeControllerDataSource, ASRangeControllerDe
   ASBatchContext *_batchContext;
 
   CGSize _lastBoundsSizeUsedForMeasuringNodes;
-  BOOL _ignoreNextBoundsSizeChangeForMeasuringNodes;
 
   NSMutableSet *_registeredSupplementaryKinds;
 
@@ -272,9 +271,6 @@ - (instancetype)_initWithFrame:(CGRect)frame collectionViewLayout:(UICollectionV
   _superIsPendingDataLoad = YES;
 
   _lastBoundsSizeUsedForMeasuringNodes = self.bounds.size;
-  // If the initial size is 0, expect a size change very soon which is part of the initial configuration
-  // and should not trigger a relayout.
-  _ignoreNextBoundsSizeChangeForMeasuringNodes = CGSizeEqualToSize(_lastBoundsSizeUsedForMeasuringNodes, CGSizeZero);
 
   _layoutFacilitator = layoutFacilitator;
 
@@ -1904,31 +1900,22 @@ - (void)layer:(CALayer *)layer didChangeBoundsWithOldValue:(CGRect)oldBounds new
   }
   _lastBoundsSizeUsedForMeasuringNodes = newBounds.size;
 
-  // First size change occurs during initial configuration. An expensive relayout pass is unnecessary at that time
-  // and should be avoided, assuming that the initial data loading automatically runs shortly afterward.
-  if (_ignoreNextBoundsSizeChangeForMeasuringNodes) {
-    _ignoreNextBoundsSizeChangeForMeasuringNodes = NO;
-  } else {
-    // Laying out all nodes is expensive, and performing an empty update may be unsafe
-    // if the data source has pending changes that it hasn't reported yet – the collection
-    // view will requery the new counts and expect them to match the previous counts.
-    //
-    // We only need to do this if the bounds changed in the non-scrollable direction.
-    // If, for example, a vertical flow layout has its height changed due to a status bar
-    // appearance update, we do not need to relayout all nodes.
-    // For a more permanent fix to the unsafety mentioned above, see https://github.com/facebook/AsyncDisplayKit/pull/2182
-    ASScrollDirection scrollDirection = self.scrollableDirections;
-    BOOL fixedVertically = (ASScrollDirectionContainsVerticalDirection(scrollDirection) == NO);
-    BOOL fixedHorizontally = (ASScrollDirectionContainsHorizontalDirection(scrollDirection) == NO);
-
-    BOOL changedInNonScrollingDirection = (fixedHorizontally && newBounds.size.width != lastUsedSize.width) || (fixedVertically && newBounds.size.height != lastUsedSize.height);
-
-    if (changedInNonScrollingDirection) {
-      [_dataController relayoutAllNodes];
-      [_dataController waitUntilAllUpdatesAreCommitted];
-      // We need to ensure the size requery is done before we update our layout.
-      [self.collectionViewLayout invalidateLayout];
-    }
+  // Laying out all nodes is expensive.
+  // We only need to do this if the bounds changed in the non-scrollable direction.
+  // If, for example, a vertical flow layout has its height changed due to a status bar
+  // appearance update, we do not need to relayout all nodes.
+  // For a more permanent fix to the unsafety mentioned above, see https://github.com/facebook/AsyncDisplayKit/pull/2182
+  ASScrollDirection scrollDirection = self.scrollableDirections;
+  BOOL fixedVertically = (ASScrollDirectionContainsVerticalDirection(scrollDirection) == NO);
+  BOOL fixedHorizontally = (ASScrollDirectionContainsHorizontalDirection(scrollDirection) == NO);
+
+  BOOL changedInNonScrollingDirection = (fixedHorizontally && newBounds.size.width != lastUsedSize.width) || (fixedVertically && newBounds.size.height != lastUsedSize.height);
+
+  if (changedInNonScrollingDirection) {
+    [_dataController relayoutAllNodes];
+    [_dataController waitUntilAllUpdatesAreCommitted];
+    // We need to ensure the size requery is done before we update our layout.
+    [self.collectionViewLayout invalidateLayout];
   }
 }
 

AsyncDisplayKit/ASTableView.mm

@@ -29,7 +29,6 @@
 #import <AsyncDisplayKit/ASTableView+Undeprecated.h>
 #import <AsyncDisplayKit/ASBatchContext.h>
 
-static const ASSizeRange kInvalidSizeRange = {CGSizeZero, CGSizeZero};
 static NSString * const kCellReuseIdentifier = @"_ASTableViewCell";
 
 //#define LOG(...) NSLog(__VA_ARGS__)
@@ -144,7 +143,6 @@ @interface ASTableView () <ASRangeControllerDataSource, ASRangeControllerDelegat
   CALayer *_retainedLayer;
 
   CGFloat _nodesConstrainedWidth;
-  BOOL _ignoreNodesConstrainedWidthChange;
   BOOL _queuedNodeHeightUpdate;
   BOOL _isDeallocating;
   BOOL _performingBatchUpdates;
@@ -268,9 +266,6 @@ - (void)configureWithDataControllerClass:(Class)dataControllerClass eventLog:(AS
   _automaticallyAdjustsContentOffset = NO;
 
   _nodesConstrainedWidth = self.bounds.size.width;
-  // If the initial size is 0, expect a size change very soon which is part of the initial configuration
-  // and should not trigger a relayout.
-  _ignoreNodesConstrainedWidthChange = (_nodesConstrainedWidth == 0);
 
   _proxyDelegate = [[ASTableViewProxy alloc] initWithTarget:nil interceptor:self];
   super.delegate = (id<UITableViewDelegate>)_proxyDelegate;
@@ -690,19 +685,14 @@ - (void)waitUntilAllUpdatesAreCommitted
 
 - (void)layoutSubviews
 {
+  // Remeasure all rows if our row width has changed.
   CGFloat constrainedWidth = self.bounds.size.width - [self sectionIndexWidth];
-  if (_nodesConstrainedWidth != constrainedWidth) {
+  if (constrainedWidth > 0 && _nodesConstrainedWidth != constrainedWidth) {
     _nodesConstrainedWidth = constrainedWidth;
 
-    // First width change occurs during initial configuration. An expensive relayout pass is unnecessary at that time
-    // and should be avoided, assuming that the initial data loading automatically runs shortly afterward.
-    if (_ignoreNodesConstrainedWidthChange) {
-      _ignoreNodesConstrainedWidthChange = NO;
-    } else {
-      [self beginUpdates];
-      [_dataController relayoutAllNodes];
-      [self endUpdatesAnimated:(ASDisplayNodeLayerHasAnimations(self.layer) == NO) completion:nil];
-    }
+    [self beginUpdates];
+    [_dataController relayoutAllNodes];
+    [self endUpdatesAnimated:(ASDisplayNodeLayerHasAnimations(self.layer) == NO) completion:nil];
   }
 
   // To ensure _nodesConstrainedWidth is up-to-date for every usage, this call to super must be done last
@@ -1613,7 +1603,7 @@ - (ASCellNodeBlock)dataController:(ASDataController *)dataController nodeBlockAt
 
 - (ASSizeRange)dataController:(ASDataController *)dataController constrainedSizeForNodeAtIndexPath:(NSIndexPath *)indexPath
 {
-  ASSizeRange constrainedSize = kInvalidSizeRange;
+  ASSizeRange constrainedSize = ASSizeRangeZero;
   if (_asyncDelegateFlags.tableNodeConstrainedSizeForRow) {
     GET_TABLENODE_OR_RETURN(tableNode, constrainedSize);
     ASSizeRange delegateConstrainedSize = [_asyncDelegate tableNode:tableNode constrainedSizeForRowAtIndexPath:indexPath];

AsyncDisplayKitTests/ASTableViewTests.mm

@@ -395,29 +395,6 @@ - (void)testRelayoutAllNodesWithNonZeroSizeInitially
   [self triggerSizeChangeAndAssertRelayoutAllNodesForTableView:tableView newSize:tableViewFinalSize];
 }
 
-- (void)testRelayoutAllNodesWithZeroSizeInitially
-{
-  // Initial width of the table view is 0. The first size change is part of the initial config.
-  // Any subsequence size change after that must trigger a relayout.
-  CGSize tableViewFinalSize = CGSizeMake(100, 500);
-  ASTestTableView *tableView = [[ASTestTableView alloc] __initWithFrame:CGRectZero
-                                                                style:UITableViewStylePlain];
-  ASTableViewFilledDataSource *dataSource = [ASTableViewFilledDataSource new];
-
-  tableView.asyncDelegate = dataSource;
-  tableView.asyncDataSource = dataSource;
-  
-  // Initial configuration
-  UIView *superview = [[UIView alloc] initWithFrame:CGRectMake(0, 0, 500, 500)];
-  [superview addSubview:tableView];
-  // Width and height are swapped so that a later size change will simulate a rotation
-  tableView.frame = CGRectMake(0, 0, tableViewFinalSize.height, tableViewFinalSize.width);
-  [tableView layoutIfNeeded];
-  
-  XCTAssertEqual(tableView.testDataController.numberOfAllNodesRelayouts, 0);
-  [self triggerSizeChangeAndAssertRelayoutAllNodesForTableView:tableView newSize:tableViewFinalSize];
-}
-
 - (void)testRelayoutVisibleRowsWhenEditingModeIsChanged
 {
   CGSize tableViewSize = CGSizeMake(100, 500);