[$1000] Dev - Pressing on ‘fix the errors’ on validation forms throw error message #13909
Comments
kavimuru
added
Daily
bfitzexpensify
added
the
External
melvin-bot
bot
changed the title
|
Job added to Upwork: https://www.upwork.com/jobs/~0147ce5eb30fa3ad88 |
|
Was able to reproduce. |
|
Current assignee @bfitzexpensify is eligible for the External assigner, not assigning anyone new. |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia ( |
melvin-bot
bot
added
the
Help Wanted
|
Triggered auto assignment to @aldo-expensify ( |
|
Proposal // 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 + 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}));
}Screen.Recording.2023-01-02.at.16.04.50.mov |
|
Proposal
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 Screen.Recording.2023-01-02.at.2.47.11.PM.mov |
ProposalThe error occurs due to Issue
Thus we need to use FixWe 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]}
|
Issue
Proposalpass down / forward the 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 updateTo address issue pointed out by @dhairyasenjaliya (#13909 (comment)) which is caused by this prop passed into ScrollView: To fix it i propose that we need to add another prop and set its default to be <ScrollView
...
-scrollToOverflowEnabled
+scrollToOverflowEnabled={this.props.scrollToOverflowEnabled}
>
+ScrollViewWithContext.defaultProps = {scrollToOverflowEnabled: true};And on the <ScrollViewWithContext
style={[styles.w100, styles.flex1]}
contentContainerStyle={styles.flexGrow1}
keyboardShouldPersistTaps="handled"
+ scrollToOverflowEnabled={false}
ref={el => this.form = el}
>Because as far as I looked, |
ProposalWe 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>
</>
);
|
|
@allroundexperts your ultimate fix will definitely fail on the native platform since you're suggesting to take value directly from
Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-01-02.at.16.21.37.mp4 |
|
@dhairyasenjaliya I'm not sure if this regression was ever fixed correctly. Even on native, |
|
@allroundexperts So its But the issue is that the |
ProposalI agree with @ali-thowfeek approach , the root cause is that the used ref in Form.js isn't forwarded , and based on this comment #11391 (comment) , I think it is wise to use the ref provided by App/src/components/Picker/index.js Lines 196 to 198 in 12fad41 We can use same implementation for the Form.js diff --git a/src/components/Form.js b/src/components/Form.js
index f3f4a5a0cf..c8d77412b4 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 */
@@ -254,7 +254,6 @@ class Form extends React.Component {
style={[styles.w100, styles.flex1]}
contentContainerStyle={styles.flexGrow1}
keyboardShouldPersistTaps="handled"
- ref={el => this.form = el}
>
<View style={[this.props.style]}>
{this.childrenWrapperWithProps(this.props.children)}
@@ -273,9 +272,9 @@ class Form extends React.Component {
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}));
+ // We subtract 10 to scroll slightly above the input
+ if (focusInput.measureLayout && typeof focusInput.measureLayout === 'function' && this.context && this.context.scrollViewRef) {
+ focusInput.measureLayout(this.context.scrollViewRef, (x, y) => this.context.scrollViewRef.scrollTo({y: y - 10, animated: false}));
}
}}
containerStyles={[styles.mh0, styles.mt5, styles.flex1]}
@@ -292,6 +291,7 @@ class Form extends React.Component {
Form.propTypes = propTypes;
Form.defaultProps = defaultProps;
+Form.contextType = ScrollContext;
export default compose(
withLocalize, |
|
Sorry for the delay, I will look into this in a few hours. |
Proposal 2Guys, as far I looked
So I propose that we have a Prop on Also this would fix the issue with what do you think @fedirjh ? |
|
Just coming back from OOO. I introduced that bit of code (measureLayout) here: #13499 when trying to redo something another reverted PR did. Thanks for the investigation so far. I'll make this internal meanwhile to catch up with the conversation and decide what to do next. |
|
@thesahindia why is this proposal downvoted? It seems to fix the issue, draft PR: #14209 Regarding this comment: #13909 (comment) , I wouldn't say that this proposal causes that regression because that is the behaviour we had before the PR #13514 broke this. That behaviour is fine IMO. From reading further proposals, this one also seems to be fixing the reference issue, but in a better way? #13909 (comment) I say a better way because I understand that then we can use the ref on I understand the possible performance issues mentioned here #13909 (comment) but maybe we should measure and see if it is a real problem before doing anything. Anyway, there is a lot of information here and thanks a lot for all the discussion/investigation done so far!. I'll come back to it tomorrow because it is the end of my day. |
No this behaviour wasn't present before #13514 This is the result we get after using the solution suggested at #13909 (comment) - Screen.Recording.2023-01-12.at.2.31.13.AM.mov |
We will get error on ios when we tap on the picker (if it's at bottom) 210420283-9ebb415b-84e7-4d69-82fb-f8dde589cfe6.mov |
Ahh, I see it now in IOS, thanks! |
|
I think we can let chrispader handle the regression from that PR if we are still figuring out the solution for it. The regression period was still active on that PR. |
Sounds like a good idea to me. @chrispader do you mind helping with this investigation? |
|
@bfitzexpensify, @thesahindia, @aldo-expensify Huh... This is 4 days overdue. Who can take care of this? |
|
DM's @chrispader in slack to see if we can get some help from him. Otherwise, tomorrow I'll start having a look at this again |
|
I think @ali-thowfeek is right, we shouldn't be using A slight slight modification of @ali-thowfeek's second proposal where we have two props such as This way, we can manually enable the By default,
Also, my fault that i forgot to forward the ref in |
|
Turns out that (either some pages have changed since i implemented this feature or) i missed some pages that have I think these changes also fit pretty well into a PR on this regression, so i'd just add those fixes too |
Here's a comparison on that: (i added some extra space between the picker and the rest of the form to illustrate this)
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-01-19.at.12.51.47.mp4
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-01-19.at.12.52.45.mp4 |
aldo-expensify
removed
Needs Reproduction
|
@bfitzexpensify, the C+ compensation is pending here |
|
yup, thanks for reminding totally forgot it! |
|
Appreciate the reminder @thesahindia! Strange that the bot didn't highlight this as being pushed to prod. Upwork job had expired so I've sent new offers out to yourself and @dhairyasenjaliya |
|
Accepted, thanks! |
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
https://expensify.slack.com/archives/C01GTK53T8Q/p1675842327530259 |
I believe we already have regression tests for this? No need for new steps? |
Agreed. Thank you for the other links @thesahindia! Checklist complete, closing issue |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
Pressing “fix the error” should not show any console error/warning
Actual Result:
Pressing “fix the error” shows console error/warning
’Uncaught TypeError: Cannot read properties of undefined (reading ‘left’) ' on web and on native platforms it shows ’ref.measureLayout must be called with a node handle or a ref to a native component`
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: v1.2.45-0
![native]()
Reproducible in staging?: Y (tested in web)
Reproducible in production?: y (tested in web)
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
**Expensify/Expensify Issue URL:** **Issue reported by:** @dhairyasenjaliya **Slack conversation:** https://expensify.slack.com/archives/C049HHMV9SM/p1672390562408519View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: