fix #10747 (ScrollView rendered incorrectly with RefreshControl)

[vonovak]

set the y offset to 0, since 0 offset is where we want to be after we hide the refreshControl.

Tested in emulator with ios 8, 9, 10 and also with section headers.

[shergin]

Why does it work?

[vonovak]

@shergin I'll try to explain with a couple of screenshots:

lightblue is refreshControl, green is listView

1 . note I hardcoded the -120 points offset and commented out the [super endRefreshing]. This screenshot is stopped at a breakpoint.

2 . The same code after the method finishes. One can see the 60 points of RefreshControl (lightblue), and the next 60 points of ListView (green) from the 120 points of offset

3 . This is how the current master behaves when the [super endRefreshing] is commented out.

4 . this is the current behavior of master, the call to [super endRefreshing] does two things (compare with the previous screenshot):

• hides the activity indicator
• moves the scrollView up BUT this doesn't happen if the scrollView content was moved in the meantime (ScrollView rendered incorrectly with RefreshControl if push back a bit before finish refreshing #10747) - this is the weird part. In either case, setting the y offset to -60 is not right, it should be 0 because the docs for contentOffset say:

contentOffset:
The point at which the origin of the content view is offset from the origin of the scroll view.

5 . when 0 is used for the y offset and [super endRefreshing] is commented out, the scrollView correctly goes all the way up. This is essentially the fix, but of course the [super endRefreshing] should still be called.

[vonovak]

ping @shergin does this look good enough?

[vonovak]

Ping @shergin @janicduplessis does this look good?

[shergin]

I am sorry, I have not chance to look at this yet. 😞
@janicduplessis Does it look good for you?

[janicduplessis]

I think this is fine, makes sense that we should set y content offset to 0.

[shergin]

Wait a second, we just found in another diff that initial contentOffset can be negative and actually directly related to contentInsets!
Right? So, why should we scroll to 0?

[shergin]

@vonovak I am really sorry, but I cannot accept your explanation. 😢

[vonovak]

@shergin can you point me to such diff?

[shergin]

See discussion here:
https://github.com/facebook/react-native/pull/15395/files/44a5ec87d1c06f4fb52bb5602f62b9015f76a786#diff-973dcd0e3fbfcc5e35485bb6404a2fd2

And follow up diff:
950c2b2

And great article about ScrollView: https://www.objc.io/issues/3-views/scroll-view/

I am not sure, honestly. May be I am wrong here.
¯\_(ツ)_/¯

[facebook-github-bot]

@vonovak I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[ngandhy]

@shergin / @janicduplessis - Why would it not "end refreshing" on 0? The if statement above the change and comment above that both imply that at the end of refreshing it should no longer be negative. What other valid values are allowed and if so should they just be temporarily stored in beginRefreshing and replaced in endRefreshing?

"-scrollView.contentInset.top" doesn't seem to make sense for end offset value... as shown by the bugs it's creating.

@vonovak - thanks for repeatedly trying to fix this issue. It's appreciated

[facebook-github-bot]

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