From 7cf4cf3afbea4463427944fbed30768a796db724 Mon Sep 17 00:00:00 2001 From: Tim Yung Date: Mon, 14 Nov 2022 08:50:22 -0800 Subject: [PATCH] ScrollView: Fix `ref` (and `innerViewRef`) Transition 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 --- Libraries/Components/ScrollView/ScrollView.js | 191 ++++++++++++------ .../ScrollView/__tests__/ScrollView-test.js | 61 ++++++ 2 files changed, 185 insertions(+), 67 deletions(-) diff --git a/Libraries/Components/ScrollView/ScrollView.js b/Libraries/Components/ScrollView/ScrollView.js index 81f05e169e0..eab0ece03c6 100644 --- a/Libraries/Components/ScrollView/ScrollView.js +++ b/Libraries/Components/ScrollView/ScrollView.js @@ -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'; @@ -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') { @@ -165,6 +165,14 @@ export type ScrollViewImperativeMethods = $ReadOnly<{| export type DecelerationRateType = 'fast' | 'normal' | number; export type ScrollResponderType = ScrollViewImperativeMethods; +type NativeScrollViewInstance = React.ElementRef>; +type PublicScrollViewInstance = $ReadOnly<{| + ...$Exact, + ...ScrollViewImperativeMethods, +|}>; + +type InnerViewInstance = React.ElementRef; + type IOSProps = $ReadOnly<{| /** * Controls whether iOS should automatically adjust the content inset @@ -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, + innerViewRef?: ForwardedRef, /** * 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, |}>; type State = {| @@ -826,36 +832,6 @@ class ScrollView extends React.Component { } } - _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 @@ -868,19 +844,19 @@ class ScrollView extends React.Component { }; 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 = () => { - return this._innerViewRef; + getInnerViewRef: () => InnerViewInstance | null = () => { + return this._innerView.nativeInstance; }; - getNativeScrollRef: () => ?React.ElementRef> = () => { - return this._scrollViewRef; + getNativeScrollRef: () => NativeScrollViewInstance | null = () => { + return this._scrollView.nativeInstance; }; /** @@ -931,10 +907,15 @@ class ScrollView extends React.Component { 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, + ); }; /** @@ -950,10 +931,10 @@ class ScrollView extends React.Component { ) => { // 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); }; /** @@ -962,10 +943,10 @@ class ScrollView extends React.Component { * @platform ios */ flashScrollIndicators: () => void = () => { - if (this._scrollViewRef == null) { + if (this._scrollView.nativeInstance == null) { return; } - Commands.flashScrollIndicators(this._scrollViewRef); + Commands.flashScrollIndicators(this._scrollView.nativeInstance); }; /** @@ -990,7 +971,7 @@ class ScrollView extends React.Component { this._additionalScrollOffset = additionalOffset || 0; this._preventNegativeScrollOffset = !!preventNegativeScrollOffset; - if (this._innerViewRef == null) { + if (this._innerView.nativeInstance == null) { return; } @@ -1004,7 +985,7 @@ class ScrollView extends React.Component { ); } else { nodeHandle.measureLayout( - this._innerViewRef, + this._innerView.nativeInstance, this._inputMeasureAndScrollToKeyboard, // $FlowFixMe[method-unbinding] added when improving typing for this parameters this._textInputFocusError, @@ -1047,10 +1028,14 @@ class ScrollView extends React.Component { ); } - 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() { @@ -1123,7 +1108,7 @@ class ScrollView extends React.Component { ) { this._scrollAnimatedValueAttachment = AnimatedImplementation.attachNativeEvent( - this._scrollViewRef, + this._scrollView.nativeInstance, 'onScroll', [{nativeEvent: {contentOffset: {y: this._scrollAnimatedValue}}}], ); @@ -1202,15 +1187,45 @@ class ScrollView extends React.Component { this.props.onContentSizeChange(width, height); }; - _scrollViewRef: ?React.ElementRef> = null; + _innerView: RefForwarder = + createRefForwarder( + (instance: InnerViewInstance): InnerViewInstance => instance, + ); - _innerViewRef: ?React.ElementRef = null; - _setInnerViewRef: $FlowFixMe = setAndForwardRef({ - getForwardedRef: () => this.props.innerViewRef, - setLocalRef: ref => { - this._innerViewRef = ref; + _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, + }, + ); + + return publicInstance; }, - }); + ); /** * Warning, this may be called several times for a single keyboard opening. @@ -1718,7 +1733,7 @@ class ScrollView extends React.Component { const contentContainer = ( { } 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 ( - + {refreshControl} {contentContainer} @@ -1827,14 +1845,14 @@ class ScrollView extends React.Component { + ref={scrollViewRef}> {contentContainer} , ); } } return ( - + {contentContainer} ); @@ -1859,6 +1877,48 @@ const styles = StyleSheet.create({ }, }); +type ForwardedRef = {current: null | T, ...} | ((null | T) => mixed); + +type RefForwarder = { + getForwardingRef: ( + ?ForwardedRef, + ) => (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( + mutator: TNativeInstance => TPublicInstance, +): RefForwarder { + const state: RefForwarder = { + 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, ...}) { @@ -1874,9 +1934,6 @@ ForwardedScrollView.displayName = 'ScrollView'; module.exports = ((ForwardedScrollView: $FlowFixMe): React.AbstractComponent< React.ElementConfig, - $ReadOnly<{| - ...$Exact>>, - ...ScrollViewImperativeMethods, - |}>, + PublicScrollViewInstance, > & ScrollViewComponentStatics); diff --git a/Libraries/Components/ScrollView/__tests__/ScrollView-test.js b/Libraries/Components/ScrollView/__tests__/ScrollView-test.js index 1af5e9d197b..5534b92ea4b 100644 --- a/Libraries/Components/ScrollView/__tests__/ScrollView-test.js +++ b/Libraries/Components/ScrollView/__tests__/ScrollView-test.js @@ -66,6 +66,67 @@ describe('ScrollView', () => { expect(scrollViewRef).toHaveBeenLastCalledWith(null); }); + + it('transitions between refs', () => { + jest.dontMock('../ScrollView'); + + const scrollViewRefA = jest.fn(); + const testRendererInstance = ReactTestRenderer.create( + , + ); + + expect(scrollViewRefA).toHaveBeenLastCalledWith( + expect.objectContaining({_nativeTag: expect.any(Number)}), + ); + + const scrollViewRefB = jest.fn(); + testRendererInstance.update(); + + expect(scrollViewRefA).toHaveBeenLastCalledWith(null); + expect(scrollViewRefB).toHaveBeenLastCalledWith( + expect.objectContaining({_nativeTag: expect.any(Number)}), + ); + }); + }); + + describe('innerViewRef', () => { + it('receives an instance or null', () => { + jest.dontMock('../ScrollView'); + + const innerViewRef = jest.fn(); + const testRendererInstance = ReactTestRenderer.create( + , + ); + + expect(innerViewRef).toHaveBeenLastCalledWith( + expect.objectContaining({_nativeTag: expect.any(Number)}), + ); + + testRendererInstance.unmount(); + + expect(innerViewRef).toHaveBeenLastCalledWith(null); + }); + + it('transitions between refs', () => { + jest.dontMock('../ScrollView'); + + const innerViewRefA = jest.fn(); + const testRendererInstance = ReactTestRenderer.create( + , + ); + + expect(innerViewRefA).toHaveBeenLastCalledWith( + expect.objectContaining({_nativeTag: expect.any(Number)}), + ); + + const innerViewRefB = jest.fn(); + testRendererInstance.update(); + + expect(innerViewRefA).toHaveBeenLastCalledWith(null); + expect(innerViewRefB).toHaveBeenLastCalledWith( + expect.objectContaining({_nativeTag: expect.any(Number)}), + ); + }); }); describe('getInnerViewRef', () => {