From fe7e97a2fd272db0d9d9aa7d0561337a7c8e2c30 Mon Sep 17 00:00:00 2001 From: dan Date: Mon, 25 Nov 2024 14:56:02 -0800 Subject: [PATCH] Fix ScrollView centerContent losing taps and causing jitter on iOS (#47591) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: The React Native `` has a peculiar `centerContent` prop. It solves a common need — keeping the image "centered" while it's not fully zoomed in (but then allowing full panning after it's sufficiently zoomed in). This prop sort of works but it has a few wonky behaviors: - If you start tapping immediately after pinch (and don't stop), the taps will not be recognized until a second after you stop tapping. I suspect this is because the existing `centerContent` implementation hijacks the `contentOffset` setter, but the calling UIKit code _does not know_ it's being hijacked, and so the calling UIKit code _thinks_ it needs to do a momentum animation. This (invisible) momentum animation causes the scroll view to keep eating the tap touches. - While you're zooming in, once you cross the threshold where `contentOffset` hijacking stops adjusting values, there will be a sudden visual jump during the pinch. This is because the "real" `contentOffset` tracks the accumulated translation from the pinch gesture, and once it gets taken into account with no "correction", the new offset snaps into place. - While not sufficiently pinched in, the vertical axis is completely rigid. It does not have the natural rubber banding. The solution to all of these issues is described [here](https://petersteinberger.com/blog/2013/how-to-center-uiscrollview/). Instead of hijacking `contentOffset`, it is more reliable to track zooming, child view, and frame changes, and adjust `contentInsets` instead. This solves all three issues: - UIKit isn't confused by the content offset changing from under it so it doesn't mistrigger a faux momentum animation. - There is no sudden jump because it's the insets that are being adjusted. - Rubber banding just works. ## Changelog: [IOS] [FIXED] - Fixed centerContent losing taps and causing jitter Pull Request resolved: https://github.com/facebook/react-native/pull/47591 Test Plan: I'm extracting this from [a patch we're applying to Bluesky](https://github.com/bluesky-social/social-app/blob/2ef697fe3d7dec198544ed6834553f33b95790b3/patches/react-native%2B0.74.1.patch). I'll be honest — I have not tested this in isolation, and it likely requires some testing to get merged in. I do not, unfortuntately, have the capacity to do it myself so this is more of a "throw over the wall" kind of patch. Maybe it will be helpful to somebody else. I've tested these in our real open source app (https://github.com/bluesky-social/social-app/pull/6298). You can reproduce it in any of the lightboxes in the feed or the profile. ### Before the fix Observe the failing tap gestures, sudden jump while pinching, lack of rubber banding. https://github.com/user-attachments/assets/c9883201-c9f0-4782-9b80-8e0a9f77c47c ### After the fix Observe the natural iOS behavior. https://github.com/user-attachments/assets/c025e1df-6963-40ba-9e28-d48bfa5e631d Unfortunately I do not have the capacity to verify this fix in other scenarios outside of our app. Reviewed By: sammy-SC Differential Revision: D66093472 Pulled By: javache fbshipit-source-id: 064f0415b8093ff55cb51bdebab2a46ee97f8fa9 --- .../ScrollView/RCTEnhancedScrollView.mm | 51 +++++++++++---- .../React/Views/ScrollView/RCTScrollView.m | 65 +++++++++++++------ 2 files changed, 86 insertions(+), 30 deletions(-) diff --git a/packages/react-native/React/Fabric/Mounting/ComponentViews/ScrollView/RCTEnhancedScrollView.mm b/packages/react-native/React/Fabric/Mounting/ComponentViews/ScrollView/RCTEnhancedScrollView.mm index 2d884a73641..c593d9ee215 100644 --- a/packages/react-native/React/Fabric/Mounting/ComponentViews/ScrollView/RCTEnhancedScrollView.mm +++ b/packages/react-native/React/Fabric/Mounting/ComponentViews/ScrollView/RCTEnhancedScrollView.mm @@ -67,28 +67,52 @@ * ScrollView, we force it to be centered, but when you zoom or the content otherwise * becomes larger than the ScrollView, there is no padding around the content but it * can still fill the whole view. + * This implementation is based on https://petersteinberger.com/blog/2013/how-to-center-uiscrollview/. */ +- (void)centerContentIfNeeded +{ + if (!_centerContent) { + return; + } + + CGSize contentSize = self.contentSize; + CGSize boundsSize = self.bounds.size; + if (CGSizeEqualToSize(contentSize, CGSizeZero) || CGSizeEqualToSize(boundsSize, CGSizeZero)) { + return; + } + + CGFloat top = 0, left = 0; + if (contentSize.width < boundsSize.width) { + left = (boundsSize.width - contentSize.width) * 0.5f; + } + if (contentSize.height < boundsSize.height) { + top = (boundsSize.height - contentSize.height) * 0.5f; + } + self.contentInset = UIEdgeInsetsMake(top, left, top, left); +} + - (void)setContentOffset:(CGPoint)contentOffset { if (_isSetContentOffsetDisabled) { return; } - - if (_centerContent && !CGSizeEqualToSize(self.contentSize, CGSizeZero)) { - CGSize scrollViewSize = self.bounds.size; - if (self.contentSize.width <= scrollViewSize.width) { - contentOffset.x = -(scrollViewSize.width - self.contentSize.width) / 2.0; - } - if (self.contentSize.height <= scrollViewSize.height) { - contentOffset.y = -(scrollViewSize.height - self.contentSize.height) / 2.0; - } - } - super.contentOffset = CGPointMake( RCTSanitizeNaNValue(contentOffset.x, @"scrollView.contentOffset.x"), RCTSanitizeNaNValue(contentOffset.y, @"scrollView.contentOffset.y")); } +- (void)setFrame:(CGRect)frame +{ + [super setFrame:frame]; + [self centerContentIfNeeded]; +} + +- (void)didAddSubview:(UIView *)subview +{ + [super didAddSubview:subview]; + [self centerContentIfNeeded]; +} + - (BOOL)touchesShouldCancelInContentView:(UIView *)view { if ([_overridingDelegate respondsToSelector:@selector(touchesShouldCancelInContentView:)]) { @@ -258,6 +282,11 @@ } } +- (void)scrollViewDidZoom:(__unused UIScrollView *)scrollView +{ + [self centerContentIfNeeded]; +} + #pragma mark - - (BOOL)isHorizontal:(UIScrollView *)scrollView diff --git a/packages/react-native/React/Views/ScrollView/RCTScrollView.m b/packages/react-native/React/Views/ScrollView/RCTScrollView.m index e83b91de675..59f956d893e 100644 --- a/packages/react-native/React/Views/ScrollView/RCTScrollView.m +++ b/packages/react-native/React/Views/ScrollView/RCTScrollView.m @@ -159,26 +159,8 @@ return !shouldDisableScrollInteraction; } -/* - * Automatically centers the content such that if the content is smaller than the - * ScrollView, we force it to be centered, but when you zoom or the content otherwise - * becomes larger than the ScrollView, there is no padding around the content but it - * can still fill the whole view. - */ - (void)setContentOffset:(CGPoint)contentOffset { - UIView *contentView = [self contentView]; - if (contentView && _centerContent && !CGSizeEqualToSize(contentView.frame.size, CGSizeZero)) { - CGSize subviewSize = contentView.frame.size; - CGSize scrollViewSize = self.bounds.size; - if (subviewSize.width <= scrollViewSize.width) { - contentOffset.x = -(scrollViewSize.width - subviewSize.width) / 2.0; - } - if (subviewSize.height <= scrollViewSize.height) { - contentOffset.y = -(scrollViewSize.height - subviewSize.height) / 2.0; - } - } - super.contentOffset = CGPointMake( RCTSanitizeNaNValue(contentOffset.x, @"scrollView.contentOffset.x"), RCTSanitizeNaNValue(contentOffset.y, @"scrollView.contentOffset.y")); @@ -433,6 +415,12 @@ static inline void RCTApplyTransformationAccordingLayoutDirection( // Does nothing } +- (void)setFrame:(CGRect)frame +{ + [super setFrame:frame]; + [self centerContentIfNeeded]; +} + - (void)insertReactSubview:(UIView *)view atIndex:(NSInteger)atIndex { [super insertReactSubview:view atIndex:atIndex]; @@ -450,6 +438,8 @@ static inline void RCTApplyTransformationAccordingLayoutDirection( RCTApplyTransformationAccordingLayoutDirection(_contentView, self.reactLayoutDirection); [_scrollView addSubview:view]; } + + [self centerContentIfNeeded]; } - (void)removeReactSubview:(UIView *)subview @@ -658,9 +648,45 @@ static inline void RCTApplyTransformationAccordingLayoutDirection( } RCT_SCROLL_EVENT_HANDLER(scrollViewWillBeginDecelerating, onMomentumScrollBegin) -RCT_SCROLL_EVENT_HANDLER(scrollViewDidZoom, onScroll) RCT_SCROLL_EVENT_HANDLER(scrollViewDidScrollToTop, onScrollToTop) +- (void)scrollViewDidZoom:(UIScrollView *)scrollView +{ + [self centerContentIfNeeded]; + + RCT_SEND_SCROLL_EVENT(onScroll, nil); + RCT_FORWARD_SCROLL_EVENT(scrollViewDidZoom : scrollView); +} + +/* + * Automatically centers the content such that if the content is smaller than the + * ScrollView, we force it to be centered, but when you zoom or the content otherwise + * becomes larger than the ScrollView, there is no padding around the content but it + * can still fill the whole view. + * This implementation is based on https://petersteinberger.com/blog/2013/how-to-center-uiscrollview/. + */ +- (void)centerContentIfNeeded +{ + if (!_scrollView.centerContent) { + return; + } + + CGSize contentSize = self.contentSize; + CGSize boundsSize = self.bounds.size; + if (CGSizeEqualToSize(contentSize, CGSizeZero) || CGSizeEqualToSize(boundsSize, CGSizeZero)) { + return; + } + + CGFloat top = 0, left = 0; + if (contentSize.width < boundsSize.width) { + left = (boundsSize.width - contentSize.width) * 0.5f; + } + if (contentSize.height < boundsSize.height) { + top = (boundsSize.height - contentSize.height) * 0.5f; + } + _scrollView.contentInset = UIEdgeInsetsMake(top, left, top, left); +} + - (void)addScrollListener:(NSObject *)scrollListener { [_scrollListeners addObject:scrollListener]; @@ -939,6 +965,7 @@ RCT_SCROLL_EVENT_HANDLER(scrollViewDidScrollToTop, onScrollToTop) CGSize contentSize = self.contentSize; if (!CGSizeEqualToSize(_scrollView.contentSize, contentSize)) { _scrollView.contentSize = contentSize; + [self centerContentIfNeeded]; } }