Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update _scrollAnimatedValue offset of ScrollViews. #19481

Closed
wants to merge 15 commits into from

Conversation

miyabi
Copy link
Contributor

@miyabi miyabi commented May 29, 2018

Motivation

_scrollAnimatedValue offset of ScrollView is set once in UNSAFE_componentWillMount but it is never updated.
It causes unexpected render result.

rn-scrollview-fix1

So I suggest to update _scrollAnimatedValue offset when ScrollView contentInset is updated.

Test Plan

Here is a demo and an example code:

rn-scrollview-fix2

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

export default class RNScrollExample extends Component {
  state = { navigationBarHeight: 88 };
  onChangeDimensions = this.onChangeDimensions.bind(this);

  componentDidMount() {
    Dimensions.addEventListener('change', this.onChangeDimensions);
  }

  componentWillUnmount() {
    Dimensions.removeEventListener('change', this.onChangeDimensions);
  }

  onChangeDimensions({ window, screen }) {
    if (window.width > window.height) {
      this.setState({ navigationBarHeight: 44 });
    } else {
      this.setState({ navigationBarHeight: 88 });
    }
  }

  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 } = this.state;
    return (
      <SectionList
        automaticallyAdjustContentInsets={false}
        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, { height: this.state.navigationBarHeight }]}>
        <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,
    justifyContent: 'flex-end',
    alignItems: 'center',
    backgroundColor: '#ffffffcc',
  },
  headerText: {
    fontSize: 16,
    lineHeight: 44,
    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);

Release Notes

[IOS][BUGFIX][ScrollView] Fix that _scrollAnimatedValue offset is not updated even contentInset is updated.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 29, 2018
@react-native-bot react-native-bot added ✅Test Plan Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. API: Animated labels May 29, 2018
@pull-bot

This comment has been minimized.

@miyabi
Copy link
Contributor Author

miyabi commented May 29, 2018

I added Release Notes.

@hramos hramos removed the Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. label Jun 7, 2018
@facebook-github-bot
Copy link
Contributor

@miyabi 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.

@facebook-github-bot
Copy link
Contributor

@miyabi 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.

@miyabi
Copy link
Contributor Author

miyabi commented Aug 22, 2018

@shergin Could you review this PR?

@hramos hramos removed the 🔶APIs label Jan 24, 2019
@cpojer
Copy link
Contributor

cpojer commented Feb 25, 2019

Thanks for this PR! Sorry that it took so long to get back to you.

In order to merge it, please fix the flow error. Also, can you make it so the method is only invoked if the inset changed between props? Thank you!

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

const currentContentInset = this.props.contentInset || {};
const nextContentInset = nextProps.contentInset || {};
if (currentContentInset.top !== nextContentInset.top) {
this._scrollAnimatedValue.setOffset(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prettier/prettier: Replace ⏎········nextContentInset.top·||·0,⏎······ with nextContentInset.top·||·0

@miyabi
Copy link
Contributor Author

miyabi commented Feb 26, 2019

@cpojer Thank you for reviewing. I fixed error and updated my code.

@cpojer
Copy link
Contributor

cpojer commented Feb 26, 2019

Thank you! I don't think it's a good idea to create up to two empty objects every time the props change. Could you change your code to be a bit more verbose but doesn't create empty objects?

@miyabi
Copy link
Contributor Author

miyabi commented Feb 26, 2019

@cpojer I improved my code. Thanks!

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 26, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @miyabi in 58c9567.

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

@react-native-bot react-native-bot added Merged This PR has been merged. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 26, 2019
grabbou pushed a commit that referenced this pull request May 6, 2019
Summary:
`_scrollAnimatedValue` offset of ScrollView is set once in `UNSAFE_componentWillMount` but it is never updated.
It causes unexpected render result.

![rn-scrollview-fix1](https://user-images.githubusercontent.com/143255/40640292-61843eca-6350-11e8-9412-f5383ea65ea0.gif)

So I suggest to update `_scrollAnimatedValue` offset when ScrollView contentInset is updated.
Pull Request resolved: #19481

Differential Revision: D14223304

Pulled By: cpojer

fbshipit-source-id: 4191cfcf6414adf3a0abd156517d5f9778565671
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Animated CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants