ScrollView, HorizontalScrollView: do not ignore null contentOffset 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 null contentOffset prop

Test Plan

Tested locally within RNTester

code

const Ex = () => {
  let _scrollView: ?React.ElementRef<typeof ScrollView> = React.useRef(null);
  const [offset, setOffset] = React.useState({x: 0, y: 20});
  setTimeout(() => {
    setOffset(undefined);
  }, 2000);

  return (
    <View>
      <ScrollView
        ref={_scrollView}
        automaticallyAdjustContentInsets={false}
        onScroll={() => {
          console.log('onScroll!');
        }}
        contentOffset={offset}
        scrollEventThrottle={200}
        style={styles.scrollView}>
        {ITEMS.map(createItemRow)}
      </ScrollView>
      <Button
        label="Scroll to top"
        onPress={() => {
          nullthrows(_scrollView.current).scrollTo({y: 0});
        }}
      />
      <Button
        label="Scroll to bottom"
        onPress={() => {
          nullthrows(_scrollView.current).scrollToEnd({animated: true});
        }}
      />
      <Button
        label="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,

react-native/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollViewManager.java

Line 305 in a38166c

public void setContentOffset(ReactHorizontalScrollView view, ReadableMap value) {

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}