Fix content offset validation

[janicduplessis]

Content offset was broken because on initial render contentSize is {0,0} so any positive offset is lost. Also inset top/bottom and left/right were inversed 🙃, this led to bad initial scrolling offset when using contentInset. This fixes it by making sure contentSize is actually measured (not {0,0}. I guess it's possible that the content is ACTUALLY {0,0} but in that case I don't think it really matters).

Test plan
Tested that a scrollview has proper scroll position when specifying contentOffset. Also tested that it works well with contentInset.

<ScrollView contentOffset={{y: 100}}>
  <View style={{height: 1000}} />
</ScrollView>

[janicduplessis]

This and 950c2b2 should be cherry picked into 0.48.0-rc

[pull-bot]

@facebook-github-bot label Core Team

Attention: @shergin

Generated by 🚫 dangerJS

[shergin]

@janicduplessis I think, the problem here is that this code can produce incorrect (overscrolled) layout at a first run/layout?

[janicduplessis]

@shergin Maybe if you specify contentOffset that is actually overscrolling but otherwise it should be fine. From what I understand the issue was with overscrolling when the scrollview changes frame (on sonething like screen rotation) and in that case it should work.

[shergin]

@janicduplessis Don't get me wring, your solution is great indeed. But... you actually convinced me to dislike whole concept of having contentOffset as prop. 😂

Now I am thinking that right plan can be:

1. Land this PR. It is fixing the regression, so, even if will hurt second bullet, it is good thing to do.
2. Mark contentOffset as deprecated and enable yellowbox for it.
3. Investigate how we should improve RN to make calling scrollTo in componentDidMount buttery slick.

What do you think?

[janicduplessis]

Yes I like that plan, I think we should find a solution for point #3 before doing #2 though.

[facebook-github-bot]

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

[wschurman]

@shergin - Is the plan to merge this and related fixes into 0.48 (prod), 0.49 (rc), or just keep it in master for 0.50?

[shergin]

@wschurman You could advocate including this into RC if you want to. I have no strong point here.

[janicduplessis]

I posted in the 0.49-rc issue to get it cherry picked

[henrikra]

@janicduplessis Would it be possible for you to implement contentOffset to Android too? :)