Skip to content

Commit

Permalink
Fix scrollToObject: bug when scrolling to bottom with content inset
Browse files Browse the repository at this point in the history
Summary:
This has been around forever, surprised we've never run into it. Instagram doesn't really use the `bottom` position except for one place that is already at the bottom of a view. Looks like this bug has just been hanging around quietly for ages.

`UIScrollView` takes into account the content inset when changing the content offset. Doing math w/ the inset is both useless and incorrect.

Issue fixed: #932

- [x] All tests pass. Demo project builds and runs.
- [x] I added tests, an experiment, or detailed why my change isn't tested.
- [x] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes.
Closes #940

Differential Revision: D5881117

Pulled By: rnystrom

fbshipit-source-id: 5c9af4b6f8b59be977491ba734e9b0fa3c538cb9
  • Loading branch information
rnystrom authored and facebook-github-bot committed Sep 21, 2017
1 parent 595c09e commit b2860c3
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 6 deletions.
7 changes: 2 additions & 5 deletions CHANGELOG.md
Expand Up @@ -13,15 +13,12 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag

- Weakly reference the `UICollectionView` in coalescence so that it can be released if the rest of system is destroyed. [Ryan Nystrom](https://github.com/rnystrom) [(#tbd)](https://github.com/Instagram/IGListKit/pull/tbd)

- Fix bug with `-[IGListAdapter scrollToObject:supplementaryKinds:scrollDirection:scrollPosition:animated:]` where the content inset of the collection view was incorrectly being applied to the final offset. [Ryan Nystrom](https://github.com/rnystrom) [(#tbd)](https://github.com/Instagram/IGListKit/pull/tbd)

- Avoid crash when invalidating the layout while inside `-[UICollectionView performBatchUpdates:completion:]. [Ryan Nystrom](https://github.com/rnystrom) [(#tbd)](https://github.com/Instagram/IGListKit/pull/tbd)

- Duplicate view models in `IGListBindingSectionController` gets filtered out. [Weyert de Boer](https://github.com/weyert) [(#916)](https://github.com/Instagram/IGListKit/pull/916)

### Enhancements

- Added `-[IGListSectionController didHighlightItemAtIndex:]` and `-[IGListSectionController didUnhighlightItemAtIndex:]` APIs to support `UICollectionView` cell highlighting. [Kevin Delannoy](https://github.com/delannoyk) [(#933)](https://github.com/Instagram/IGListKit/pull/933)
>>>>>>> source: 872ef395b114 arcpatch-D5872117 - facebook-github-bot: [IGList...
3.1.1
-----

Expand Down
2 changes: 1 addition & 1 deletion Source/IGListAdapter.m
Expand Up @@ -275,7 +275,7 @@ - (void)scrollToObject:(id)object
case UICollectionViewScrollDirectionVertical: {
switch (scrollPosition) {
case UICollectionViewScrollPositionBottom:
contentOffset.y = offsetMax - collectionViewHeight - contentInset.top;
contentOffset.y = offsetMax - collectionViewHeight;
break;
case UICollectionViewScrollPositionCenteredVertically: {
const CGFloat insets = (contentInset.top - contentInset.bottom) / 2.0;
Expand Down
28 changes: 28 additions & 0 deletions Tests/IGListAdapterTests.m
Expand Up @@ -714,6 +714,34 @@ - (void)test_whenScrollVerticallyToItemWithPositionning {
IGAssertEqualPoint([self.collectionView contentOffset], 0, self.collectionView.contentSize.height - self.collectionView.frame.size.height);
}

- (void)test_whenScrollVerticallyToBottom_withContentInsets_thatBottomFlushWithCollectionViewBounds {
self.dataSource.objects = @[@100];
[self.adapter reloadDataWithCompletion:nil];

// no insets
self.collectionView.contentInset = UIEdgeInsetsMake(0, 0, 0, 0);
[self.collectionView layoutIfNeeded];
[self.adapter scrollToObject:@100 supplementaryKinds:nil scrollDirection:UICollectionViewScrollDirectionVertical scrollPosition:UICollectionViewScrollPositionBottom animated:NO];
IGAssertEqualPoint([self.collectionView contentOffset], 0, 900);

// top 100
self.collectionView.contentInset = UIEdgeInsetsMake(100, 0, 0, 0);
[self.adapter scrollToObject:@100 supplementaryKinds:nil scrollDirection:UICollectionViewScrollDirectionVertical scrollPosition:UICollectionViewScrollPositionBottom animated:NO];
IGAssertEqualPoint([self.collectionView contentOffset], 0, 900);

// bottom 100
self.collectionView.contentInset = UIEdgeInsetsMake(0, 0, 100, 0);
[self.collectionView layoutIfNeeded];
[self.adapter scrollToObject:@100 supplementaryKinds:nil scrollDirection:UICollectionViewScrollDirectionVertical scrollPosition:UICollectionViewScrollPositionBottom animated:NO];
IGAssertEqualPoint([self.collectionView contentOffset], 0, 900);

// top 50, bottom 100
self.collectionView.contentInset = UIEdgeInsetsMake(50, 0, 100, 0);
[self.collectionView layoutIfNeeded];
[self.adapter scrollToObject:@100 supplementaryKinds:nil scrollDirection:UICollectionViewScrollDirectionVertical scrollPosition:UICollectionViewScrollPositionBottom animated:NO];
IGAssertEqualPoint([self.collectionView contentOffset], 0, 900);
}

- (void)test_whenScrollHorizontallyToItem {
// # of items for each object == [item integerValue], so @2 has 2 items (cells)
IGListTestAdapterHorizontalDataSource *dataSource = [[IGListTestAdapterHorizontalDataSource alloc] init];
Expand Down

0 comments on commit b2860c3

Please sign in to comment.