From 620080487b1832f1e7659052f3e3b2cdb3af9491 Mon Sep 17 00:00:00 2001 From: Graham Mendick Date: Sun, 17 Jul 2022 14:43:51 +0100 Subject: [PATCH 1/8] Recreated UIScrollView when component recycled iOS doesn't like reusing the underlying UIScrollView. It won't reset the content insets because it think it's already done it. So recreated the scroll view when the component is recycled. Also had to trigger a contentSize update after recycling otherwise the scroll view started with no inset and wouldn't scroll. --- .../ScrollView/RCTScrollViewComponentView.mm | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm index e699640940d88e..05eb3a05463e57 100644 --- a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm @@ -98,6 +98,8 @@ @implementation RCTScrollViewComponentView { // some other part of the system scrolls scroll view. BOOL _isUserTriggeredScrolling; BOOL _shouldUpdateContentInsetAdjustmentBehavior; + BOOL _createScrollView; + BOOL _shouldUpdateContextSize; CGPoint _contentOffsetWhenClipped; } @@ -115,7 +117,16 @@ - (instancetype)initWithFrame:(CGRect)frame if (self = [super initWithFrame:frame]) { static const auto defaultProps = std::make_shared(); _props = defaultProps; + _createScrollView = YES; + _scrollEventThrottle = INFINITY; + } + + return self; +} +- (void)ensureScrollView +{ + if (_createScrollView) { _scrollView = [[RCTEnhancedScrollView alloc] initWithFrame:self.bounds]; _scrollView.autoresizingMask = UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight; _scrollView.delaysContentTouches = NO; @@ -128,18 +139,8 @@ - (instancetype)initWithFrame:(CGRect)frame [_scrollView addSubview:_containerView]; [self.scrollViewDelegateSplitter addDelegate:self]; - - _scrollEventThrottle = INFINITY; + _createScrollView = NO; } - - return self; -} - -- (void)dealloc -{ - // Removing all delegates from the splitter nils the actual delegate which prevents a crash on UIScrollView - // deallocation. - [self.scrollViewDelegateSplitter removeAllDelegates]; } - (RCTGenericDelegateSplitter> *)scrollViewDelegateSplitter @@ -178,6 +179,7 @@ - (void)updateLayoutMetrics:(const LayoutMetrics &)layoutMetrics - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const &)oldProps { + [self ensureScrollView]; const auto &oldScrollViewProps = *std::static_pointer_cast(_props); const auto &newScrollViewProps = *std::static_pointer_cast(props); @@ -306,12 +308,13 @@ - (void)updateState:(State::Shared const &)state oldState:(State::Shared const & CGSize contentSize = RCTCGSizeFromSize(data.getContentSize()); - if (CGSizeEqualToSize(_contentSize, contentSize)) { + if (!_shouldUpdateContextSize && CGSizeEqualToSize(_contentSize, contentSize)) { return; } _contentSize = contentSize; _containerView.frame = CGRect{RCTCGPointFromPoint(data.contentBoundingRect.origin), contentSize}; + _shouldUpdateContextSize = NO; [self _preserveContentOffsetIfNeededWithBlock:^{ self->_scrollView.contentSize = contentSize; @@ -336,6 +339,7 @@ - (void)_preserveContentOffsetIfNeededWithBlock:(void (^)())block - (void)mountChildComponentView:(UIView *)childComponentView index:(NSInteger)index { + [self ensureScrollView]; [_containerView insertSubview:childComponentView atIndex:index]; } @@ -401,6 +405,9 @@ - (void)prepareForRecycle _shouldUpdateContentInsetAdjustmentBehavior = YES; _state.reset(); _isUserTriggeredScrolling = NO; + [self.scrollViewDelegateSplitter removeAllDelegates]; + _createScrollView = YES; + _shouldUpdateContextSize = YES; [super prepareForRecycle]; } From 36ab76d5e6de1c9faa7adc0d3a01140056e6f202 Mon Sep 17 00:00:00 2001 From: Graham Mendick Date: Sun, 17 Jul 2022 14:50:04 +0100 Subject: [PATCH 2/8] Reset props back to default when recycled The underlying UIScrollView is create new each time so don't want to use default props instead of current props for comparison. Backed out the reset of other scroll props because not needed now it's created new each time --- .../ScrollView/RCTScrollViewComponentView.mm | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm index 05eb3a05463e57..02f9db6aac6110 100644 --- a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm @@ -97,7 +97,6 @@ @implementation RCTScrollViewComponentView { // This helps to only update state from `scrollViewDidScroll` in case // some other part of the system scrolls scroll view. BOOL _isUserTriggeredScrolling; - BOOL _shouldUpdateContentInsetAdjustmentBehavior; BOOL _createScrollView; BOOL _shouldUpdateContextSize; @@ -132,7 +131,6 @@ - (void)ensureScrollView _scrollView.delaysContentTouches = NO; ((RCTEnhancedScrollView *)_scrollView).overridingDelegate = self; _isUserTriggeredScrolling = NO; - _shouldUpdateContentInsetAdjustmentBehavior = YES; [self addSubview:_scrollView]; _containerView = [[UIView alloc] initWithFrame:CGRectZero]; @@ -270,8 +268,7 @@ - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const & } } - if ((oldScrollViewProps.contentInsetAdjustmentBehavior != newScrollViewProps.contentInsetAdjustmentBehavior) || - _shouldUpdateContentInsetAdjustmentBehavior) { + if (oldScrollViewProps.contentInsetAdjustmentBehavior != newScrollViewProps.contentInsetAdjustmentBehavior) { auto const contentInsetAdjustmentBehavior = newScrollViewProps.contentInsetAdjustmentBehavior; if (contentInsetAdjustmentBehavior == ContentInsetAdjustmentBehavior::Never) { scrollView.contentInsetAdjustmentBehavior = UIScrollViewContentInsetAdjustmentNever; @@ -282,7 +279,6 @@ - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const & } else if (contentInsetAdjustmentBehavior == ContentInsetAdjustmentBehavior::Always) { scrollView.contentInsetAdjustmentBehavior = UIScrollViewContentInsetAdjustmentAlways; } - _shouldUpdateContentInsetAdjustmentBehavior = NO; } MAP_SCROLL_VIEW_PROP(disableIntervalMomentum); @@ -396,13 +392,8 @@ - (void)_updateStateWithContentOffset - (void)prepareForRecycle { - const auto &props = *std::static_pointer_cast(_props); - _scrollView.contentOffset = RCTCGPointFromPoint(props.contentOffset); - // We set the default behavior to "never" so that iOS - // doesn't do weird things to UIScrollView insets automatically - // and keeps it as an opt-in behavior. - _scrollView.contentInsetAdjustmentBehavior = UIScrollViewContentInsetAdjustmentNever; - _shouldUpdateContentInsetAdjustmentBehavior = YES; + const auto defaultProps = std::make_shared(); + _props = defaultProps; _state.reset(); _isUserTriggeredScrolling = NO; [self.scrollViewDelegateSplitter removeAllDelegates]; From 0877c74abbab8b63cd1ba029a87243c6ff87f4b5 Mon Sep 17 00:00:00 2001 From: Graham Mendick Date: Sun, 17 Jul 2022 14:52:16 +0100 Subject: [PATCH 3/8] Reset event throttle for consistency --- .../ComponentViews/ScrollView/RCTScrollViewComponentView.mm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm index 02f9db6aac6110..9686dc642b9abf 100644 --- a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm @@ -117,7 +117,6 @@ - (instancetype)initWithFrame:(CGRect)frame static const auto defaultProps = std::make_shared(); _props = defaultProps; _createScrollView = YES; - _scrollEventThrottle = INFINITY; } return self; @@ -137,6 +136,8 @@ - (void)ensureScrollView [_scrollView addSubview:_containerView]; [self.scrollViewDelegateSplitter addDelegate:self]; + + _scrollEventThrottle = INFINITY; _createScrollView = NO; } } From 2b1be32d9b590b4e9c815a571498548b407eb7fa Mon Sep 17 00:00:00 2001 From: Graham Mendick Date: Sun, 17 Jul 2022 15:53:18 +0100 Subject: [PATCH 4/8] Removed old scroll view before adding new --- .../ComponentViews/ScrollView/RCTScrollViewComponentView.mm | 1 + 1 file changed, 1 insertion(+) diff --git a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm index 9686dc642b9abf..ae20cb8878e5c0 100644 --- a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm @@ -125,6 +125,7 @@ - (instancetype)initWithFrame:(CGRect)frame - (void)ensureScrollView { if (_createScrollView) { + [_scrollView removeFromSuperview]; _scrollView = [[RCTEnhancedScrollView alloc] initWithFrame:self.bounds]; _scrollView.autoresizingMask = UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight; _scrollView.delaysContentTouches = NO; From 06e6c8f792cec7715163bbff0ced57c78da21ebd Mon Sep 17 00:00:00 2001 From: Graham Mendick Date: Sun, 17 Jul 2022 15:58:03 +0100 Subject: [PATCH 5/8] Fixed typo --- .../ScrollView/RCTScrollViewComponentView.mm | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm index ae20cb8878e5c0..9c9a22cb8ea348 100644 --- a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm @@ -98,7 +98,7 @@ @implementation RCTScrollViewComponentView { // some other part of the system scrolls scroll view. BOOL _isUserTriggeredScrolling; BOOL _createScrollView; - BOOL _shouldUpdateContextSize; + BOOL _shouldUpdateContentSize; CGPoint _contentOffsetWhenClipped; } @@ -306,13 +306,13 @@ - (void)updateState:(State::Shared const &)state oldState:(State::Shared const & CGSize contentSize = RCTCGSizeFromSize(data.getContentSize()); - if (!_shouldUpdateContextSize && CGSizeEqualToSize(_contentSize, contentSize)) { + if (!_shouldUpdateContentSize && CGSizeEqualToSize(_contentSize, contentSize)) { return; } _contentSize = contentSize; _containerView.frame = CGRect{RCTCGPointFromPoint(data.contentBoundingRect.origin), contentSize}; - _shouldUpdateContextSize = NO; + _shouldUpdateContentSize = NO; [self _preserveContentOffsetIfNeededWithBlock:^{ self->_scrollView.contentSize = contentSize; @@ -400,7 +400,7 @@ - (void)prepareForRecycle _isUserTriggeredScrolling = NO; [self.scrollViewDelegateSplitter removeAllDelegates]; _createScrollView = YES; - _shouldUpdateContextSize = YES; + _shouldUpdateContentSize = YES; [super prepareForRecycle]; } From f28c6f6e46059c0faeb94d2765cc870a486d7b07 Mon Sep 17 00:00:00 2001 From: Graham Mendick Date: Mon, 18 Jul 2022 18:15:31 +0100 Subject: [PATCH 6/8] Changed frame to 0 when recycled for large titles Previous solution of recreating scroll view each time didn't work for the large-title-bug. Go to large title, scroll to collapse, go back then go to large title and it would be collapsed - it would scroll slightly so could see the first line of content but it would be collapsed. Stumbled across changing frame from 0 then back to old value and it fixes all the inset behavior auto bugs. Guess it resets everything - without resetting the frame it remembered some settings and made it go through a completely different path when remounting from when recycling --- .../ScrollView/RCTScrollViewComponentView.mm | 52 +++++++++++-------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm index 9c9a22cb8ea348..0e2ea8b62b5d50 100644 --- a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm @@ -97,10 +97,10 @@ @implementation RCTScrollViewComponentView { // This helps to only update state from `scrollViewDidScroll` in case // some other part of the system scrolls scroll view. BOOL _isUserTriggeredScrolling; - BOOL _createScrollView; - BOOL _shouldUpdateContentSize; + BOOL _shouldUpdateContentInsetAdjustmentBehavior; CGPoint _contentOffsetWhenClipped; + CGRect _oldFrame; } + (RCTScrollViewComponentView *_Nullable)findScrollViewComponentViewForView:(UIView *)view @@ -116,21 +116,14 @@ - (instancetype)initWithFrame:(CGRect)frame if (self = [super initWithFrame:frame]) { static const auto defaultProps = std::make_shared(); _props = defaultProps; - _createScrollView = YES; - } - - return self; -} + _oldFrame = frame; -- (void)ensureScrollView -{ - if (_createScrollView) { - [_scrollView removeFromSuperview]; _scrollView = [[RCTEnhancedScrollView alloc] initWithFrame:self.bounds]; _scrollView.autoresizingMask = UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight; _scrollView.delaysContentTouches = NO; ((RCTEnhancedScrollView *)_scrollView).overridingDelegate = self; _isUserTriggeredScrolling = NO; + _shouldUpdateContentInsetAdjustmentBehavior = YES; [self addSubview:_scrollView]; _containerView = [[UIView alloc] initWithFrame:CGRectZero]; @@ -139,8 +132,16 @@ - (void)ensureScrollView [self.scrollViewDelegateSplitter addDelegate:self]; _scrollEventThrottle = INFINITY; - _createScrollView = NO; } + + return self; +} + +- (void)dealloc +{ + // Removing all delegates from the splitter nils the actual delegate which prevents a crash on UIScrollView + // deallocation. + [self.scrollViewDelegateSplitter removeAllDelegates]; } - (RCTGenericDelegateSplitter> *)scrollViewDelegateSplitter @@ -179,7 +180,10 @@ - (void)updateLayoutMetrics:(const LayoutMetrics &)layoutMetrics - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const &)oldProps { - [self ensureScrollView]; + if (!CGRectEqualToRect(_oldFrame, CGRectNull)) { + [self setFrame:_oldFrame]; + _oldFrame = CGRectNull; + } const auto &oldScrollViewProps = *std::static_pointer_cast(_props); const auto &newScrollViewProps = *std::static_pointer_cast(props); @@ -270,7 +274,8 @@ - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const & } } - if (oldScrollViewProps.contentInsetAdjustmentBehavior != newScrollViewProps.contentInsetAdjustmentBehavior) { + if ((oldScrollViewProps.contentInsetAdjustmentBehavior != newScrollViewProps.contentInsetAdjustmentBehavior) || + _shouldUpdateContentInsetAdjustmentBehavior) { auto const contentInsetAdjustmentBehavior = newScrollViewProps.contentInsetAdjustmentBehavior; if (contentInsetAdjustmentBehavior == ContentInsetAdjustmentBehavior::Never) { scrollView.contentInsetAdjustmentBehavior = UIScrollViewContentInsetAdjustmentNever; @@ -281,6 +286,7 @@ - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const & } else if (contentInsetAdjustmentBehavior == ContentInsetAdjustmentBehavior::Always) { scrollView.contentInsetAdjustmentBehavior = UIScrollViewContentInsetAdjustmentAlways; } + _shouldUpdateContentInsetAdjustmentBehavior = NO; } MAP_SCROLL_VIEW_PROP(disableIntervalMomentum); @@ -306,13 +312,12 @@ - (void)updateState:(State::Shared const &)state oldState:(State::Shared const & CGSize contentSize = RCTCGSizeFromSize(data.getContentSize()); - if (!_shouldUpdateContentSize && CGSizeEqualToSize(_contentSize, contentSize)) { + if (CGSizeEqualToSize(_contentSize, contentSize)) { return; } _contentSize = contentSize; _containerView.frame = CGRect{RCTCGPointFromPoint(data.contentBoundingRect.origin), contentSize}; - _shouldUpdateContentSize = NO; [self _preserveContentOffsetIfNeededWithBlock:^{ self->_scrollView.contentSize = contentSize; @@ -337,7 +342,6 @@ - (void)_preserveContentOffsetIfNeededWithBlock:(void (^)())block - (void)mountChildComponentView:(UIView *)childComponentView index:(NSInteger)index { - [self ensureScrollView]; [_containerView insertSubview:childComponentView atIndex:index]; } @@ -394,13 +398,17 @@ - (void)_updateStateWithContentOffset - (void)prepareForRecycle { - const auto defaultProps = std::make_shared(); - _props = defaultProps; + const auto &props = *std::static_pointer_cast(_props); + _scrollView.contentOffset = RCTCGPointFromPoint(props.contentOffset); + // We set the default behavior to "never" so that iOS + // doesn't do weird things to UIScrollView insets automatically + // and keeps it as an opt-in behavior. + _scrollView.contentInsetAdjustmentBehavior = UIScrollViewContentInsetAdjustmentNever; + _shouldUpdateContentInsetAdjustmentBehavior = YES; _state.reset(); _isUserTriggeredScrolling = NO; - [self.scrollViewDelegateSplitter removeAllDelegates]; - _createScrollView = YES; - _shouldUpdateContentSize = YES; + _oldFrame = self.frame; + [self setFrame:CGRectZero]; [super prepareForRecycle]; } From a81587f6196d0278a9b58bf3a28e66f0179df3c1 Mon Sep 17 00:00:00 2001 From: Graham Mendick Date: Mon, 18 Jul 2022 18:22:55 +0100 Subject: [PATCH 7/8] Started as null - only need update after a recycle --- .../ComponentViews/ScrollView/RCTScrollViewComponentView.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm index 0e2ea8b62b5d50..8c895878ece217 100644 --- a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm @@ -116,7 +116,6 @@ - (instancetype)initWithFrame:(CGRect)frame if (self = [super initWithFrame:frame]) { static const auto defaultProps = std::make_shared(); _props = defaultProps; - _oldFrame = frame; _scrollView = [[RCTEnhancedScrollView alloc] initWithFrame:self.bounds]; _scrollView.autoresizingMask = UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight; @@ -132,6 +131,7 @@ - (instancetype)initWithFrame:(CGRect)frame [self.scrollViewDelegateSplitter addDelegate:self]; _scrollEventThrottle = INFINITY; + _oldFrame = CGRectNull; } return self; From bba6de811a9761668943482d7931e8cfbbc46a9c Mon Sep 17 00:00:00 2001 From: Graham Mendick Date: Tue, 19 Jul 2022 17:13:16 +0100 Subject: [PATCH 8/8] Changed frame to 0 then back again on recycle As per review comments, there's no need to delay the reset. Can do it all in when recycling --- .../ScrollView/RCTScrollViewComponentView.mm | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm index 8c895878ece217..1a5aa5b1f8b89a 100644 --- a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm @@ -100,7 +100,6 @@ @implementation RCTScrollViewComponentView { BOOL _shouldUpdateContentInsetAdjustmentBehavior; CGPoint _contentOffsetWhenClipped; - CGRect _oldFrame; } + (RCTScrollViewComponentView *_Nullable)findScrollViewComponentViewForView:(UIView *)view @@ -131,7 +130,6 @@ - (instancetype)initWithFrame:(CGRect)frame [self.scrollViewDelegateSplitter addDelegate:self]; _scrollEventThrottle = INFINITY; - _oldFrame = CGRectNull; } return self; @@ -180,10 +178,6 @@ - (void)updateLayoutMetrics:(const LayoutMetrics &)layoutMetrics - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const &)oldProps { - if (!CGRectEqualToRect(_oldFrame, CGRectNull)) { - [self setFrame:_oldFrame]; - _oldFrame = CGRectNull; - } const auto &oldScrollViewProps = *std::static_pointer_cast(_props); const auto &newScrollViewProps = *std::static_pointer_cast(props); @@ -407,8 +401,9 @@ - (void)prepareForRecycle _shouldUpdateContentInsetAdjustmentBehavior = YES; _state.reset(); _isUserTriggeredScrolling = NO; - _oldFrame = self.frame; - [self setFrame:CGRectZero]; + CGRect oldFrame = self.frame; + self.frame = CGRectZero; + self.frame = oldFrame; [super prepareForRecycle]; }