Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix UICollectionView cell enumeration hitting asserts in Xcode 16 #1307

Merged
merged 2 commits into from
Sep 24, 2024
Merged

Fix UICollectionView cell enumeration hitting asserts in Xcode 16 #1307

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a3a3fb8
Fix UICollectionView cell enumeration hitting asserts in Xcode 16
Sep 24, 2024
5b54b28
PR Feedback
Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 21 additions & 28 deletions Sources/KIF/Additions/UIView-KIFAdditions.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
#import "KIFUITestActor.h"
#import <WebKit/WebKit.h>

#define DRAG_TOUCH_DELAY 0.01
#define CELL_SCROLL_DELAY_STABILIZATION 0.05

double KIFDegreesToRadians(double deg) {
return (deg) / 180.0 * M_PI;
}
Expand Down Expand Up @@ -310,7 +313,6 @@ - (UIAccessibilityElement *)accessibilityElementMatchingBlock:(BOOL(^)(UIAccessi
}
}];

CFTimeInterval delay = 0.05;
for (NSUInteger section = 0, numberOfSections = [tableView numberOfSections]; section < numberOfSections; section++) {
for (NSUInteger row = 0, numberOfRows = [tableView numberOfRowsInSection:section]; row < numberOfRows; row++) {
if (!self.window) {
Expand All @@ -328,7 +330,7 @@ - (UIAccessibilityElement *)accessibilityElementMatchingBlock:(BOOL(^)(UIAccessi
}

@autoreleasepool {
// Scroll to the cell and wait for the animation to complete. Using animations here may not be optimal.
// Scroll to the cell and wait for the animation to complete.
CGRect sectionRect = [tableView rectForRowAtIndexPath:indexPath];
[tableView scrollRectToVisible:sectionRect animated:NO];

Expand All @@ -342,24 +344,25 @@ - (UIAccessibilityElement *)accessibilityElementMatchingBlock:(BOOL(^)(UIAccessi
}

// Note: using KIFRunLoopRunInModeRelativeToAnimationSpeed here may cause tests to stall
CFRunLoopRunInMode(UIApplicationCurrentRunMode, delay, false);
CFRunLoopRunInMode(UIApplicationCurrentRunMode, CELL_SCROLL_DELAY_STABILIZATION, false);
return [self accessibilityElementMatchingBlock:matchBlock disableScroll:NO];
}
}

//if we're in a picker (scrollView), let's make sure we set the position back to how it was last set.
if(scrollView != nil && scrollContentOffset.x != -1.0)
if (scrollView != nil && scrollContentOffset.x != -1.0)
{
[scrollView setContentOffset:scrollContentOffset];
} else {
[tableView setContentOffset:initialPosition.origin];
}
CFRunLoopRunInMode(UIApplicationCurrentRunMode, delay, false);

CFRunLoopRunInMode(UIApplicationCurrentRunMode, CELL_SCROLL_DELAY_STABILIZATION, false);
} else if ([self isKindOfClass:[UICollectionView class]]) {
UICollectionView *collectionView = (UICollectionView *)self;

CGRect initialPosition = CGRectMake(collectionView.contentOffset.x, collectionView.contentOffset.y, collectionView.frame.size.width, collectionView.frame.size.height);
NSArray *indexPathsForVisibleItems = [collectionView indexPathsForVisibleItems];

for (NSUInteger section = 0, numberOfSections = [collectionView numberOfSections]; section < numberOfSections; section++) {
for (NSUInteger item = 0, numberOfItems = [collectionView numberOfItemsInSection:section]; item < numberOfItems; item++) {
if (!self.window) {
Expand All @@ -373,36 +376,28 @@ - (UIAccessibilityElement *)accessibilityElementMatchingBlock:(BOOL(^)(UIAccessi
}

@autoreleasepool {
// Get the cell directly from the dataSource because UICollectionView will only vend visible cells
UICollectionViewCell *cell = [collectionView.dataSource collectionView:collectionView cellForItemAtIndexPath:indexPath];

// The cell contents might change just prior to being displayed, so we simulate the cell appearing onscreen
if ([collectionView.delegate respondsToSelector:@selector(collectionView:willDisplayCell:forItemAtIndexPath:)]) {
[collectionView.delegate collectionView:collectionView willDisplayCell:cell forItemAtIndexPath:indexPath];
}

CGRect visibleRect = [collectionView layoutAttributesForItemAtIndexPath:indexPath].frame;
[collectionView scrollRectToVisible:visibleRect animated:NO];
[collectionView layoutIfNeeded];
UICollectionViewCell *cell = [collectionView cellForItemAtIndexPath:indexPath];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

NSAssert(cell, @"UICollectionViewCell returned from 'cellForItemAtIndexPath' is unexpectedly nil!");
UIAccessibilityElement *element = [cell accessibilityElementMatchingBlock:matchBlock notHidden:NO disableScroll:NO];

// Remove the cell from the collection view so that it doesn't stick around
[cell removeFromSuperview];

// 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!element || CGSizeEqualToSize(cell.frame.size, CGSizeZero)) {
if (!element) {
continue;
}
}

// Scroll to the cell and wait for the animation to complete
CGRect frame = [collectionView.collectionViewLayout layoutAttributesForItemAtIndexPath:indexPath].frame;
[collectionView scrollRectToVisible:frame animated:YES];
// Note: using KIFRunLoopRunInModeRelativeToAnimationSpeed here may cause tests to stall
CFRunLoopRunInMode(UIApplicationCurrentRunMode, 0.5, false);

// Now try finding the element again
CFRunLoopRunInMode(UIApplicationCurrentRunMode, CELL_SCROLL_DELAY_STABILIZATION, false);
return [self accessibilityElementMatchingBlock:matchBlock disableScroll:NO];
}
}

[collectionView setContentOffset:initialPosition.origin];
Copy link
Contributor

@kyleve kyleve Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do a layoutIfNeeded after scrollRectToVisible above; should we do that here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, doesn't really hurt. I'll add it for consistency.

[collectionView layoutIfNeeded];
CFRunLoopRunInMode(UIApplicationCurrentRunMode, CELL_SCROLL_DELAY_STABILIZATION, false);
}
}

Expand Down Expand Up @@ -592,8 +587,6 @@ - (void)twoFingerTapAtPoint:(CGPoint)point {
[[UIApplication sharedApplication] kif_sendEvent:event];
}

#define DRAG_TOUCH_DELAY 0.01

- (void)longPressAtPoint:(CGPoint)point duration:(NSTimeInterval)duration
{
UITouch *touch = [[UITouch alloc] initAtPoint:point inView:self];
Expand Down