[IOS][BUGFIX] Fix: Added scroll Bounds check in scrollToOffset method in RCTScrollView on iOS #17927
Conversation
siddhantsoni
changed the title
|
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! |
|
Something went wrong executing that command, @hramos could you take a look? |
facebook-github-bot
added
Failed Commands
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
hramos
added
Android
and removed
CLA Signed
siddhantsoni
changed the title
siddhantsoni
changed the title
| @@ -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]; | |||
There was a problem hiding this comment.
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) { | |||
There was a problem hiding this comment.
Why should we have this condition?
Seems we just have to have one single expression which ensure that offset inside allowed range.
There was a problem hiding this comment.
@shergin Updated PR to handle offset range in single expression.
|
@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
changed the title
|
The anaylze_pr failure seems unrelated to this PR. |
facebook-github-bot
added
the
Import Started
There was a problem hiding this comment.
@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@shergin is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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
|
@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. |
|
@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." |
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
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
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.
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.