Add allowScrollOutOfBounds to ScrollView #22769

cabelitos · 2018-12-23T21:27:37Z

When using the scrollTo() function the user can specify a x/y position
that tells the ScrollView to scroll to. Prior to this patch the x/y were
not validated which could cause the SrollView to scroll to a position
outside the content view bounds, leaving the screen empty (the content
disapears, since it goes outside the screen). In order to fix this problem,
this commit imposes that the x/y must be inside the view bounds if allowScrollOutOfBounds
is set to false. This new prop is also set to true automatically to not break
existing apps which may depends on this feature.

Note that this behavior does not occur on Android, because the
Android's framework itself implements this features via 1 and 2, which are used
by React Native.

Changelog:

[iOS] [Added] - Add allowScrollOutOfBounds to ScrollView

Test Plan:

Run the following code on a iOS simulator/device and play around with the new allowScrollOutOfBounds property.

When set to false the scrollToOffset will stop at the last item (the item 12)
When set to true the scrollToOffset will scroll outside the content bounds and leave the screen completely white.

import React, {Component} from 'react';
import { StyleSheet, Text, View, FlatList, SafeAreaView } from 'react-native';

const data = [
  { key: "1" },
  { key: "2" },
  { key: "3" },
  { key: "4" },
  { key: "5" },
  { key: "6" },
  { key: "7" },
  { key: "8" },
  { key: "9" },
  { key: "10" },
  { key: "11" },
  { key: "12" }
];

export default class App extends Component {
  componentDidMount() {
    this.timeoutID = setTimeout(() => {
      if (this.flatList) {
        console.log('sdsadsadasdads');
        this.flatList.scrollToOffset({
          offset: 10000,
          animated: true
        });
      }
      this.timeoutID = null;
    }, 2000);
  }

  componentWillUnmount() {
    clearTimeout(this.timeoutID);
  }

  renderItem({ item: { key }}) {
    return (
      <View style={styles.view}>
        <Text style={styles.text}>{key}</Text>
      </View>
    )
  }

  onRef = (flatList) => {
    this.flatList = flatList;
  };

  timeoutID = null;
  flatList = null;

  render() {
    return (
      <SafeAreaView>
        <FlatList
          allowScrollOutOfBounds={false} // NOTE THIS NEW FLAG
          ref={this.onRef}
          style={styles.container}
          data={data}
          renderItem={this.renderItem}
        />
      </SafeAreaView>
    );
  }
}

const styles = StyleSheet.create({
  container: {
    paddingTop: 20,
    backgroundColor: '#F5FCFF',
  },
  view: {
    marginBottom: 10,
    justifyContent: 'center',
    alignItems: 'center',
    width: '100%',
    backgroundColor: 'green',
    height: 100,
  },
  text: {
    color: 'red',
    fontSize: 20,
  },
});

GIF demo

Without the patch

With the patch

fixes #22768

elicwhite · 2018-12-30T20:20:32Z

If I understand correctly, you are changing scrollTo so that if you try to scrollTo a location outside of the bounds of the scrollview it will be bounded to the size of the scrollview. And you are adding this prop so as to not change the current behavior?

I'm not very familiar with ScrollView behavior but I'm curious if the current behavior would be seen as a bug that we should just change without adding the additional prop. Maybe @sahrens or @wcandillon have some thoughts.

When using the scrollTo() function the user can specify a x/y position that tells the ScrollView to scroll to. Prior to this patch the x/y were not validated which could cause the SrollView to scroll to a position outside the content view bounds, leaving the screen empty (the content disapears, since it goes outside the screen). In order to fix this problem, this commit imposes that the x/y must be inside the view bounds if allowScrollOutOfBounds is set to false. This new prop is also set to true automatically to not break existing apps which may depends on this feature. Note that this behavior does not occur on Android, because the Android's framework itself implements this features via [1] and [2], which are used by React Native. [1]: https://developer.android.com/reference/android/widget/ScrollView#smoothScrollTo(int,%20int) [2]: https://developer.android.com/reference/android/widget/ScrollView.html#scrollTo(int,%20int)

cabelitos · 2019-01-01T16:52:27Z

Hello @TheSavior, yes you're correct. I'm only adding this prop to do not break the current behavior in iOS in case someone depends on this. But if you guys think it's not necessary I'll be glad to remove it.

I also updated my PR in order to fixing a problem that may happen on scrollviews that support both horizontal and vertical directions

By the way, thank you for your review and happy new year!

cabelitos · 2019-01-25T16:32:32Z

Hey guys, any news on this?

@TheSavior @sahrens

berickson1 · 2019-02-08T19:30:34Z

I was actually going to open an issue/PR to prevent this same programatic overscroll behaviour.

I don't think RN should expose a prop to keep the "existing" behaviour. This behaviour sounds entirely like a bug, especially since the first user interaction causes a rubber-band effect & the behaviour differs from Android (consistent behaviour should be used wherever possible between platforms)

I can't speak for Facebook on this matter, but I'd highly suggest taking this PR without the allowScrollOutOfBounds flag

cabelitos · 2019-02-09T15:05:02Z

@berickson1 I'm happy to remove the flag. I just to know what Facebook has to say

cpojer · 2019-03-26T22:03:53Z

@cabelitos can you rebase this PR and remove the allowScrollOutOfBounds?

cabelitos · 2019-03-27T17:39:36Z

@cpojer as a matter of fact a fix for this was already merged #23427 . This is soo sad, because I have this PR sitting here for months and that other PR was merged even though it was newer than mime.

I have no idea why @zhongwuzw implemented a fix for for a bug which I created and I provided the fix for it. He even links my bug in his PR.

cpojer · 2019-03-27T17:48:25Z

Oh you are right! I'm so sorry about that. Up until recently we didn't have the bandwidth to take care of all contributions. We recently worked through 600 Pull Requests (See https://facebook.github.io/react-native/blog/2019/03/01/react-native-open-source-update ) and are now much more active on the React Native repo. As you can see, the number of open Pull Requests is less than 50, which is much more manageable than it was in the past :) Again, I'm so sorry this happened but I'm hoping to see your contributions again in the future, and I'll make sure to provide feedback quickly and get your changes merged.

cabelitos · 2019-03-27T17:52:47Z

@cpojer np at all, these things happens specially when you have 500+ PRs. I have another PR open which fixes problems for ART on Android. I hope you guys are able to take a look into it!

zhongwuzw · 2019-03-28T01:23:41Z

@cabelitos Ahha, I'm sorry, I didn't see this PR when I linked your bug report. And I see your fix now, we try to fix in different way, you add an option to enable/disable overflow, maybe you can try to create a new PR to let react native team for review.

zhongwuzw · 2019-03-28T01:25:51Z

React/Views/ScrollView/RCTScrollView.m

+- (CGPoint)getMaxYOffset
+{
+  CGFloat offsetY = _scrollView.contentSize.height - _scrollView.bounds.size.height + _scrollView.contentInset.bottom;
+  return CGPointMake(0, fmax(offsetY, 0));


The minimum is not 0, we also need to consider _scrollView.contentInset.top.

cpojer · 2019-03-28T01:28:33Z

@zhongwuzw we agreed that your solution is fine, and that we don't want to add a new flag for this.

zhongwuzw · 2019-03-28T01:34:00Z

@cpojer Get it ! 👍
@cabelitos Apologize again! ❤️

mysport12 · 2019-03-31T17:58:42Z

@zhongwuzw we agreed that your solution is fine, and that we don't want to add a new flag for this.

@cpojer So there is no chance of adding a new flag for this to re-enable the old behavior? We were using the behavior quite extensively as an alternative way to position TextInputs/Dropdowns/etc on the screen when selected/focused and oftentimes that caused some over scroll of the content.

sheepmiee · 2019-05-27T06:44:01Z

@zhongwuzw we agreed that your solution is fine, and that we don't want to add a new flag for this.

@cpojer So there is no chance of adding a new flag for this to re-enable the old behavior? We were using the behavior quite extensively as an alternative way to position TextInputs/Dropdowns/etc on the screen when selected/focused and oftentimes that caused some over scroll of the content.

I agree with you, I used this old behavior to position TextInputs in previous projects, but now,I have to edit the native code to use the old behavior

zhongwuzw · 2019-05-27T06:52:23Z

@sheepmiee Hey, flag already be added in #24296, please see that.

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 Dec 23, 2018

react-native-bot added 🔶Components Component: ScrollView labels Dec 23, 2018

cabelitos changed the title ~~Add allowScrollOutOfBounds to ScrollView~~ [iOS][ScrollView] Add allowScrollOutOfBounds to ScrollView Dec 24, 2018

react-native-bot added the Platform: iOS iOS applications. label Dec 24, 2018

cabelitos force-pushed the scrollFix branch from ac83183 to 7c77b24 Compare January 1, 2019 16:50

elicwhite mentioned this pull request Jan 3, 2019

Flatlist scrollToIndex scrolls item all the way to the top of the list and produces whitespace #19080

Closed

3 tasks

hramos added ✅Changelog labels Jan 15, 2019

hramos removed the 🔶Components label Jan 24, 2019

berickson1 mentioned this pull request Feb 8, 2019

[iOS][VirtualizedList] Fix scroll over bottom with scrollToIndex #19970

Closed

hramos changed the title ~~[iOS][ScrollView] Add allowScrollOutOfBounds to ScrollView~~ Add allowScrollOutOfBounds to ScrollView Feb 25, 2019

react-native-bot added PR: Includes Changelog Type: Enhancement A new feature or enhancement of an existing feature. labels Mar 14, 2019

hramos removed the PR: Includes Changelog label Mar 14, 2019

cpojer closed this Mar 27, 2019

zhongwuzw reviewed Mar 28, 2019

View reviewed changes

Add allowScrollOutOfBounds to ScrollView #22769

Add allowScrollOutOfBounds to ScrollView #22769

Uh oh!

Conversation

cabelitos commented Dec 23, 2018

Changelog:

Test Plan:

GIF demo

Without the patch

With the patch

Uh oh!

elicwhite commented Dec 30, 2018

Uh oh!

cabelitos commented Jan 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cabelitos commented Jan 25, 2019

Uh oh!

berickson1 commented Feb 8, 2019

Uh oh!

cabelitos commented Feb 9, 2019

Uh oh!

cpojer commented Mar 26, 2019

Uh oh!

cabelitos commented Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpojer commented Mar 27, 2019

Uh oh!

cabelitos commented Mar 27, 2019

Uh oh!

zhongwuzw commented Mar 28, 2019

Uh oh!

zhongwuzw Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

cpojer commented Mar 28, 2019

Uh oh!

zhongwuzw commented Mar 28, 2019

Uh oh!

mysport12 commented Mar 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sheepmiee commented May 27, 2019

Uh oh!

zhongwuzw commented May 27, 2019

Uh oh!

Uh oh!

cabelitos commented Jan 1, 2019 •

edited

Loading

cabelitos commented Mar 27, 2019 •

edited

Loading

mysport12 commented Mar 31, 2019 •

edited

Loading