From 671b975d92a462be63cb8edad45b455da2e81d5d Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Fri, 19 Feb 2016 05:54:46 -0800 Subject: [PATCH] Fix a bug with ListView with sticky headers + RefreshControl 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 https://github.com/facebook/react-native/pull/5445 Differential Revision: D2953984 Pulled By: nicklockwood fb-gh-sync-id: c17a6a75ab31ef54d478706ed17a8115a11d734e shipit-source-id: c17a6a75ab31ef54d478706ed17a8115a11d734e --- React/Views/RCTRefreshControl.m | 6 ++---- React/Views/RCTScrollView.m | 23 ++++++++++++++++++----- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/React/Views/RCTRefreshControl.m b/React/Views/RCTRefreshControl.m index b066f1b632..282cd7f303 100644 --- a/React/Views/RCTRefreshControl.m +++ b/React/Views/RCTRefreshControl.m @@ -30,7 +30,7 @@ RCT_NOT_IMPLEMENTED(- (instancetype)initWithCoder:(NSCoder *)aDecoder) - (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) { @@ -46,9 +46,7 @@ RCT_NOT_IMPLEMENTED(- (instancetype)initWithCoder:(NSCoder *)aDecoder) 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 diff --git a/React/Views/RCTScrollView.m b/React/Views/RCTScrollView.m index 4cc9d4bc87..af0091e03a 100644 --- a/React/Views/RCTScrollView.m +++ b/React/Views/RCTScrollView.m @@ -145,7 +145,7 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init) @property (nonatomic, copy) NSIndexSet *stickyHeaderIndices; @property (nonatomic, assign) BOOL centerContent; -@property (nonatomic, strong) UIRefreshControl *refreshControl; +@property (nonatomic, strong) RCTRefreshControl *refreshControl; @end @@ -287,10 +287,12 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init) __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; } @@ -365,7 +367,7 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init) return hitView ?: [super hitTest:point withEvent:event]; } -- (void)setRefreshControl:(UIRefreshControl *)refreshControl +- (void)setRefreshControl:(RCTRefreshControl *)refreshControl { if (_refreshControl) { [_refreshControl removeFromSuperview]; @@ -826,6 +828,17 @@ RCT_SCROLL_EVENT_HANDLER(scrollViewDidZoom, RCTScrollEventTypeMove) _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]; } @@ -888,7 +901,7 @@ RCT_SET_AND_PRESERVE_OFFSET(setScrollIndicatorInsets, UIEdgeInsets); _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; }