ScrollView, HorizontalScrollView: do not ignore nullcontentOffset prop

[vonovak]

Summary

motivation: I was just checking out 30cc158
and noticed that the commit, I believe, is missing logic for when contentOffset is actually null.

That is, consider you render ScrollView with contentOffset { x: 0, y: 100 } and then change that to null / undefined. I'd expect the content offset to invalidate (set to 0 - hope that's the default).

Changelog

[Android] [Fixed] - ScrollView, HorizontalScrollView: do not ignore nullcontentOffset prop

Test Plan

Tested locally within RNTester

code

constEx=()=>{let_scrollView: ?React.ElementRef<typeofScrollView>=React.useRef(null);const[offset,setOffset]=React.useState({x: 0,y: 20});setTimeout(()=>{setOffset(undefined);},2000);return(<View><ScrollViewref={_scrollView}automaticallyAdjustContentInsets={false}onScroll={()=>{console.log('onScroll!');}}contentOffset={offset}scrollEventThrottle={200}style={styles.scrollView}>{ITEMS.map(createItemRow)}</ScrollView><Buttonlabel="Scroll to top"onPress={()=>{nullthrows(_scrollView.current).scrollTo({y: 0});}}/><Buttonlabel="Scroll to bottom"onPress={()=>{nullthrows(_scrollView.current).scrollToEnd({animated: true});}}/><Buttonlabel="Flash scroll indicators"onPress={()=>{nullthrows(_scrollView.current).flashScrollIndicators();}}/></View>);};

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	6,743,515	40
android	hermes	armeabi-v7a	6,408,491	47
android	hermes	x86	7,125,417	40
android	hermes	x86_64	7,015,801	55
android	jsc	arm64-v8a	8,912,035	27
android	jsc	armeabi-v7a	8,569,424	44
android	jsc	x86	8,737,078	55
android	jsc	x86_64	9,313,096	57

Base commit: 30cc158

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	809,568	0

Base commit: 30cc158

[JoshuaGross]

Hi @vonovak - what does iOS do, will it reset to 0,0 if you pass in null from JS?

[JoshuaGross]

Depending on what the behavior is for iOS, I actually don't think this is the correct thing to do. Passing in null equivalent to saying "delete this prop from the native side". I would intuitively expect that to actually cause no visible changes. If the engineer wants to reset the position to 0,0 I think those coordinates should be passed in directly.

Treating null as 0,0 also prevents us from doing the following in a typesafe manner: "Pass in coordinates if they're present, otherwise pass along no value, which will cause a noop". The only other way is to pass in condition ? { contentOffset: data } : {} and spread that into the element, which Flow and probably TypeScript don't like.

[vonovak]

@JoshuaGross Let me start with a premise that rendering the following should always render the same result (I hope that makes sense 😃 ). To give an example, imagine we render this ScrollView, then re-render it with different props and the last render looks again like this:

<ScrollView
contentOffset={undefined}>
{ITEMS.map(createItemRow)}
</ScrollView>

now if we take the example code I gave (I just changed null from the original post to undefined)

const Ex = () => {
const [offset, setOffset] = React.useState({x: 0, y: 20});
setTimeout(() => {
setOffset(undefined);
}, 2000);
return (
<View>
<ScrollView
contentOffset={offset}
style={styles.scrollView}>
{ITEMS.map(createItemRow)}
</ScrollView>
</View>
);
};

first, we start with contentOffset={{x: 0, y: 20}}, then we re-render with contentOffset={undefined}. At this point, https://github.com/facebook/react-native/blob/a38166cfa43472035c403ef9c6aee5a0d13a8a19/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollViewManager.java#L305

will be called with null because we effectively removed the contentOffset prop. With code from master, nothing will happen. That breaks the initial premise which is why I think the behavior is incorrect. I think if developer wants to control the offset "outside of react" they should use imperative scrollTo* methods.

edit: iOS will scroll to top for both setOffset(undefined) and setOffset(null). So this should unify the behavior.

It's rather interesting on iOS, I don't know the internals, but if I pass contentOffset={undefined}, that undefined is treated as nil in ObjectiveC and is transformed into CGPoint (x = 0, y = 0).

[vonovak]

@JoshuaGross would you please re-review? Thanks!

[vonovak]

@JoshuaGross sorry to ping you again, would you please take a look? The PR unifies behavior between ios and android. Thank you!

[JoshuaGross]

Thanks for clarifying that this will unify behavior! I will test a bit internally.

[facebook-github-bot]

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

[react-native-bot]

This pull request was successfully merged by @vonovak in 9e85b7a.

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