fix scrollToLocation not consistent between platforms

[vonovak]

Summary

first, I edited this post so the comment below are NOT relevant any more

problem: scrollToLocation behavior in SectionList on master is inconsistent between platforms.

Recently, there have been two PRs changing this function: #21577 and #24034

let me just quote the docs here

Scrolls to the item at the specified sectionIndex and itemIndex (within the section)

old behavior

originally, the code was let index = params.itemIndex + 1;

so if we call

    node.scrollToLocation({
      animated: false,
      itemIndex: 0,
      sectionIndex: 0,
      viewPosition: 0,
      viewOffset: 0,
    });

inside scrollToLocation, index is incremented and that results into the list scrolling to the first item (the first section heading is not visible - see the gif). If I specify itemIndex = -1, it will scroll all the way up. I was expecting itemIndex = 0 to bring me all the way up, so that the first section heading is visible. Not sure which way this should work but at least this behavior is in line with the docs.

new behavior

next, there was #21577 that changed it to

let index = Platform.OS === 'ios' ? params.itemIndex: params.itemIndex - 1;

so that #18098 is fixed. The PR had a bug and so next, there was #24034 that changed it to

let index = Platform.OS === 'ios' ? params.itemIndex : params.itemIndex + 1;

the reasoning was

Note however, how the sign for non iOS changed from + to - causing a crash on Android when trying to scroll to index 0 as that will be evaluated to -1 instead of 1 and thus be out of bounds.

with this change, the list does not crash. However, the behavior is not consistent between platforms (see gifs):

this PR

So, this PR works under the assumption that

    node.scrollToLocation({
      animated: false,
      itemIndex: 0,
      sectionIndex: 0,
      viewPosition: 0,
      viewOffset: 0,
    });

should scroll all the way up, so that even the first section heading is visible. This should keep this bug #18098 fixed. However, this change means different behavior from how it used to be and from how it is documented, which should be pointed out in release notes and docs need to be updated.

Finally, if we agree this is the way to go, it'd be nice to have some automated tests.

cc @melina7890 @danilobuerger

Changelog

[Android][Fixed] - fix scrollToLocation not consistent between platforms

Test plan

I used https://gist.github.com/vonovak/d85a95ac6bb189a8b5d1ef61c58e0406 (just copy-paste it to RNTesterApp.android.js/ios.js in the RNTester app)

gifs of master branch behavior:

gifs of this branch behavior (both behave the same, so just one gif)

[danilobuerger]

However, it seems that the author failed to notice (?) that the list does not scroll to the very top, but to the second item in the list.

The intention of PR #21577 was, as far as I can tell, to change the behaviour for iOS, not modify it for Android. That is why my PR (#24034) reverted the behaviour on non-iOS to what it was before. I considered it to be out of scope for my PR to judge wether the previous behaviour was indeed correct or not. It was about reverting a obvious mistake (+ 1 vs - 1).

[danilobuerger]

Just tested, I can confirm that this works as advertised.

However, unrelated to this specific change, I noticed during testing that when having stickySectionHeadersEnabled = true, the scrollToLocation will not account for the sticky header. I will open another PR with a bugfix for that.

[vonovak]

@danilobuerger thanks for a fast response

Just tested, I can confirm that this works as advertised.

just to be sure - you're talking about this PR fixing the issue of scrolling to top on android?

I noticed during testing that when having stickySectionHeadersEnabled = true, the scrollToLocation will not account for the sticky header

I think this is the intended behavior, if you take a look at https://facebook.github.io/react-native/docs/sectionlist#scrolltolocation the docs says

'viewOffset' (number) - A fixed number of pixels to offset the final target position, e.g. to compensate for sticky headers.

[danilobuerger]

just to be sure - you're talking about this PR fixing the issue of scrolling to top on android?

Yes.

I think this is the intended behavior

However, viewOffset is not listed in the typing of scrollToLocation. Also, why do it manually when it can be accounted for automatically. But it was just a sidenote, I am going to open a PR for it, no point of discussing it here.

[danilobuerger]

@vonovak you should edit the description based on what @pull-bot suggested so that someone can actually review and merge it.

[vonovak]

the PR is ready, I rewrote the description so please keep in mind the above comments are not relevant any more

cc @cpojer since you merged the two PRs that changed this function recently

[cpojer]

Alright, let's revert this then. It seems we are going back and forth on this and people are confused so if you could send a PR to update the docs on the website that would be good.

[facebook-github-bot]

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

[react-native-bot]

This pull request was successfully merged by @vonovak in 248a108.

^{When will my fix make it into a release? | Upcoming Releases}