Skip to content

Commit

Permalink
ScrollView: Fix ref (and innerViewRef) Transition
Browse files Browse the repository at this point in the history 
Summary:
Normally when a `ref` prop supplied to a component is changed, the old `ref` receives `null` and the new `ref` receives the active component instance.

However, the current implementation of `ScrollView` has a bug where this does not happen (i.e. changing `ref` or `innerViewRef` does not cause either the old or new refs to be updated). This bug is due to a subtle issue with how `ScrollView` internally memoizes the `ref` (or `innerViewRef`) that is passed into the native component.

This commit fixes this behavior so that `ScrollView`'s `ref` and `innerViewRef` have the correct behavior.

Changelog:
[General][Fixed] When a ScrollView's `ref` or `innnerViewRef` changes, the old ref will now be invoked with `null` and the new ref with the active instance. (Previously, changing `ref` or `innerViewRef` on a `ScrollView` would be treated as though the ref had not changed at all.)

Reviewed By: sammy-SC

Differential Revision: D41208895

fbshipit-source-id: b465f666076edbef410cdf9661e040e1d8fa0404
  • Loading branch information
@yungsters @facebook-github-bot
yungsters authored and facebook-github-bot committed Nov 14, 2022
1 parent e36c492 commit 7cf4cf3
Show file tree
Hide file tree
Showing 2 changed files with 185 additions and 67 deletions.
191 changes: 124 additions & 67 deletions Libraries/Components/ScrollView/ScrollView.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import StyleSheet from '../../StyleSheet/StyleSheet';
import Dimensions from '../../Utilities/Dimensions';
import dismissKeyboard from '../../Utilities/dismissKeyboard';
import Platform from '../../Utilities/Platform';
import setAndForwardRef from '../../Utilities/setAndForwardRef';
import Keyboard from '../Keyboard/Keyboard';
import TextInputState from '../TextInput/TextInputState';
import View from '../View/View';
Expand All @@ -46,6 +45,7 @@ import ScrollViewContext, {HORIZONTAL, VERTICAL} from './ScrollViewContext';
import ScrollViewNativeComponent from './ScrollViewNativeComponent';
import ScrollViewStickyHeader from './ScrollViewStickyHeader';
import invariant from 'invariant';
import memoize from 'memoize-one';
import * as React from 'react';

if (Platform.OS === 'ios') {
Expand Down Expand Up @@ -165,6 +165,14 @@ export type ScrollViewImperativeMethods = $ReadOnly<{|
export type DecelerationRateType = 'fast' | 'normal' | number;
export type ScrollResponderType = ScrollViewImperativeMethods;

type NativeScrollViewInstance = React.ElementRef<HostComponent<mixed>>;
type PublicScrollViewInstance = $ReadOnly<{|
...$Exact<NativeScrollViewInstance>,
...ScrollViewImperativeMethods,
|}>;

type InnerViewInstance = React.ElementRef<typeof View>;

type IOSProps = $ReadOnly<{|
/**
* Controls whether iOS should automatically adjust the content inset
Expand Down Expand Up @@ -658,15 +666,13 @@ export type Props = $ReadOnly<{|
* A ref to the inner View element of the ScrollView. This should be used
* instead of calling `getInnerViewRef`.
*/
innerViewRef?: React.Ref<typeof View>,
innerViewRef?: ForwardedRef<InnerViewInstance>,
/**
* A ref to the Native ScrollView component. This ref can be used to call
* all of ScrollView's public methods, in addition to native methods like
* measure, measureLayout, etc.
*/
scrollViewRef?: React.Ref<
typeof ScrollViewNativeComponent & ScrollViewImperativeMethods,
>,
scrollViewRef?: ForwardedRef<PublicScrollViewInstance>,
|}>;

type State = {|
Expand Down Expand Up @@ -826,36 +832,6 @@ class ScrollView extends React.Component<Props, State> {
}
}

_setNativeRef: $FlowFixMe = setAndForwardRef({
getForwardedRef: () => this.props.scrollViewRef,
setLocalRef: ref => {
this._scrollViewRef = ref;

/*
This is a hack. Ideally we would forwardRef to the underlying
host component. However, since ScrollView has it's own methods that can be
called as well, if we used the standard forwardRef then these
methods wouldn't be accessible and thus be a breaking change.
Therefore we edit ref to include ScrollView's public methods so that
they are callable from the ref.
*/
if (ref) {
ref.getScrollResponder = this.getScrollResponder;
ref.getScrollableNode = this.getScrollableNode;
ref.getInnerViewNode = this.getInnerViewNode;
ref.getInnerViewRef = this.getInnerViewRef;
ref.getNativeScrollRef = this.getNativeScrollRef;
ref.scrollTo = this.scrollTo;
ref.scrollToEnd = this.scrollToEnd;
ref.flashScrollIndicators = this.flashScrollIndicators;
ref.scrollResponderZoomTo = this.scrollResponderZoomTo;
ref.scrollResponderScrollNativeHandleToKeyboard =
this.scrollResponderScrollNativeHandleToKeyboard;
}
},
});

/**
* Returns a reference to the underlying scroll responder, which supports
* operations like `scrollTo`. All ScrollView-like components should
Expand All @@ -868,19 +844,19 @@ class ScrollView extends React.Component<Props, State> {
};

getScrollableNode: () => ?number = () => {
return findNodeHandle(this._scrollViewRef);
return findNodeHandle(this._scrollView.nativeInstance);
};

getInnerViewNode: () => ?number = () => {
return findNodeHandle(this._innerViewRef);
return findNodeHandle(this._innerView.nativeInstance);
};

getInnerViewRef: () => ?React.ElementRef<typeof View> = () => {
return this._innerViewRef;
getInnerViewRef: () => InnerViewInstance | null = () => {
return this._innerView.nativeInstance;
};

getNativeScrollRef: () => ?React.ElementRef<HostComponent<mixed>> = () => {
return this._scrollViewRef;
getNativeScrollRef: () => NativeScrollViewInstance | null = () => {
return this._scrollView.nativeInstance;
};

/**
Expand Down Expand Up @@ -931,10 +907,15 @@ class ScrollView extends React.Component<Props, State> {
x = options.x;
animated = options.animated;
}
if (this._scrollViewRef == null) {
if (this._scrollView.nativeInstance == null) {
return;
}
Commands.scrollTo(this._scrollViewRef, x || 0, y || 0, animated !== false);
Commands.scrollTo(
this._scrollView.nativeInstance,
x || 0,
y || 0,
animated !== false,
);
};

/**
Expand All @@ -950,10 +931,10 @@ class ScrollView extends React.Component<Props, State> {
) => {
// Default to true
const animated = (options && options.animated) !== false;
if (this._scrollViewRef == null) {
if (this._scrollView.nativeInstance == null) {
return;
}
Commands.scrollToEnd(this._scrollViewRef, animated);
Commands.scrollToEnd(this._scrollView.nativeInstance, animated);
};

/**
Expand All @@ -962,10 +943,10 @@ class ScrollView extends React.Component<Props, State> {
* @platform ios
*/
flashScrollIndicators: () => void = () => {
if (this._scrollViewRef == null) {
if (this._scrollView.nativeInstance == null) {
return;
}
Commands.flashScrollIndicators(this._scrollViewRef);
Commands.flashScrollIndicators(this._scrollView.nativeInstance);
};

/**
Expand All @@ -990,7 +971,7 @@ class ScrollView extends React.Component<Props, State> {
this._additionalScrollOffset = additionalOffset || 0;
this._preventNegativeScrollOffset = !!preventNegativeScrollOffset;

if (this._innerViewRef == null) {
if (this._innerView.nativeInstance == null) {
return;
}

Expand All @@ -1004,7 +985,7 @@ class ScrollView extends React.Component<Props, State> {
);
} else {
nodeHandle.measureLayout(
this._innerViewRef,
this._innerView.nativeInstance,
this._inputMeasureAndScrollToKeyboard,
// $FlowFixMe[method-unbinding] added when improving typing for this parameters
this._textInputFocusError,
Expand Down Expand Up @@ -1047,10 +1028,14 @@ class ScrollView extends React.Component<Props, State> {
);
}

if (this._scrollViewRef == null) {
if (this._scrollView.nativeInstance == null) {
return;
}
Commands.zoomToRect(this._scrollViewRef, rect, animated !== false);
Commands.zoomToRect(
this._scrollView.nativeInstance,
rect,
animated !== false,
);
};

_textInputFocusError() {
Expand Down Expand Up @@ -1123,7 +1108,7 @@ class ScrollView extends React.Component<Props, State> {
) {
this._scrollAnimatedValueAttachment =
AnimatedImplementation.attachNativeEvent(
this._scrollViewRef,
this._scrollView.nativeInstance,
'onScroll',
[{nativeEvent: {contentOffset: {y: this._scrollAnimatedValue}}}],
);
Expand Down Expand Up @@ -1202,15 +1187,45 @@ class ScrollView extends React.Component<Props, State> {
this.props.onContentSizeChange(width, height);
};

_scrollViewRef: ?React.ElementRef<HostComponent<mixed>> = null;
_innerView: RefForwarder<InnerViewInstance, InnerViewInstance> =
createRefForwarder(
(instance: InnerViewInstance): InnerViewInstance => instance,
);

_scrollView: RefForwarder<
NativeScrollViewInstance,
PublicScrollViewInstance,
> = createRefForwarder(
(nativeInstance: NativeScrollViewInstance): PublicScrollViewInstance => {
// This is a hack. Ideally we would forwardRef to the underlying
// host component. However, since ScrollView has it's own methods that can be
// called as well, if we used the standard forwardRef then these
// methods wouldn't be accessible and thus be a breaking change.
//
// Therefore we edit ref to include ScrollView's public methods so that
// they are callable from the ref.

// $FlowFixMe[prop-missing] - Known issue with appending custom methods.
const publicInstance: PublicScrollViewInstance = Object.assign(
nativeInstance,
{
getScrollResponder: this.getScrollResponder,
getScrollableNode: this.getScrollableNode,
getInnerViewNode: this.getInnerViewNode,
getInnerViewRef: this.getInnerViewRef,
getNativeScrollRef: this.getNativeScrollRef,
scrollTo: this.scrollTo,
scrollToEnd: this.scrollToEnd,
flashScrollIndicators: this.flashScrollIndicators,
scrollResponderZoomTo: this.scrollResponderZoomTo,
scrollResponderScrollNativeHandleToKeyboard:
this.scrollResponderScrollNativeHandleToKeyboard,
},
);

_innerViewRef: ?React.ElementRef<typeof View> = null;
_setInnerViewRef: $FlowFixMe = setAndForwardRef({
getForwardedRef: () => this.props.innerViewRef,
setLocalRef: ref => {
this._innerViewRef = ref;
return publicInstance;
},
});
);

/**
* Warning, this may be called several times for a single keyboard opening.
Expand Down Expand Up @@ -1718,7 +1733,7 @@ class ScrollView extends React.Component<Props, State> {
const contentContainer = (
<NativeDirectionalScrollContentView
{...contentSizeChangeProps}
ref={this._setInnerViewRef}
ref={this._innerView.getForwardingRef(this.props.innerViewRef)}
style={contentContainerStyle}
removeClippedSubviews={
// Subview clipping causes issues with sticky headers on Android and
Expand Down Expand Up @@ -1804,12 +1819,15 @@ class ScrollView extends React.Component<Props, State> {
}

const refreshControl = this.props.refreshControl;
const scrollViewRef = this._scrollView.getForwardingRef(
this.props.scrollViewRef,
);

if (refreshControl) {
if (Platform.OS === 'ios') {
// On iOS the RefreshControl is a child of the ScrollView.
return (
<NativeDirectionalScrollView {...props} ref={this._setNativeRef}>
<NativeDirectionalScrollView {...props} ref={scrollViewRef}>
{refreshControl}
{contentContainer}
</NativeDirectionalScrollView>
Expand All @@ -1827,14 +1845,14 @@ class ScrollView extends React.Component<Props, State> {
<NativeDirectionalScrollView
{...props}
style={StyleSheet.compose(baseStyle, inner)}
ref={this._setNativeRef}>
ref={scrollViewRef}>
{contentContainer}
</NativeDirectionalScrollView>,
);
}
}
return (
<NativeDirectionalScrollView {...props} ref={this._setNativeRef}>
<NativeDirectionalScrollView {...props} ref={scrollViewRef}>
{contentContainer}
</NativeDirectionalScrollView>
);
Expand All @@ -1859,6 +1877,48 @@ const styles = StyleSheet.create({
},
});

type ForwardedRef<T> = {current: null | T, ...} | ((null | T) => mixed);

type RefForwarder<TNativeInstance, TPublicInstance> = {
getForwardingRef: (
?ForwardedRef<TPublicInstance>,
) => (TNativeInstance | null) => void,
nativeInstance: TNativeInstance | null,
publicInstance: TPublicInstance | null,
};

/**
* Helper function that should be replaced with `useCallback` and `useMergeRefs`
* once `ScrollView` is reimplemented as a functional component.
*/
function createRefForwarder<TNativeInstance, TPublicInstance>(
mutator: TNativeInstance => TPublicInstance,
): RefForwarder<TNativeInstance, TPublicInstance> {
const state: RefForwarder<TNativeInstance, TPublicInstance> = {
getForwardingRef: memoize(forwardedRef => {
return (nativeInstance: TNativeInstance | null): void => {
const publicInstance =
nativeInstance == null ? null : mutator(nativeInstance);

state.nativeInstance = nativeInstance;
state.publicInstance = publicInstance;

if (forwardedRef != null) {
if (typeof forwardedRef === 'function') {
forwardedRef(publicInstance);
} else {
forwardedRef.current = publicInstance;
}
}
};
}),
nativeInstance: null,
publicInstance: null,
};

return state;
}

/* $FlowFixMe[missing-local-annot] The type annotation(s) required by Flow's
* LTI update could not be added via codemod */
function Wrapper(props, ref: (mixed => mixed) | {current: mixed, ...}) {
Expand All @@ -1874,9 +1934,6 @@ ForwardedScrollView.displayName = 'ScrollView';

module.exports = ((ForwardedScrollView: $FlowFixMe): React.AbstractComponent<
React.ElementConfig<typeof ScrollView>,
$ReadOnly<{|
...$Exact<React.ElementRef<HostComponent<mixed>>>,
...ScrollViewImperativeMethods,
|}>,
PublicScrollViewInstance,
> &
ScrollViewComponentStatics);
Loading

0 comments on commit 7cf4cf3

Please sign in to comment.