Skip to content

Commit

Permalink
fix: incorrect ScrollView offset on update (#30647)
Browse files Browse the repository at this point in the history
Summary:
This pull request is to fix #30258.

After an investigation, I found out the scroll offset of the list seems to be calculated incorrectly due to a workaround in the source code.

Instead of fixing the `calculateOffsetForContentSize`, I chose to remove it, the reason why I do so is because this workaround is for fixing the offset when `contentSize` is set manually, but according to the source code, there is no interface for a react-native user to set the `contentSize` of ScrollView, it is just set to `GCSizeZero` and will never be changed ([ref](https://github.com/facebook/react-native/pull/30647/files#diff-cf6f991f585ebf4cfdd555fe474e1f9ce40c2e4f823fc3f42b549414639c8c30L304)).

Also I changed the function name from `updateContentOffsetIfNeeded` to  `updateContentSizeIfNeeded` according what the function is actually doing.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - Incorrect ScrollView offset on update

Pull Request resolved: #30647

Test Plan:
1. Create a fresh new project named `testapp` using npx create-react-native-app
2. Apply example code to `testapp` from https://snack.expo.io/lokomass/flatlist
3. Run `testapp` on iOS emulator and reproduce the bug
4. Make changes to files in `testapp/node_modules/react-native/`
5. Rebuild `testapp` and run on iOS emulator again, the bug is no more exist
6. Apply changes from step 4 to react-native, make a pull request.

#### Screenshots

Before: The scroll offset is incorrect after children of FlatList has changed

https://user-images.githubusercontent.com/48589760/103155130-b54a0580-47d7-11eb-97af-bdfd3e728714.mov

After: No more incorrect scroll offset if children of FlatList has changed

https://user-images.githubusercontent.com/48589760/103155091-6ef4a680-47d7-11eb-89fa-6f708bfef1c9.mov

Reviewed By: sammy-SC

Differential Revision: D25732958

Pulled By: shergin

fbshipit-source-id: dac6eff15ac3bbfec502452ac14b3d49fee76c29
  • Loading branch information
rnike authored and facebook-github-bot committed Jan 5, 2021
1 parent 0db56f1 commit a4526bc
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 68 deletions.
2 changes: 1 addition & 1 deletion React/Views/ScrollView/RCTScrollContentView.m
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ - (void)reactSetFrame:(CGRect)frame

RCTAssert([scrollView isKindOfClass:[RCTScrollView class]], @"Unexpected view hierarchy of RCTScrollView component.");

[scrollView updateContentOffsetIfNeeded];
[scrollView updateContentSizeIfNeeded];
}

@end
8 changes: 1 addition & 7 deletions React/Views/ScrollView/RCTScrollView.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,6 @@
*/
@property (nonatomic, readonly) UIView *contentView;

/**
* If the `contentSize` is not specified (or is specified as {0, 0}, then the
* `contentSize` will automatically be determined by the size of the subview.
*/
@property (nonatomic, assign) CGSize contentSize;

/**
* The underlying scrollView (TODO: can we remove this?)
*/
Expand Down Expand Up @@ -68,7 +62,7 @@

@interface RCTScrollView (Internal)

- (void)updateContentOffsetIfNeeded;
- (void)updateContentSizeIfNeeded;

@end

Expand Down
62 changes: 2 additions & 60 deletions React/Views/ScrollView/RCTScrollView.m
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,6 @@ - (instancetype)initWithEventDispatcher:(id<RCTEventDispatcherProtocol>)eventDis

_automaticallyAdjustContentInsets = YES;
_contentInset = UIEdgeInsetsZero;
_contentSize = CGSizeZero;
_lastClippedToRect = CGRectNull;

_scrollEventThrottle = 0.0;
Expand Down Expand Up @@ -381,7 +380,7 @@ - (void)didUpdateReactSubviews
- (void)didSetProps:(NSArray<NSString *> *)changedProps
{
if ([changedProps containsObject:@"contentSize"]) {
[self updateContentOffsetIfNeeded];
[self updateContentSizeIfNeeded];
}
}

Expand Down Expand Up @@ -799,8 +798,6 @@ - (UIView *)viewForZoomingInScrollView:(__unused UIScrollView *)scrollView
return _contentView;
}

#pragma mark - Setters

- (CGSize)_calculateViewportSize
{
CGSize viewportSize = self.bounds.size;
Expand All @@ -813,71 +810,16 @@ - (CGSize)_calculateViewportSize
return viewportSize;
}

- (CGPoint)calculateOffsetForContentSize:(CGSize)newContentSize
{
CGPoint oldOffset = _scrollView.contentOffset;
CGPoint newOffset = oldOffset;

CGSize oldContentSize = _scrollView.contentSize;
CGSize viewportSize = [self _calculateViewportSize];

BOOL fitsinViewportY = oldContentSize.height <= viewportSize.height && newContentSize.height <= viewportSize.height;
if (newContentSize.height < oldContentSize.height && !fitsinViewportY) {
CGFloat offsetHeight = oldOffset.y + viewportSize.height;
if (oldOffset.y < 0) {
// overscrolled on top, leave offset alone
} else if (offsetHeight > oldContentSize.height) {
// overscrolled on the bottom, preserve overscroll amount
newOffset.y = MAX(0, oldOffset.y - (oldContentSize.height - newContentSize.height));
} else if (offsetHeight > newContentSize.height) {
// offset falls outside of bounds, scroll back to end of list
newOffset.y = MAX(0, newContentSize.height - viewportSize.height);
}
}

BOOL fitsinViewportX = oldContentSize.width <= viewportSize.width && newContentSize.width <= viewportSize.width;
if (newContentSize.width < oldContentSize.width && !fitsinViewportX) {
CGFloat offsetHeight = oldOffset.x + viewportSize.width;
if (oldOffset.x < 0) {
// overscrolled at the beginning, leave offset alone
} else if (offsetHeight > oldContentSize.width && newContentSize.width > viewportSize.width) {
// overscrolled at the end, preserve overscroll amount as much as possible
newOffset.x = MAX(0, oldOffset.x - (oldContentSize.width - newContentSize.width));
} else if (offsetHeight > newContentSize.width) {
// offset falls outside of bounds, scroll back to end
newOffset.x = MAX(0, newContentSize.width - viewportSize.width);
}
}

// all other cases, offset doesn't change
return newOffset;
}

/**
* Once you set the `contentSize`, to a nonzero value, it is assumed to be
* managed by you, and we'll never automatically compute the size for you,
* unless you manually reset it back to {0, 0}
*/
- (CGSize)contentSize
{
if (!CGSizeEqualToSize(_contentSize, CGSizeZero)) {
return _contentSize;
}

return _contentView.frame.size;
}

- (void)updateContentOffsetIfNeeded
- (void)updateContentSizeIfNeeded
{
CGSize contentSize = self.contentSize;
if (!CGSizeEqualToSize(_scrollView.contentSize, contentSize)) {
// When contentSize is set manually, ScrollView internals will reset
// contentOffset to {0, 0}. Since we potentially set contentSize whenever
// anything in the ScrollView updates, we workaround this issue by manually
// adjusting contentOffset whenever this happens
CGPoint newOffset = [self calculateOffsetForContentSize:contentSize];
_scrollView.contentSize = contentSize;
_scrollView.contentOffset = newOffset;
}
}

Expand Down

0 comments on commit a4526bc

Please sign in to comment.