[ASScrollNode] Fix small bugs and add unit tests (#637)

* Add unit tests for ASScrollNode

* Make sure ASScrollNode's size is clamped against its size range

* Invalidate ASScrollNode's calculated layout if its scrollable directions changed

* Update comment

* Update CHANGELOG

* Address Adlai's comments

AsyncDisplayKit.xcodeproj/project.pbxproj

@@ -443,6 +443,7 @@
 		E5775B041F16759F00CAC9BC /* ASCollectionLayoutCache.mm in Sources */ = {isa = PBXBuildFile; fileRef = E5775B031F16759F00CAC9BC /* ASCollectionLayoutCache.mm */; };
 		E5855DEF1EBB4D83003639AE /* ASCollectionLayoutDefines.m in Sources */ = {isa = PBXBuildFile; fileRef = E5855DED1EBB4D83003639AE /* ASCollectionLayoutDefines.m */; };
 		E5855DF01EBB4D83003639AE /* ASCollectionLayoutDefines.h in Headers */ = {isa = PBXBuildFile; fileRef = E5855DEE1EBB4D83003639AE /* ASCollectionLayoutDefines.h */; settings = {ATTRIBUTES = (Private, ); }; };
+		E586F96C1F9F9E2900ECE00E /* ASScrollNodeTests.m in Sources */ = {isa = PBXBuildFile; fileRef = E586F96B1F9F9E2900ECE00E /* ASScrollNodeTests.m */; };
 		E58E9E421E941D74004CFC59 /* ASCollectionFlowLayoutDelegate.h in Headers */ = {isa = PBXBuildFile; fileRef = E58E9E3D1E941D74004CFC59 /* ASCollectionFlowLayoutDelegate.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		E58E9E431E941D74004CFC59 /* ASCollectionFlowLayoutDelegate.m in Sources */ = {isa = PBXBuildFile; fileRef = E58E9E3E1E941D74004CFC59 /* ASCollectionFlowLayoutDelegate.m */; };
 		E58E9E441E941D74004CFC59 /* ASCollectionLayoutContext.h in Headers */ = {isa = PBXBuildFile; fileRef = E58E9E3F1E941D74004CFC59 /* ASCollectionLayoutContext.h */; settings = {ATTRIBUTES = (Public, ); }; };
@@ -936,6 +937,7 @@
 		E5775B031F16759F00CAC9BC /* ASCollectionLayoutCache.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ASCollectionLayoutCache.mm; sourceTree = "<group>"; };
 		E5855DED1EBB4D83003639AE /* ASCollectionLayoutDefines.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASCollectionLayoutDefines.m; sourceTree = "<group>"; };
 		E5855DEE1EBB4D83003639AE /* ASCollectionLayoutDefines.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASCollectionLayoutDefines.h; sourceTree = "<group>"; };
+		E586F96B1F9F9E2900ECE00E /* ASScrollNodeTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = ASScrollNodeTests.m; sourceTree = "<group>"; };
 		E58E9E3D1E941D74004CFC59 /* ASCollectionFlowLayoutDelegate.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASCollectionFlowLayoutDelegate.h; sourceTree = "<group>"; };
 		E58E9E3E1E941D74004CFC59 /* ASCollectionFlowLayoutDelegate.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = ASCollectionFlowLayoutDelegate.m; sourceTree = "<group>"; };
 		E58E9E3F1E941D74004CFC59 /* ASCollectionLayoutContext.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASCollectionLayoutContext.h; sourceTree = "<group>"; };
@@ -1219,6 +1221,7 @@
 				E51B78BD1F01A0EE00E32604 /* ASLayoutFlatteningTests.m */,
 				052EE0651A159FEF002C6279 /* ASMultiplexImageNodeTests.m */,
 				058D0A32195D057000B7D73C /* ASMutableAttributedStringBuilderTests.m */,
+				E586F96B1F9F9E2900ECE00E /* ASScrollNodeTests.m */,
 				3C9C128419E616EF00E942A0 /* ASTableViewTests.mm */,
 				CC4981B21D1A02BE004E13CC /* ASTableViewThrashTests.m */,
 				058D0A33195D057000B7D73C /* ASTextKitCoreTextAdditionsTests.m */,
@@ -2177,6 +2180,7 @@
 				CC7FD9E11BB5F750005CCB2B /* ASPhotosFrameworkImageRequestTests.m in Sources */,
 				052EE0661A159FEF002C6279 /* ASMultiplexImageNodeTests.m in Sources */,
 				058D0A3C195D057000B7D73C /* ASMutableAttributedStringBuilderTests.m in Sources */,
+				E586F96C1F9F9E2900ECE00E /* ASScrollNodeTests.m in Sources */,
 				CC8B05D81D73979700F54286 /* ASTextNodePerformanceTests.m in Sources */,
 				CC583AD91EF9BDC600134156 /* ASDisplayNode+OCMock.m in Sources */,
 				697B315A1CFE4B410049936F /* ASEditableTextNodeTests.m in Sources */,

CHANGELOG.md

@@ -12,6 +12,8 @@
 - [Collection/Table] Added direct support for mapping section indexes between data spaces. [Adlai Holler](https://github.com/Adlai-Holler) [#651](https://github.com/TextureGroup/Texture/pull/660)
 - [ASCornerLayoutSpec] New layout spec class for declarative corner element layout. [#657](https://github.com/TextureGroup/Texture/pull/657) [huangkun](https://github.com/huang-kun)
 - [Layout] Fix an issue that causes a pending layout to be applied multiple times.  [Huy Nguyen](https://github.com/nguyenhuy) [#695](https://github.com/TextureGroup/Texture/pull/695)
+- [ASScrollNode] Ensure the node respects the given size range while calculating its layout. [#637](https://github.com/TextureGroup/Texture/pull/637) [Huy Nguyen](https://github.com/nguyenhuy)
+- [ASScrollNode] Invalidate the node's calculated layout if its scrollable directions changed. Also add unit tests for the class. [#637](https://github.com/TextureGroup/Texture/pull/637) [Huy Nguyen](https://github.com/nguyenhuy)
 
 ## 2.6
 - [Xcode 9] Updated to require Xcode 9 (to fix warnings) [Garrett Moon](https://github.com/garrettmoon)

Source/ASScrollNode.mm

@@ -99,10 +99,12 @@
     // To understand this code, imagine we're containing a horizontal stack set within a vertical table node.
     // Our parentSize is fixed ~375pt width, but 0 - INF height.  Our stack measures 1000pt width, 50pt height.
     // In this case, we want our scrollNode.bounds to be 375pt wide, and 50pt high.  ContentSize 1000pt, 50pt.
-    // We can achieve this behavior by: 1. Always set contentSize to layout.size.  2. Set bounds to parentSize,
+    // We can achieve this behavior by:
+    // 1. Always set contentSize to layout.size.
+    // 2. Set bounds to a size that is calculated by clamping parentSize against constrained size,
     // unless one dimension is not defined, in which case adopt the contentSize for that dimension.
     _contentCalculatedSizeFromLayout = layout.size;
-    CGSize selfSize = parentSize;
+    CGSize selfSize = ASSizeRangeClamp(constrainedSize, parentSize);
     if (ASPointsValidForLayout(selfSize.width) == NO) {
       selfSize.width = _contentCalculatedSizeFromLayout.width;
     }
@@ -161,7 +163,10 @@
 - (void)setScrollableDirections:(ASScrollDirection)scrollableDirections
 {
   ASDN::MutexLocker l(__instanceLock__);
-  _scrollableDirections = scrollableDirections;
+  if (_scrollableDirections != scrollableDirections) {
+    _scrollableDirections = scrollableDirections;
+    [self setNeedsLayout];
+  }
 }
 
 @end

Tests/ASScrollNodeTests.m

@@ -0,0 +1,139 @@
+//
+//  ASScrollNodeTests.m
+//  Texture
+//
+//  Copyright (c) 2017-present, Pinterest, Inc.  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
+//
+
+#import <UIKit/UIKit.h>
+#import <XCTest/XCTest.h>
+#import <AsyncDisplayKit/AsyncDisplayKit.h>
+#import "ASXCTExtensions.h"
+
+@interface ASScrollNodeTests : XCTestCase
+
+@property (nonatomic) ASScrollNode *scrollNode;
+@property (nonatomic) ASDisplayNode *subnode;
+
+@end
+
+@implementation ASScrollNodeTests
+
+- (void)setUp
+{
+  ASDisplayNode *subnode = [[ASDisplayNode alloc] init];
+  self.subnode = subnode;
+
+  self.scrollNode = [[ASScrollNode alloc] init];
+  self.scrollNode.scrollableDirections = ASScrollDirectionVerticalDirections;
+  self.scrollNode.automaticallyManagesContentSize = YES;
+  self.scrollNode.automaticallyManagesSubnodes = YES;
+  self.scrollNode.layoutSpecBlock = ^ASLayoutSpec * _Nonnull(__kindof ASDisplayNode * _Nonnull node, ASSizeRange constrainedSize) {
+    return [[ASWrapperLayoutSpec alloc] initWithLayoutElement:subnode];
+  };
+  [self.scrollNode view];
+}
+
+- (void)testSubnodeLayoutCalculatedWithUnconstrainedMaxSizeInScrollableDirection
+{
+  CGSize parentSize = CGSizeMake(100, 100);
+  ASSizeRange sizeRange = ASSizeRangeMake(parentSize);
+
+  [self.scrollNode layoutThatFits:sizeRange parentSize:parentSize];
+
+  ASSizeRange subnodeSizeRange = sizeRange;
+  subnodeSizeRange.max.height = CGFLOAT_MAX;
+  XCTAssertEqual(self.scrollNode.scrollableDirections, ASScrollDirectionVerticalDirections);
+  ASXCTAssertEqualSizeRanges(self.subnode.constrainedSizeForCalculatedLayout, subnodeSizeRange);
+
+  // Same test for horizontal scrollable directions
+  self.scrollNode.scrollableDirections = ASScrollDirectionHorizontalDirections;
+  [self.scrollNode layoutThatFits:sizeRange parentSize:parentSize];
+
+  subnodeSizeRange = sizeRange;
+  subnodeSizeRange.max.width = CGFLOAT_MAX;
+
+  ASXCTAssertEqualSizeRanges(self.subnode.constrainedSizeForCalculatedLayout, subnodeSizeRange);
+}
+
+- (void)testAutomaticallyManagesContentSizeUnderflow
+{
+  CGSize subnodeSize = CGSizeMake(100, 100);
+  CGSize parentSize = CGSizeMake(100, 200);
+  ASSizeRange sizeRange = ASSizeRangeUnconstrained;
+
+  self.subnode.style.preferredSize = subnodeSize;
+
+  [self.scrollNode layoutThatFits:sizeRange parentSize:parentSize];
+  [self.scrollNode layout];
+
+  ASXCTAssertEqualSizes(self.scrollNode.calculatedSize, parentSize);
+  ASXCTAssertEqualSizes(self.scrollNode.view.contentSize, subnodeSize);
+}
+
+- (void)testAutomaticallyManagesContentSizeOverflow
+{
+  CGSize subnodeSize = CGSizeMake(100, 500);
+  CGSize parentSize = CGSizeMake(100, 200);
+  ASSizeRange sizeRange = ASSizeRangeUnconstrained;
+
+  self.subnode.style.preferredSize = subnodeSize;
+
+  [self.scrollNode layoutThatFits:sizeRange parentSize:parentSize];
+  [self.scrollNode layout];
+
+  ASXCTAssertEqualSizes(self.scrollNode.calculatedSize, parentSize);
+  ASXCTAssertEqualSizes(self.scrollNode.view.contentSize, subnodeSize);
+}
+
+- (void)testAutomaticallyManagesContentSizeWithSizeRangeSmallerThanParentSize
+{
+  CGSize subnodeSize = CGSizeMake(100, 100);
+  CGSize parentSize = CGSizeMake(100, 500);
+  ASSizeRange sizeRange = ASSizeRangeMake(CGSizeMake(100, 100), CGSizeMake(100, 200));
+
+  self.subnode.style.preferredSize = subnodeSize;
+
+  [self.scrollNode layoutThatFits:sizeRange parentSize:parentSize];
+  [self.scrollNode layout];
+
+  ASXCTAssertEqualSizes(self.scrollNode.calculatedSize, sizeRange.max);
+  ASXCTAssertEqualSizes(self.scrollNode.view.contentSize, subnodeSize);
+}
+
+- (void)testAutomaticallyManagesContentSizeWithSizeRangeBiggerThanParentSize
+{
+  CGSize subnodeSize = CGSizeMake(100, 200);
+  CGSize parentSize = CGSizeMake(100, 100);
+  ASSizeRange sizeRange = ASSizeRangeMake(CGSizeMake(100, 150));
+
+  self.subnode.style.preferredSize = subnodeSize;
+
+  [self.scrollNode layoutThatFits:sizeRange parentSize:parentSize];
+  [self.scrollNode layout];
+
+  ASXCTAssertEqualSizes(self.scrollNode.calculatedSize, sizeRange.min);
+  ASXCTAssertEqualSizes(self.scrollNode.view.contentSize, subnodeSize);
+}
+
+- (void)testAutomaticallyManagesContentSizeWithInvalidCalculatedSizeForLayout
+{
+  CGSize subnodeSize = CGSizeMake(100, 200);
+  CGSize parentSize = CGSizeMake(CGFLOAT_MAX, CGFLOAT_MAX);
+  ASSizeRange sizeRange = ASSizeRangeUnconstrained;
+
+  self.subnode.style.preferredSize = subnodeSize;
+
+  [self.scrollNode layoutThatFits:sizeRange parentSize:parentSize];
+  [self.scrollNode layout];
+
+  ASXCTAssertEqualSizes(self.scrollNode.calculatedSize, subnodeSize);
+  ASXCTAssertEqualSizes(self.scrollNode.view.contentSize, subnodeSize);
+}
+
+@end