Skip to content

Commit

Permalink
Fix scrollview over bounds of content size (#23427)
Browse files Browse the repository at this point in the history 
Summary:
Fix scrollview `offset` out of content size in iOS, Android uses `scrollTo` and `smoothScrollTo` which not have this issue.

Fixes like #13594 #22768 #19970 .

[iOS] [Fixed] - Fixed scrollView offset out of content size.
Pull Request resolved: #23427

Differential Revision: D14162663

Pulled By: cpojer

fbshipit-source-id: a95371c8d703b6d5f604af0072f86c01c2018f4a
  • Loading branch information
@zhongwuzw @grabbou
zhongwuzw authored and grabbou committed Mar 22, 2019
1 parent 699fad7 commit f6516d2
Showing 1 changed file with 11 additions and 0 deletions.
11 changes: 11 additions & 0 deletions React/Views/ScrollView/RCTScrollView.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -588,8 +588,19 @@ - (void)scrollToOffset:(CGPoint)offset
- (void)scrollToOffset:(CGPoint)offset animated:(BOOL)animated
{
if (!CGPointEqualToPoint(_scrollView.contentOffset, offset)) {
CGRect maxRect = CGRectMake(fmin(-_scrollView.contentInset.left, 0),
fmin(-_scrollView.contentInset.top, 0),
fmax(_scrollView.contentSize.width - _scrollView.bounds.size.width + _scrollView.contentInset.right + fmax(_scrollView.contentInset.left, 0), 0.01),
fmax(_scrollView.contentSize.height - _scrollView.bounds.size.height + _scrollView.contentInset.bottom + fmax(_scrollView.contentInset.top, 0), 0.01)); // Make width and height greater than 0
// Ensure at least one scroll event will fire
_allowNextScrollNoMatterWhat = YES;
if (!CGRectContainsPoint(maxRect, offset)) {
CGFloat x = fmax(offset.x, CGRectGetMinX(maxRect));
x = fmin(x, CGRectGetMaxX(maxRect));
CGFloat y = fmax(offset.y, CGRectGetMinY(maxRect));
y = fmin(y, CGRectGetMaxY(maxRect));
offset = CGPointMake(x, y);
}
[_scrollView setContentOffset:offset animated:animated];
}
}
Expand Down

9 comments on commit f6516d2

@mysport12
Copy link
Contributor

@mysport12 mysport12 commented on f6516d2 Mar 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance this behavior can be made optional (with an opt-in/opt-out prop depending upon the default)? In our apps, we use the over scroll ability to move content out of the way of the virtual keyboard. Not sure if we are the exception here but it may impact other projects that were relying on this ability. @zhongwuzw

@zhongwuzw
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use contentInset to adjust scroll area?

@mysport12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do that then the user can scroll all the way down to it. We only want to permit the over scroll programmatically when the onFocus event is fired from a TextInput on the screen.

@mysport12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see a way of doing it with contentInset assuming a new state variable is created in my components and keyboard event listeners are used but I was hoping to not go down that road if possible. I can make an attempt at a new prop to opt out of this behavior and create a PR proposing the change. My native skills are shaky at best but I'll give it a go.

@fenghengzhi
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's better to add a new boolean param (false by default) to scrollTo method than just change it's old behavior

@mysport12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new Boolean param was added that allows for opting back in to the old behavior

@jjhampton
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mysport12 Do you know where the new Boolean param is at, and where it can be set, which allows for opting back in to the old behavior? The app I'm working on is experiencing some unintended side-effects which appear to be caused by this change.

@jjhampton
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mysport12 ^ Disregard the above question, I see it in RN 0.59.4: e3ac329 .

@mysport12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjhampton To opt back in to the old behavior you can pass a prop to your ScrollView.

scrollToOverflowEnabled={true}

Or set it globally like we do in our apps. In App.js set the defaultProps outside of your component with something like this...

ScrollView.defaultProps = {
...ScrollView.defaultProps,
scrollToOverflowEnabled: true
}

Hope that helps!

Please sign in to comment.