Fix that section headers in SectionList don't stick at correct position.

[miyabi]

Motivation

I noticed that section headers in SectionList don't stick at correct position in case of using with contentInset and contentOffset. (See the demo below. It looks that contentInset.top is ignored.)
This is a common case of use of NavigationBar and TableView on iOS.

Test Plan

Here is a demo and an example code:

import React, { Component } from 'react';
import {
  AppRegistry,
  StyleSheet,
  Text,
  View,
  SectionList,
} from 'react-native';

export default class RNScrollExample extends Component {
  renderSectionHeader(title) {
    return (
      <View style={styles.sectionHeader}>
        <Text>{title}</Text>
      </View>
    )
  }

  renderItem(content) {
    return (
      <View style={styles.cell}>
        <Text>{`Item ${content}`}</Text>
      </View>
    )
  }

  renderSeparator() {
    return <View style={styles.separator} />
  }

  renderSectionList() {
    const sections = Array.from(Array(10), (e, i) => ({ title: `Section ${i+1}`, data: Array.from(Array(10)).map((e, i) => i+1) }))
    const navigationBarHeight = 64
    return (
      <SectionList
        contentInset={{ top: navigationBarHeight }}
        contentOffset={{ y: -navigationBarHeight }}
        sections={sections}
        keyExtractor={(item, index) => index}
        renderSectionHeader={({ section }) => this.renderSectionHeader(section.title)}
        renderItem={({ item }) => this.renderItem(item)}
        ItemSeparatorComponent={this.renderSeparator}
      />
    )
  }

  renderHeader() {
    return (
      <View style={styles.header}>
        <Text style={styles.headerText}>Contents</Text>
      </View>
    )
  }

  render() {
    return (
      <View>
        {this.renderSectionList()}
        {this.renderHeader()}
      </View>
    )
  }
}

const styles = StyleSheet.create({
  header: {
    position: 'absolute',
    top: 0,
    left: 0,
    right: 0,
    height: 64,
    paddingTop: 20,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: '#ffffffcc',
  },
  headerText: {
    fontSize: 16,
    fontWeight: 'bold',
  },
  sectionHeader: {
    paddingHorizontal: 8,
    paddingVertical: 4,
    backgroundColor: '#05bbd3',
  },
  cell: {
    paddingHorizontal: 8,
    paddingVertical: 16,
    backgroundColor: '#82dde9',
  },
  separator: {
    height: 1,
    backgroundColor: '#7d888d',
  },
});

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

[janicduplessis]

Thanks for the PR! JS changes look good.

@shergin Can you confirm this is ok on the iOS side. I think this should also fix the issue I was talking about when using inset + offset to change the position of RefreshControl.

[shergin]

What the point of this change? I always thought that contentOffset already "contains" contentInset, so when contentOffset is {0, 0}, the insets are visible. So, when contentOffset is {-contentInset.top, 0} we have doubled top inset. Am I wrong?

[janicduplessis]

That’s the code I’m using to make insets visible so I guess they are not included.

contentInset={{ top: HEADER_MAX_HEIGHT }}
contentOffset={{ y: -HEADER_MAX_HEIGHT }}

[shergin]

Sorry, I was terribly wrong. We have to fix this asap.

https://www.objc.io/issues/3-views/scroll-view/

The property contentInset can change the maximum and minimum values of the content offset to allow scrolling outside of the scrollable area. Its type is UIEdgeInsets, which consists of 4 numbers: {top, left, bottom, right}. When you introduce an inset, you change the range of the content offset. For example, setting the content inset to have a value of 10 for its top value allows the content offset’s y value to reach -10. This introduces padding around the scrollable area.

[shergin]

I mean we have to fix that for all four sides, in separate PR. I will be able to do it later this week. Or PR is welcomed. 😄

[janicduplessis]

@shergin I looked at the issue with the RCTScrollView#setFrame code and it seems worse than I thought. What happens is that when setFrame is called initially the content view isn't measured yet so self.contentSize has size {0,0}. This makes it so setting the contentOffset prop has no effect on initial render since initial offset will always be {0,0} (or -contentInset.{top|left} with the fix from this PR). I'll let you have a look since I'm not sure what was the bug the changes in setFrame were supposed to fix.

[shergin]

@janicduplessis Yeah, you are absolutely right. We also have pretty similar bug internally. 😢 And I don't have good idea how to fix that. As I wrote internally:

I understand why it is happening but I have no idea how to fix it properly (without reverting the diff). And the problem seems that there is no enough content size in the rendering moment to scroll to desired offset.
Conceptually, the problem is that we should not have scrollOffset property in the first place, because it is momentary value, not something permanent, and because of this the semantic of this property is unclear.

So, let's imagine we have scrollOffset={10,10} enforced in render method.
Questions:
When exactly ScrollView must scroll to this offset?
What if there is no enough content for this?
Should user actions (touch gestures) reset this value?
Why should we scroll to this offset only when it is changing?

So, again, because it is momentary, it the perfect world, we have to use scrollTo method instead of using this prop.

Any ideas how we can address this?

[shergin]

And for context, that diff is supposed to fix the problem known as #13566

[janicduplessis]

I feel like contentOffset is kind of weird right now and should be deprecated or at least make it's behaviour clearer. The only use case it has is to set the initial scroll position because using refs with scrollTo will usually result in a flicker for a frame.

I was thinking changing it so something like initialContentOffset so it's clear it only affects the scroll position of the ScrollView when it's mounted and that changing the property after that won't have any effect. With these constraints do you think it would be possible to fix both issues? We'd probably have to find a way to wait for the ScrollView's content to be measured before adjusting contentOffset to the initial offset. I haven't worked with that code much recently so hopefully that makes sense.

[shergin]

Having reactive and declarative nature of React and React Native, I don't think that even having initialContentOffset will make it clear. Because... Which moment is "initial"? What if some ancestor component was rebuilt? How RN should response on changing this value later, on runtime?

@sahrens Do you have any thoughts about it?

[shergin]

(I fixed the issue with offsets, 950c2b2. )

[janicduplessis]

@miyabi Could you rebase this on master with only the fix in ScrollView.js. Insets are still a bit broken on master currently but I have a PR to fix it (for good hopefully :))

[miyabi]

@janicduplessis Thank you for your work. I have merged master into my fork.

[miyabi]

Any updates? Is there anything that I can do?

[janicduplessis]

Sorry forgot about this PR, looks good!

[janicduplessis]

@facebook-github-bot shipit

[janicduplessis]

@shergin Could you land this, the bot still doesn't work :(

[shergin]

@janicduplessis Can this issue be related to a mistake described in a discussion here: 950c2b2 ?

[janicduplessis]

@shergin This was affected by the contentOffset regressions but we still need this so sticky headers appear correctly if a view uses contentOffset/Inset. We need to initialize the animated value at the right position and add an offset to make up for contentInset.

[facebook-github-bot]

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

[facebook-github-bot]

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