Fix: issue #7001

[godness84]

It fixes the issue #7001

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @kmagiera, @andreicoman11 and @dmmiller to be potential reviewers.

[ghost]

@godness84 updated the pull request.

[ghost]

@kmagiera would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

[mkonicek]

@godness84 I'm not sure I understand the issue well - could you show a GIF of how it's broken?

Sorry for the long delay!

[mkonicek]

If you use a normal <ScrollView> instead does it work?

[godness84]

Yes, definitely. Using a normal <ScrollView> it works.
Try this piece of code:

import React, { Component } from 'react';
import {
  AppRegistry,  
  StyleSheet,
  Text,
  View,
  ListView,
  ScrollView,
  RecyclerViewBackedScrollView,
  ViewPagerAndroid,
} from 'react-native';

class Issue7001 extends Component {
  constructor(props){
    super(props);

    var ds = new ListView.DataSource({rowHasChanged: (r1, r2) => r1 !== r2});
    var rows = [];
    for (var i=0; i<100; i++){
      rows.push(i);
    }

    this.state = {
      dataSource: ds.cloneWithRows(rows),
    };
  }

  renderItem(item){
      return <Text style={{color: '#000000', fontSize: 20, flex: 1, textAlign: 'center'}}>{item}</Text>;
  }

  render() {
    return (
      <View style={styles.container}>
        <ViewPagerAndroid
          initialPage={0}
          style={{flex: 1}}
          >
          <View>
            <ListView
              dataSource={this.state.dataSource}
              renderRow={this.renderItem.bind(this)}
              style={styles.listView}
              pageSize={1}
              initialListSize={10}
              renderScrollComponent={props => {
                //return <ScrollView {...props} />; uncommenting this line it will work as expected
                return <RecyclerViewBackedScrollView {...props} />;
              }}
            />
          </View>
        </ViewPagerAndroid>
      </View>
    );
  }
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
  },
  listView: {
    position: 'absolute',
    top: 0,
    left: 0,
    right: 0,
    bottom: 0,
  },
});

AppRegistry.registerComponent('Issue7001', () => Issue7001);

[andreicoman11]

Neither the ScrollView, nor the HorizontalScrollView have this issue, but neither of them override onAttachedToWindow in the way it's done here. That makes me think that the problem is coming from somewhere else and this is not the correct fix. Do you mind explaining how you got to this fix?

[godness84]

In ReactNative the items are added to the scrollview while scrolling, Just few initial items are added to the scrollview at startup (it depends on the pagesize). When the RecycleView is scrolled, new items are added. These items are added to the adapter and the RecycleView is informed that it should refresh its content. The RecycleView enqueues these updates and performs them if the layout pass has been happened ('mFirstLayoutComplete' stores this information) otherwise it will wait for the layout pass to happen.

ReactNative uses its own layout system and bypass the normal one, it layouts the views before adding them to the view hierarchy. The RecycleView is expecting to be added to the hierarchy and that a layout pass will occur. RecycleView overrides the 'onAttachedToWindow' method and set the value of mFirstLayoutComplete to false: it's expecting that the layout pass will happen soon or later, but it won't, because ReactNative uses its own system.

Indeed, if you rotate the device a new layout pass is triggered, and the RecycleView will show the new items added by React, and it will always show new items added during scrolling, because mFirstLayoutComplete will remain true.

My fix is simulating a new layout pass calling the onLayout method, preserving the values of the layout already calculated by React.

[godness84]

any news?

[lacker]

I feel like you have explained why adding an extra onLayout call fixes this particular issue, but this does not explain why it is correct to call onLayout here in general. onLayout is supposed to be invoked on mount and layout changes. Either onAttachedToWindow is considered a mount and layout change, or it is not. So it does not seem like the behavior of ScrollView and RecyclerViewBackedScrollView, in terms of when they fire onLayout, should be different here.

[hramos]

@godness84 still planning to ship this?

[godness84]

@hramos if the other option is to leave the issue unfixed, yes, I would like to see it shipped.

When I wrote this fix, I already knew that it would have seemed a hack. The difficulties to make it shipped mean that the code review of ReactNative is quite hard, and I appreciate it.

These are my thoughts about this fix:

1. ScrollView and RecyclerView add views in a very different way, the latter is using an Adapter. so it's acceptable that internally they behave differently.
2. ReactScrollView(that extends ScrollView) is also using some fixes for the layout (eg. 'onLayout' method at lines 125-128)
3. RecyclerView overrides onAttachedToWindow method and it flags itself as the layout pass has not happened still, because normally the layout pass happens after onAttachedToWindow is called - but not in this case because of React. Besides, the fact that RecyclerView overrides onAttachedToWindow and ScrollView does not, indicates that there is already a strong difference in the behavior of these two views.
4. RecyclerViewBackedScrollView extends RecyclerView, so in order to fix the issue, we must override again onAttachedToWindow and call onLayout immediately after super.onAttachedToWindow().
5. The fix does not add any "strange" behavior to the RecyclerViewBackedScrollView: it just reminds the RecyclerViewBackedScrollView that the layout pass had already happened.
6. The real question is: why the issue does not occur when the RecyclerViewBackedScrollView is added to a simple ViewGroup and instead it occurs when it's added to a ViewPager? The answer is that the ViewPager adds new views through an Adapter, not immediately as a normal ViewGroup. So in this case, the RecyclerViewBackedScrollView is layout by React before being added to the view hierarchy.

I hope to have been able to explain in detail why this fix is the best fix currently :)

[mkonicek]

Why do you use the RecyclerViewBackedScrollView? We don't use it internally anymore and I was thinking of deleting the code, see #11095.

[godness84]

@mkonicek I used RecyclerViewBackedScrollView because at that time (7 months ago) ScrollView had some issues and it was less performant.

So ok, let's kill it :)

[lacker]

At this point it seems like the right way to go is for Martin to just kill the RecyclerViewBackedScrollView so I am going to close this pull request. Thanks for bringing this issue up!

[facebook-github-bot]

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

[facebook-github-bot]

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

[facebook-github-bot]

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