App: [$1000] Dev - Pressing on ‘fix the errors’ on validation forms throw error message

@aldo-expensify, actually it is a regression from https://github.com/Expensify/App/pull/13514

Issue

this.form does not refer to the actual relative node, which is the underlying ScrollView

Proposal

pass down / forward the ref properly to ScrollViewWithContext component

        render() {
        return (
            <ScrollView
                // eslint-disable-next-line react/jsx-props-no-spreading
                {...this.props}
-                ref={this.scrollViewRef}
+                ref={this.props.innerRef || this.scrollViewRef}
                onScroll={this.setContextScrollPosition}
                scrollEventThrottle={this.props.scrollEventThrottle || MIN_SMOOTH_SCROLL_EVENT_THROTTLE}
                scrollToOverflowEnabled
            >
                <ScrollContext.Provider
                    value={{
-                        scrollViewRef: this.scrollViewRef,
+                        scrollViewRef: this.props.innerRef || this.scrollViewRef,
                        contentOffsetY: this.state.contentOffsetY,
                    }}
                >

- export default ScrollViewWithContext;
+ export default React.forwardRef((props, ref) => <ScrollViewWithContext innerRef={ref} {...props}/>);

This gives us the flexibility to either consume it using a ScrollContext.Consumer or directly pass a ref to the ScrollViewWithContext component.

update

To address issue pointed out by @dhairyasenjaliya (https://github.com/Expensify/App/issues/13909#issuecomment-1368838061) which is caused by this prop passed into ScrollView: scrollToOverflowEnabled which would happen anyway (iOS only) fixing the issue in question here.

To fix it i propose that we need to add another prop and set its default to be true:

<ScrollView
  ...
-scrollToOverflowEnabled
+scrollToOverflowEnabled={this.props.scrollToOverflowEnabled}
>

+ScrollViewWithContext.defaultProps = {scrollToOverflowEnabled: true};

And on the Form.js

<ScrollViewWithContext
    style={[styles.w100, styles.flex1]}
    contentContainerStyle={styles.flexGrow1}
    keyboardShouldPersistTaps="handled"
+  scrollToOverflowEnabled={false}
    ref={el => this.form = el}
>

Because as far as I looked, scrollToOverflowEnabled has been set to true by default in this PR (https://github.com/Expensify/App/pull/13514) to fix an issue with WorkspacePageWithSections component. @chrispader correct me if I’m wrong. (I’m new in here 😉) So it is safe to set scrollToOverflowEnabled to false for the Form component which in turn I believe will not need any regression testing.

@bfitzexpensify, the C+ compensation is pending here Also I think the reporting compensation for @dhairyasenjaliya is due.

Can you try your solution on a long form at ios? e.g. AddDebitCardPage, when opening picker it will throw an error

https://user-images.githubusercontent.com/83179295/210420283-9ebb415b-84e7-4d69-82fb-f8dde589cfe6.mov

I have added some top margin because the picker needs to be near the bottom.

@dhairyasenjaliya I’m not sure if this regression was ever fixed correctly. Even on native, this.form points to ref of the class component. When supplying it to measureLayout, we’re getting this error: As such, the this.form.scrollTo is undefined on native as well. Would be better to remove this functionality entirely since this is not available on any platform.

Proposal

We already wrap the view with ScrollViewWithContext, but we didn’t consume the provided value. So, what we need to do is to wrap it again with the consumer to receive the scroll view ref.

diff --git a/src/components/Form.js b/src/components/Form.js
index f3f4a5a0c..c5b76c721 100644
--- a/src/components/Form.js
+++ b/src/components/Form.js
@@ -10,7 +10,7 @@ import * as FormActions from '../libs/actions/FormActions';
 import * as ErrorUtils from '../libs/ErrorUtils';
 import styles from '../styles/styles';
 import FormAlertWithSubmitButton from './FormAlertWithSubmitButton';
-import ScrollViewWithContext from './ScrollViewWithContext';
+import ScrollViewWithContext, { ScrollContext } from './ScrollViewWithContext';
 
 const propTypes = {
     /** A unique Onyx key identifying the form */
@@ -256,34 +256,39 @@ class Form extends React.Component {
                     keyboardShouldPersistTaps="handled"
-                    ref={el => this.form = el}
                 >
-                    <View style={[this.props.style]}>
-                        {this.childrenWrapperWithProps(this.props.children)}
-                        {this.props.isSubmitButtonVisible && (
-                        <FormAlertWithSubmitButton
-                            buttonText={this.props.submitButtonText}
-                            isAlertVisible={_.size(this.state.errors) > 0 || Boolean(this.getErrorMessage()) || !_.isEmpty(this.props.formState.errorFields)}
-                            isLoading={this.props.formState.isLoading}
-                            message={_.isEmpty(this.props.formState.errorFields) ? this.getErrorMessage() : null}
-                            onSubmit={this.submit}
-                            onFixTheErrorsLinkPressed={() => {
-                                const errors = !_.isEmpty(this.state.errors) ? this.state.errors : this.props.formState.errorFields;
-                                const focusKey = _.find(_.keys(this.inputRefs), key => _.keys(errors).includes(key));
-                                const focusInput = this.inputRefs[focusKey];
-                                if (focusInput.focus && typeof focusInput.focus === 'function') {
-                                    focusInput.focus();
-                                }
-
-                                // We substract 10 to scroll slightly above the input
-                                if (focusInput.measureLayout && typeof focusInput.measureLayout === 'function') {
-                                    focusInput.measureLayout(this.form, (x, y) => this.form.scrollTo({y: y - 10, animated: false}));
-                                }
-                            }}
-                            containerStyles={[styles.mh0, styles.mt5, styles.flex1]}
-                            enabledWhenOffline={this.props.enabledWhenOffline}
-                            isDangerousAction={this.props.isDangerousAction}
-                        />
+                    <ScrollContext.Consumer>
+                        {({scrollViewRef}) => (
+                            <View style={[this.props.style]}>
+                                {this.childrenWrapperWithProps(this.props.children)}
+                                {this.props.isSubmitButtonVisible && (
+                                <FormAlertWithSubmitButton
+                                    buttonText={this.props.submitButtonText}
+                                    isAlertVisible={_.size(this.state.errors) > 0 || Boolean(this.getErrorMessage()) || !_.isEmpty(this.props.formState.errorFields)}
+                                    isLoading={this.props.formState.isLoading}
+                                    message={_.isEmpty(this.props.formState.errorFields) ? this.getErrorMessage() : null}
+                                    onSubmit={this.submit}
+                                    onFixTheErrorsLinkPressed={() => {
+                                        const errors = !_.isEmpty(this.state.errors) ? this.state.errors : this.props.formState.errorFields;
+                                        const focusKey = _.find(_.keys(this.inputRefs), key => _.keys(errors).includes(key));
+                                        const focusInput = this.inputRefs[focusKey];
+                                        if (focusInput.focus && typeof focusInput.focus === 'function') {
+                                            focusInput.focus();
+                                        }
+
+                                        // We substract 10 to scroll slightly above the input
+                                        if (focusInput.measureLayout && typeof focusInput.measureLayout === 'function') {
+                                            focusInput.measureLayout(scrollViewRef.current, (x, y) => scrollViewRef.current.scrollTo({y: y - 10, animated: false}));
+                                        }
+                                    }}
+                                    containerStyles={[styles.mh0, styles.mt5, styles.flex1]}
+                                    enabledWhenOffline={this.props.enabledWhenOffline}
+                                    isDangerousAction={this.props.isDangerousAction}
+                                />
+                                )}
+                            </View>
                         )}
-                    </View>
+                    </ScrollContext.Consumer>
                 </ScrollViewWithContext>
             </>
         );

Proposal

The error occurs due to this.form.scrollTo being undefined. However, we need to investigate why this is undefined. None of the above proposals try to find and ultimately fix this.

Issue

this.form returns reference to the class ScrollViewWithContext instead of the ScrollView component itself. As seen here: https://github.com/Expensify/App/blob/main/src/components/ScrollViewWithContext.js#L41-L42

Thus we need to use scrollViewRef property of the class component ScrollViewWithContext.

Fix

We need to change the following:

diff --git a/src/components/Form.js b/src/components/Form.js
index f3f4a5a0c..9f743cc73 100644
--- a/src/components/Form.js
+++ b/src/components/Form.js
@@ -275,7 +275,7 @@ class Form extends React.Component {
 
                                 // We substract 10 to scroll slightly above the input
                                 if (focusInput.measureLayout && typeof focusInput.measureLayout === 'function') {
-                                    focusInput.measureLayout(this.form, (x, y) => this.form.scrollTo({y: y - 10, animated: false}));
+                                    focusInput.measureLayout(this.form.scrollViewRef.current, (x, y) => this.form.scrollViewRef.current.scrollTo({y: y - 10, animated: false}));
                                 }
                             }}
                             containerStyles={[styles.mh0, styles.mt5, styles.flex1]}

Proposal

• Here for the web we are not getting this.form.scrollTo index due to that we are receiving an undefined warning we need to add more validation to check if form.scrollTo has a valid scroll index based on that we are moving screen with the keyboard open

• IMO we need to use _.isFunction to check & valid form.scrollTo this is already being used in many places

• Also need to change typeof focusInput.measureLayout === 'function' to -> _.isFunction(focusInput.measureLayout)

Changes

+  if (focusInput.measureLayout && _.isFunction(focusInput.measureLayout) && _.isFunction(this.form.scrollTo)) {
     focusInput.measureLayout(this.form, (x, y) => this.form.scrollTo({y: y - 10, animated: false}));
 }

Result

https://user-images.githubusercontent.com/47522946/210212706-89e5e3bc-42d9-4587-9c6a-67a5868ba7ec.mov

Proposal This causes by the function this.form.scrollTo inside onFixTheErrorsLinkPressed return undefinded. Solution Remove the block of code:

// We substract 10 to scroll slightly above the input
if (focusInput.measureLayout && typeof focusInput.measureLayout === 'function') {
 focusInput.measureLayout(this.form, (x, y) => this.form.scrollTo({y: y - 10, animated: false}));
  }

or add the check if the this.form.scrollTo is a function before using it (typeof this.form.scrollTo === ‘function’)

+ if (focusInput.measureLayout && typeof focusInput.measureLayout === 'function' && typeof this.form.scrollTo === 'function') {
focusInput.measureLayout(this.form, (x, y) => this.form.scrollTo({y: y - 10, animated: false}));
}

https://user-images.githubusercontent.com/16502320/210211504-c4ea89f6-8736-4629-a94e-7dae20eb4de5.mov