Fix CollectionView scroll behaviour when section is empty

[gmoledina]

Changes in this pull request

Issue fixed: When a section is empty (0 cells) but has a header/footer supplementary views, calling scrollToObject:supplementaryKinds:scrollDirection:scrollPosition:animated wouldn't do anything as the method implementation has an early return if the section doesn't have any cells:

As the section does have supplementary views and is displayed on screen, there's no reason not to scroll to it. This minor change fixes this detail.

Checklist

Change in internal implementation of one method - all current tests pass. No tests added.

• All tests pass. Demo project builds and runs.
• I added tests, an experiment, or detailed why my change isn't tested.
• I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
• I have reviewed the contributing guide

[gmoledina]

1 Travis check failed, looks like the parameter for the device is wrong : "Unsupported device specifier option.".

[jessesquires]

Seems reasonable to me 👍 💯

cc @rnystrom

[rnystrom]

@gmoledina awesome! Do you mind adding a unit test to make sure this doesn't regress in the future?

[facebook-github-bot]

@gmoledina updated the pull request - view changes

[gmoledina]

@jessesquires @rnystrom Sure thing ! I've added some simple tests.

While adding the tests, I noticed 2 things :

1- calling [layout layoutAttributesForSupplementaryViewOfKind:supplementaryKind atIndexPath:indexPath]; with supplementaryKind footer for example, returns the frame the footer would have even if the section doesn't have a footer.

2- if we choose UICollectionViewScrollPositionBottom as the position parameter to call the scrollToObject... method, the contentOffset might be a negative number even if the collectionView doesn't have any insets.

For the 1st point, I would ignore it as it's the default UICVFlowLayout behaviour.
But for the 2nd point, I think we need to define the behaviour we'd like to have. Should I create an issue for this point ?

[facebook-github-bot]

@gmoledina updated the pull request - view changes

[facebook-github-bot]

@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[gmoledina]

@rnystrom Let me know if you need more details regarding the 2 points I mentioned in my last comment ! I could write failing tests in a separate branch for point 2.

[rnystrom]

@gmoledina good points! Agree let's skip 1 b/c that's flow layout's behavior. I don't want to work around it too much as some people could expect it or it might change.

Let's whip up another issue for 2 though and include some failing unit tests in the issue if you have them on hand.

[rnystrom]

@gmoledina quick heads up that the test_whenScrollVerticallyToItemInASectionWithNoCellsAndNoSupplymentaryView test is actually failing both on Travis and internally atm. Mind taking a look?

[rnystrom]

@gmoledina checking in on this?

[facebook-github-bot]

@gmoledina updated the pull request - view changes - changes since last import

[facebook-github-bot]

@gmoledina updated the pull request - view changes - changes since last import

[gmoledina]

@rnystrom Indeed, tests were failing on my side as well - weird. The reason was that collectionView.contentSize.height was smaller then collectionView.frame.size.height and thus no scrolling would occur. I must have changed the array contents last minute as the tests were passing when I wrote them 🤔
PR updated with the fix !
I'll open a new PR for the other issue as well.

[rnystrom]

Hell ya! Going to land this when I'm back from a vacation early next week. That way if something breaks, nobody has to scramble to fix while I'm out. Pumped for this!

[gmoledina]

@rnystrom sounds good - enjoy your vacation !

[facebook-github-bot]

@gmoledina updated the pull request - view changes - changes since last import

[facebook-github-bot]

@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[rnystrom]

FYI this caused a regression internally b/c calling numberOfItemsInSection: triggers a prepareLayout pass early. I'll work on a unit test that repros this issue and we can retry this PR. Backing it out for now.