Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IOS][BUGFIX] Fix: Added scroll Bounds check in scrollToOffset method in RCTScrollView on iOS #17927

Closed

Conversation

siddhantsoni
Copy link
Contributor

@siddhantsoni siddhantsoni commented Feb 9, 2018

Motivation

The scrollToOffset method of RCTScrollView for iOS does not include bound check for the scroll offset provided to the method. This can cause the whole view to scroll out of screen if the offset provided is greater than the bounds of the View.
The same does not happen on Android, where the scroll halts once the last item of the scrollView is in the viewport.
I have added bounds check so the offset resets to the MaxOffset which makes sure the view does not scroll out of the viewport.

The issue can be observed in the following snack:
https://snack.expo.io/H1363Uo8f

ezgif com-optimize 1

Test Plan

To test my changes I ran the code provided in the snack above with the react-native dependency pointing to my branch. As can be see in the video attached below, the scroll halts once it hits the end of the ScrollView even if the scroll offset provided is higher than the bounds of the ScrollView. It also does not scroll up for negative scroll offset.

ezgif com-optimize

Release Notes

[IOS] [BUGFIX] [ScrollView] - Added bounds check to prevent ScrollView from scrolling to an offset which is out of bounds of the ScrollView for iOS.

@siddhantsoni siddhantsoni changed the title [i]Added scroll Bounds check in scrollToOffset method in RCTScrollView on iOS [iOS][Scroll Issue]Added scroll Bounds check in scrollToOffset method in RCTScrollView on iOS Feb 9, 2018
@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Something went wrong executing that command, @hramos could you take a look?

@facebook-github-bot facebook-github-bot added Failed Commands CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Feb 9, 2018
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@hramos hramos added Android and removed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Failed Commands labels Feb 9, 2018
@siddhantsoni siddhantsoni changed the title [iOS][Scroll Issue]Added scroll Bounds check in scrollToOffset method in RCTScrollView on iOS [iOS][ScrollView] Fix: Added scroll Bounds check in scrollToOffset method in RCTScrollView on iOS Feb 11, 2018
@siddhantsoni siddhantsoni changed the title [iOS][ScrollView] Fix: Added scroll Bounds check in scrollToOffset method in RCTScrollView on iOS [IOS][ScrollView] Fix: Added scroll Bounds check in scrollToOffset method in RCTScrollView on iOS Feb 11, 2018
@shergin shergin added Platform: iOS iOS applications. and removed Android labels Feb 12, 2018
@@ -577,6 +577,23 @@ - (void)scrollToOffset:(CGPoint)offset
- (void)scrollToOffset:(CGPoint)offset animated:(BOOL)animated
{
if (!CGPointEqualToPoint(_scrollView.contentOffset, offset)) {
BOOL isHorizontal = [self isHorizontal:_scrollView];
Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow existing code style guides.

@@ -577,6 +577,23 @@ - (void)scrollToOffset:(CGPoint)offset
- (void)scrollToOffset:(CGPoint)offset animated:(BOOL)animated
{
if (!CGPointEqualToPoint(_scrollView.contentOffset, offset)) {
BOOL isHorizontal = [self isHorizontal:_scrollView];
if (isHorizontal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should we have this condition?
Seems we just have to have one single expression which ensure that offset inside allowed range.

Copy link
Contributor Author

@siddhantsoni siddhantsoni Feb 12, 2018

Choose a reason for hiding this comment

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

@shergin Updated PR to handle offset range in single expression.

@siddhantsoni
Copy link
Contributor Author

@shergin Can you help with one of the tests that is failing. I am not sure what kind of correction the test expects. The failing test is: analyze_pr

@siddhantsoni siddhantsoni changed the title [IOS][ScrollView] Fix: Added scroll Bounds check in scrollToOffset method in RCTScrollView on iOS [IOS][BUGFIX] Fix: Added scroll Bounds check in scrollToOffset method in RCTScrollView on iOS Feb 16, 2018
@hramos
Copy link
Contributor

hramos commented Feb 16, 2018

The anaylze_pr failure seems unrelated to this PR.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 16, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@naqvitalha
Copy link

@shergin @hramos Is this getting pulled in anytime soon? Not sure about your labels.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@shergin is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot pushed a commit that referenced this pull request Mar 14, 2018
Summary:
This change is a revert of [#17927](#17927) pull-request.
The pull-request caused an issue with the keyboard covering the text field at the bottom of the scroll view.

Reviewed By: shergin

Differential Revision: D7246609

fbshipit-source-id: 149f825274c4fa79ab593f1bae3602667d16ddee
campsafari pushed a commit to exozet/react-native that referenced this pull request Apr 11, 2018
Summary:
This change is a revert of [facebook#17927](facebook#17927) pull-request.
The pull-request caused an issue with the keyboard covering the text field at the bottom of the scroll view.

Reviewed By: shergin

Differential Revision: D7246609

fbshipit-source-id: 149f825274c4fa79ab593f1bae3602667d16ddee
@naqvitalha
Copy link

@shergin Why was this reverted? This leads to inconsistent behaviour between iOS and Android. This might be a breaking change but one that should be made eventually.

@hramos
Copy link
Contributor

hramos commented Jun 25, 2018

@naqvitalha this is covered in the revert commit at 8466db0#diff-2f26f4ef865c6f1f1fb4485aa7fdb3a9: "The pull-request caused an issue with the keyboard covering the text field at the bottom of the scroll view."

macdoum1 pushed a commit to macdoum1/react-native that referenced this pull request Jun 28, 2018
Summary:
This change is a revert of [facebook#17927](facebook#17927) pull-request.
The pull-request caused an issue with the keyboard covering the text field at the bottom of the scroll view.

Reviewed By: shergin

Differential Revision: D7246609

fbshipit-source-id: 149f825274c4fa79ab593f1bae3602667d16ddee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Import Started This pull request has been imported. This does not imply the PR has been approved. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants