Fix UICollectionView cell enumeration hitting asserts in Xcode 16 #1307
Conversation
Enumerate the cells by scrolling the collection view's content offset and calling `layoutIfNeeded` instead of programmatically enumerating them using the data source and delegate APIs directly.
justinseanmartin
requested review from
RoyalPineapple,
dostrander and
congt
|
I tested your fix, and it appears to fix my problem related to the collection view. Thank you. |
| CGRect visibleRect = [collectionView layoutAttributesForItemAtIndexPath:indexPath].frame; | ||
| [collectionView scrollRectToVisible:visibleRect animated:NO]; | ||
| [collectionView layoutIfNeeded]; | ||
| UICollectionViewCell *cell = [collectionView cellForItemAtIndexPath:indexPath]; |
There was a problem hiding this comment.
I don't know the KIF internal best practices, but is there some warning or error we should throw if this returns a nil cell? Or meh?
There was a problem hiding this comment.
I could NSAssert that the cell is non-nil to abort the search. I'll add that and see how it goes. Seems to pass locally at least.
There was a problem hiding this comment.
Honestly, I don't know if an assert is worth it – maybe a log or something? Unclear on the risk of false positives vs. false negatives where the test would just fail.
|
|
||
| // Skip this cell if it isn't the one we're looking for | ||
| // Sometimes we get cells with no size here which can cause an endless loop, so we ignore those |
There was a problem hiding this comment.
I'm not sure why this would cause an endless loop, but I'm guessing that realizing these cells in a more realistic way may sidestep this issue. We can always add this back later if need be.
| return [self accessibilityElementMatchingBlock:matchBlock disableScroll:NO]; | ||
| } | ||
| } | ||
|
|
||
| [collectionView setContentOffset:initialPosition.origin]; |
There was a problem hiding this comment.
We do a layoutIfNeeded after scrollRectToVisible above; should we do that here too?
There was a problem hiding this comment.
Sure, doesn't really hurt. I'll add it for consistency.
justinseanmartin
deleted the
jmartin/collection-view-cell-enumeration-assert
branch
In Xcode 16, an assertion was to UICollectionView:
Rather than calling
-[UICollectionViewDataSource collectionView:cellForItemAtIndexPath:]and[UICollectionViewDelegate collectionView:willDisplayCell:forItemAtIndexPath:], instead we will scroll the collection view's content offset to make the offscreen cells be realized by UIKit. If the cell matches the predicate, then we will return the matching element (which should now be on screen). If a match isn't found in any of the cells, scroll back to the original scroll position and keep searching the view hierarchy for a matching element.One notable thing to call out is that unlike cell enumeration in a UITableView using
cellForRowAtIndexPath:, the UICollectionView equivalentcellForItemAtIndexPath:will return a nil cell unless a layour pass is performed. We could either spin the runloop and let UIKit update on its own or directly calllayoutIfNeededon the collection view. We're going with the latter, because it is faster on Xcode 16 and avoids a bunch of unnecessary and distracting redrawing of the screen at each scroll position.A new API
[viewTester usingCurrentFrame]was introduced in #1296 that can avoid this logic of implicitly scrolling collection views where that is needed or desired.Fixes #1303
CC: @jdev7 @danielbarela @ispatel22 - Could you give this change a test locally and see if this addresses the issues you're seeing and see if it surfaces any other problems? I've landed a couple other changes recently, so it is possible that those could also have altered behavior in minor ways (#1296, #1300 and #1301).