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

ScrollView contentOffset bug #15808

Closed
vivalaakam opened this issue Sep 5, 2017 · 9 comments
Closed

ScrollView contentOffset bug #15808

vivalaakam opened this issue Sep 5, 2017 · 9 comments
Labels
Ran Commands One of our bots successfully processed a command. Resolution: Locked This issue was locked by the bot.

Comments

@vivalaakam
Copy link

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
  1. node -v
v8.4.0
  1. npm -v
5.4.0
  1. yarn --version
0.27.5
@stage88
Copy link

stage88 commented Sep 5, 2017

This is probably related to 1d22f8f, 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.

wschurman referenced this issue Sep 5, 2017
Summary:
Previous `contentOffset` can be invalid for a new layout and overscroll the ScrollView, so the diff fixes that.
Also documented here: #13566

Reviewed By: mmmulani

Differential Revision: D5414442

fbshipit-source-id: 7de1b4a4571108a37d1795e80f165bca5aba5fef
@shergin
Copy link
Contributor

shergin commented Sep 5, 2017

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
Copy link
Contributor

@shergin I think we should try landing #15670 to fix that regression

@krizpoon
Copy link

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
Copy link

@facebook-github-bot no-template

@facebook-github-bot
Copy link
Contributor

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.

@facebook-github-bot facebook-github-bot added the Ran Commands One of our bots successfully processed a command. label Oct 10, 2017
@henrikra
Copy link

@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
Copy link
Contributor

shergin commented Nov 1, 2017

@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
Copy link

henrikra commented Nov 1, 2017

@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 :/

@facebook facebook locked as resolved and limited conversation to collaborators Oct 10, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Oct 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Ran Commands One of our bots successfully processed a command. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

9 participants