From 248a108abf206b7ae32208537f0b73a8192a4829 Mon Sep 17 00:00:00 2001
From: Vojtech Novak <vonovak@gmail.com>
Date: Tue, 14 May 2019 06:20:07 -0700
Subject: [PATCH] fix scrollToLocation not consistent between platforms
 (#24734)

Summary:
first, I edited this post so that 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: https://github.com/facebook/react-native/pull/21577 and https://github.com/facebook/react-native/pull/24034

let me just quote the [docs](https://facebook.github.io/react-native/docs/sectionlist#scrolltolocation) here

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

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

so if we call

```js
    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.

next, there was https://github.com/facebook/react-native/pull/21577 that changed it to

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

so that https://github.com/facebook/react-native/issues/18098 is fixed. The PR had a bug and so next, there was https://github.com/facebook/react-native/pull/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):

So, this PR works under the assumption that

```js
    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 https://github.com/facebook/react-native/issues/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

[Android][Fixed] - fix scrollToLocation not consistent between platforms
Pull Request resolved: https://github.com/facebook/react-native/pull/24734

Differential Revision: D15334565

Pulled By: cpojer

fbshipit-source-id: f7122ef4a5c252fc3e0d2112861d1460742e9fa4
---
 Libraries/Lists/VirtualizedSectionList.js | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Libraries/Lists/VirtualizedSectionList.js b/Libraries/Lists/VirtualizedSectionList.js
index 042eca2c5d7304..ebd12dfad57dc7 100644
--- a/Libraries/Lists/VirtualizedSectionList.js
+++ b/Libraries/Lists/VirtualizedSectionList.js
@@ -9,7 +9,6 @@
  */
 'use strict';
 
-const Platform = require('../Utilities/Platform');
 const React = require('react');
 const View = require('../Components/View/View');
 const VirtualizedList = require('./VirtualizedList');
@@ -145,7 +144,7 @@ class VirtualizedSectionList<
     sectionIndex: number,
     viewPosition?: number,
   }) {
-    let index = Platform.OS === 'ios' ? params.itemIndex : params.itemIndex + 1;
+    let index = params.itemIndex;
     for (let i = 0; i < params.sectionIndex; i++) {
       index += this.props.getItemCount(this.props.sections[i].data) + 2;
     }