facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
119.22k stars 24.33k forks source link

ScrollView contentOffset bug #15808

Closed vivalaakam closed 7 years ago

vivalaakam commented 7 years ago

Hi! in RN 0.48.1 doesn`t work contentOffset when component is initialized. Here is example for reproduction https://github.com/vivalaakam/rn_issue

In this example after initialize i will be see second screen, named 'Welcome to screen #2'

In version 0.47.2 it work as well

  1. react-native -v
    react-native-cli: 2.0.1
    react-native: 0.48.1
  2. node -v
    v8.4.0
  3. npm -v
    5.4.0
  4. yarn --version
    0.27.5
stage88 commented 7 years ago

This is probably related to 1d22f8fb27e9432e357401ce6f6cede4d710472c, the scenario the commit addresses doesn't apply to me so i have replaced line 319 in node_modules/react-native/React/Views/RCTScrollView.m with

// self.contentOffset = CGPointMake(
//   MAX(0, MIN(originalOffset.x, fullContentSize.width - boundsSize.width)),
//   MAX(0, MIN(originalOffset.y, fullContentSize.height - boundsSize.height)));
self.contentOffset= originalOffset;

That fixes my negative offset with refresh control problem, it will have to do until the issue is fixed.

shergin commented 7 years ago

Well, the problem with this issue is that contentOffset prop should not exist in the first place. (And it does not exist on Android.) Could we use scrollTo method instead?

janicduplessis commented 7 years ago

@shergin I think we should try landing https://github.com/facebook/react-native/pull/15670 to fix that regression

krizpoon commented 7 years ago

I have similar problem and solved it with:

self.contentOffset = CGPointMake(
    MAX(-contentInset.left, MIN(originalOffset.x, fullContentSize.width - boundsSize.width)),
    MAX(-contentInset.top, MIN(originalOffset.y, fullContentSize.height - boundsSize.height)));
pull-bot commented 7 years ago

@facebook-github-bot no-template

facebook-github-bot commented 7 years ago

Hey, thanks for reporting this issue! It looks like your description is missing some necessary information, or the list of reproduction steps is not complete. Can you please add all the details specified in the Issue Template? This is necessary for people to be able to understand and reproduce the issue being reported. I am going to close this, but feel free to open a new issue with the additional information provided. Thanks! See "What to Expect from Maintainers" to learn more.

henrikra commented 7 years ago

@shergin Could you explain more why contentOffset prop should not exists? For example now on Android I have to do weird hacks to init scroll view's position to something that I want.

shergin commented 7 years ago

@henrikra It was discussed here several times. And yes, it is kinda controversial; we will not introduce breaking change before we have a consensus, but I pesonally think this prop should be removed. The problem with this props is that it is unclear when (at which stage) we have to enforce/apply the offset, it causes bugs and unexpected behaviors. (And, I assume, the situation became much worse with async Fiber.) Such kind of momentum values should be represented as methods, not props.

henrikra commented 7 years ago

@shergin Okay so what would be ideal api for this? Do you have suggestions? Since we have to do the offsetting on native level so there isn't any flickering like now :/