Skip to content

Commit

Permalink
Fix a bug with ListView with sticky headers + RefreshControl
Browse files Browse the repository at this point in the history
Summary:The bug is caused by a weird race condition. What happens is that when calling `UIRefreshControl#endRefreshing` the `UIScrollView` delegate `scrollViewDidScroll` function is called synchronously and then `dockClosestSectionHeader` crashes because the sticky header indexes are updated but not the contentView children.

I fixed it by adding an updating property on `RCTRefreshControl` and setting it before calling `endRefreshing` so we can know not to call `dockClosestSectionHeader` at that moment.

Tested with both `RefreshControl` and `onRefreshStart` prop.

I reproduced the bug by replacing ListViewExample.js in UIExplorer with https://gist.github.com/janicduplessis/05fc58e852f3e80e51b9

Fixes #5440

cc nicklockwood
Closes #5445

Differential Revision: D2953984

Pulled By: nicklockwood

fb-gh-sync-id: c17a6a75ab31ef54d478706ed17a8115a11d734e
shipit-source-id: c17a6a75ab31ef54d478706ed17a8115a11d734e
  • Loading branch information
janicduplessis authored and facebook-github-bot-6 committed Feb 19, 2016
1 parent 183d6a0 commit 671b975
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 9 deletions.
6 changes: 2 additions & 4 deletions React/Views/RCTRefreshControl.m
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ - (instancetype)init
- (void)layoutSubviews
{
[super layoutSubviews];

// If the control is refreshing when mounted we need to call
// beginRefreshing in layoutSubview or it doesn't work.
if (_isInitialRender && _initialRefreshingState) {
Expand All @@ -46,9 +46,7 @@ - (void)beginRefreshing
CGPoint offset = {scrollView.contentOffset.x, scrollView.contentOffset.y - self.frame.size.height};
// Don't animate when the prop is set initialy.
if (_isInitialRender) {
// Must use `[scrollView setContentOffset:offset animated:NO]` instead of just setting
// `scrollview.contentOffset` or it doesn't work, don't ask me why!
[scrollView setContentOffset:offset animated:NO];
scrollView.contentOffset = offset;
[super beginRefreshing];
} else {
// `beginRefreshing` must be called after the animation is done. This is why it is impossible
Expand Down
23 changes: 18 additions & 5 deletions React/Views/RCTScrollView.m
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ @interface RCTCustomScrollView : UIScrollView<UIGestureRecognizerDelegate>

@property (nonatomic, copy) NSIndexSet *stickyHeaderIndices;
@property (nonatomic, assign) BOOL centerContent;
@property (nonatomic, strong) UIRefreshControl *refreshControl;
@property (nonatomic, strong) RCTRefreshControl *refreshControl;

@end

Expand Down Expand Up @@ -287,10 +287,12 @@ - (void)dockClosestSectionHeader
__block UIView *nextHeader = nil;
NSUInteger subviewCount = contentView.reactSubviews.count;
[_stickyHeaderIndices enumerateIndexesWithOptions:0 usingBlock:
^(NSUInteger idx, __unused BOOL *stop) {
^(NSUInteger idx, BOOL *stop) {

// If the subviews are out of sync with the sticky header indices don't
// do anything.
if (idx >= subviewCount) {
RCTLogError(@"Sticky header index %zd was outside the range {0, %zd}", idx, subviewCount);
*stop = YES;
return;
}

Expand Down Expand Up @@ -365,7 +367,7 @@ - (UIView *)hitTest:(CGPoint)point withEvent:(UIEvent *)event
return hitView ?: [super hitTest:point withEvent:event];
}

- (void)setRefreshControl:(UIRefreshControl *)refreshControl
- (void)setRefreshControl:(RCTRefreshControl *)refreshControl
{
if (_refreshControl) {
[_refreshControl removeFromSuperview];
Expand Down Expand Up @@ -826,6 +828,17 @@ - (void)reactBridgeDidFinishTransaction
_scrollView.contentSize = contentSize;
_scrollView.contentOffset = newOffset;
}

if (RCT_DEBUG) {
// Validate that sticky headers are not out of range.
NSUInteger subviewCount = _scrollView.contentView.reactSubviews.count;
NSUInteger lastIndex = _scrollView.stickyHeaderIndices.lastIndex;
if (lastIndex != NSNotFound && lastIndex >= subviewCount) {
RCTLogWarn(@"Sticky header index %zd was outside the range {0, %zd}",
lastIndex, subviewCount);
}
}

[_scrollView dockClosestSectionHeader];
}

Expand Down Expand Up @@ -888,7 +901,7 @@ - (void)setOnRefreshStart:(RCTDirectEventBlock)onRefreshStart
_onRefreshStart = [onRefreshStart copy];

if (!_scrollView.refreshControl) {
UIRefreshControl *refreshControl = [[UIRefreshControl alloc] init];
RCTRefreshControl *refreshControl = [[RCTRefreshControl alloc] init];
[refreshControl addTarget:self action:@selector(refreshControlValueChanged) forControlEvents:UIControlEventValueChanged];
_scrollView.refreshControl = refreshControl;
}
Expand Down

0 comments on commit 671b975

Please sign in to comment.