fix: prevent redirecting to / on form submit #15240

mdneyazahmad · 2023-02-17T06:08:46Z

Details

This PR prevents the default behaviour of form that is to submit on action url on web.

Fixed Issues

$ #7917
PROPOSAL: #7917 (comment)

Tests

Signup a new account using email
Open magic received from mail (replace domain to localhost)
Enter invalid password or don't type anything
Press enter
Verify that this is not redirected to /

Verify that no errors appear in the JS console

Offline tests

QA Steps

Signup a new account using email
Open magic received from mail
Enter invalid password or don't type anything
Press enter
Verify that this is not redirected to /

Verify that no errors appear in the JS console

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

mweb-chrome.mov

Mobile Web - Safari

mweb-safari.mov

Desktop

iOS

Android

MelvinBot · 2023-02-22T10:16:35Z

@thienlnam @Santhosh-Sellavel One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

Santhosh-Sellavel · 2023-02-22T18:00:10Z

src/pages/SetPasswordPage.js

@@ -117,6 +117,7 @@ class SetPasswordPage extends Component {
                                message={error}
                                isAlertVisible={!_.isEmpty(error)}
                                isDisabled={!this.state.isFormValid}
+                                disablePressOnEnter


Enter should submit the form, right? Can you clarify

App/src/pages/SetPasswordPage.js

Line 103 in 94057ba

<FormSubmit onSubmit={this.validateAndSubmitForm}>

Enter is handled in this component. disablePressOnEnter prevents attaching a global listener.

Santhosh-Sellavel · 2023-02-22T18:08:37Z

src/components/SignInPageForm/index.js

+    preventFormDefault(event) {
+        // When enter is pressed form is submitted to action url (POST /).
+        // As we are using controlled component, we need to disable it here.
+        event.preventDefault();
    }


This is intentional right, why prevent that?

cc: @thienlnam

Yeah I believe so, if this was using the form default submit action before then how does this work now?

Is there a onSubmit handler for the form now? Also if there is, then you can prevent eventDefault via the onSubmit handler instead of adding an event listener like this

App/src/pages/SetPasswordPage.js

Line 103 in 94057ba

<FormSubmit onSubmit={this.validateAndSubmitForm}>

Form submit is handled in this component. I believe, there is no onSubmit prop in react-native-web, this is the reason handled in this way.

We do not use the Form here, curious why?

@luacmartins or @thienlnam ?

I think mostly because Form doesn't have a couple of features used in the sign in pages:

Auto-submit the form when entering a 6-digit 2FA code

This dark magic (here and here) to keep both LoginForm and PasswordForm mounted, but visibly hidden

Santhosh-Sellavel · 2023-02-24T16:06:51Z

src/components/SignInPageForm/index.js

        // Password Managers need these attributes to be able to identify the form elements properly.
        this.form.setAttribute('method', 'post');
        this.form.setAttribute('action', '/');
+
+        this.form.addEventListener('submit', this.preventFormDefault);
+    }


I'm still confused, here we set post action. And we have a submit listener which

When enter is pressed form is submitted to action url (POST /).
As we are using controlled component, we need to disable it here.

@Santhosh-Sellavel this is the default browser behaviour to submit form data to action url, and this makes a full page refresh. As in our case this is a web app and does not require refresh. We need to prevent this default behaviour, otherwise user will be redirected to / when he presses enter on password input field. Form submition is manually handled using ajax request.

react-native-web does not accept onSubmit prop, therefore addEventListener is used here.

I'm okay here, but double checking @thienlnam Any thoughts?

cc: @luacmartins would you like to add anything?

react-native-web does not accept onSubmit prop

Where did you find this out? I'm pretty sure this is not true as we have it many places and onSubmit is a common prop that seems like it would be supported in react-native-web

I agree that this should be supported, and this has been raised earlier in this issue, also you can check the docs page here this mentions the supported props.

Took a look, it seems like in that issue they are trying to add a onSubmit handler to the View which isn't even supported in react-native. The on submit has to be applied to the button event

Following the code, it seems that we're setting up our own onSubmit event handler in FormSubmit which wraps is wrapped in all the sign in content

Can you try preventing default in the FormSubmit component?

I tried, but it does not work.

This feels off and weird, when I debug this code never executes i.e breakpoint never hit. So I remove the block and checked whether this occurs it didn't occur. But when I turned off debugging I was able to reproduce the issue and adding this back solves the issue.

If we are good with this lets go ahead and merge, thanks! @thienlnam

thienlnam

@Santhosh-Sellavel Have you run through the checklist for this PR yet? I'm hesitant since this feels like a workaround for prevent default submission but can we see if this doesn't break the login flow?

luacmartins · 2023-03-06T13:06:43Z

Bump on this comment @Santhosh-Sellavel

Santhosh-Sellavel · 2023-03-06T17:58:16Z

Thanks for the bump @luacmartins

Sorry, @thienlnam missed this comment, will run through the checklist and update you!

Santhosh-Sellavel · 2023-03-08T16:19:23Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web & Desktop

Screen.Recording.2023-03-09.at.12.41.30.AM.mov

Mobile Web - Chrome

Android_mWeb_log.mp4

Mobile Web - Safari

Simulator.Screen.Recording.-.iPhone.14.-.2023-03-09.at.00.52.14.mp4

iOS

N/A

Android

N/A

Santhosh-Sellavel · 2023-03-08T19:17:12Z

Tab to focus now brings to the footer it's okay, right?

@thienlnam or @luacmartins

Santhosh-Sellavel · 2023-03-08T19:29:56Z

@thienlnam it doesn't break the login flow. Tests well for me!

Santhosh-Sellavel

Tests well, just waiting for this

cc: @thienlnam @luacmartins

thienlnam · 2023-03-09T10:25:57Z

Tab to focus now brings to the footer it's okay, right?

Ah yeah, I'd say so now that we added that to the email page (should still be accesible for keyboard users)

OSBotify · 2023-03-09T10:31:16Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-03-10T14:50:18Z

🚀 Deployed to staging by https://github.com/thienlnam in version: 1.2.82-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-03-13T12:38:34Z

🚀 Deployed to production by https://github.com/Julesssss in version: 1.2.82-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

fix: prevent redirecting to / on form submit

3465895

mdneyazahmad marked this pull request as ready for review February 22, 2023 10:16

mdneyazahmad requested a review from a team as a code owner February 22, 2023 10:16

melvin-bot bot requested review from Santhosh-Sellavel and thienlnam and removed request for a team February 22, 2023 10:16

Santhosh-Sellavel reviewed Feb 22, 2023

View reviewed changes

Santhosh-Sellavel reviewed Feb 24, 2023

View reviewed changes

thienlnam reviewed Feb 28, 2023

View reviewed changes

Santhosh-Sellavel reviewed Mar 8, 2023

View reviewed changes

thienlnam approved these changes Mar 9, 2023

View reviewed changes

thienlnam merged commit 1860947 into Expensify:main Mar 9, 2023

OSBotify mentioned this pull request Mar 10, 2023

Deploy Checklist: New Expensify 2023-03-10 #15836

Closed

16 tasks

MelvinBot mentioned this pull request Mar 13, 2023

[HOLD for payment 2023-03-20] [HOLD #12370][$2000] Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 3/4 #7917

Closed

5 tasks

mdneyazahmad deleted the fix/7917-accessibility branch March 14, 2023 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent redirecting to / on form submit #15240

fix: prevent redirecting to / on form submit #15240

mdneyazahmad commented Feb 17, 2023 •

edited

Loading

MelvinBot commented Feb 22, 2023

Santhosh-Sellavel Feb 22, 2023

mdneyazahmad Feb 23, 2023

Santhosh-Sellavel Feb 22, 2023

thienlnam Feb 22, 2023

mdneyazahmad Feb 23, 2023

Santhosh-Sellavel Feb 23, 2023

luacmartins Feb 23, 2023

Santhosh-Sellavel Feb 24, 2023

mdneyazahmad Feb 24, 2023

mdneyazahmad Feb 24, 2023

Santhosh-Sellavel Feb 24, 2023 •

edited

Loading

thienlnam Feb 24, 2023

mdneyazahmad Feb 27, 2023

thienlnam Feb 27, 2023

mdneyazahmad Feb 27, 2023

Santhosh-Sellavel Mar 8, 2023 •

edited

Loading

thienlnam left a comment

luacmartins commented Mar 6, 2023

Santhosh-Sellavel commented Mar 6, 2023 •

edited

Loading

Santhosh-Sellavel commented Mar 8, 2023 •

edited

Loading

Santhosh-Sellavel commented Mar 8, 2023

Santhosh-Sellavel commented Mar 8, 2023 •

edited

Loading

Santhosh-Sellavel left a comment

thienlnam commented Mar 9, 2023

OSBotify commented Mar 9, 2023

OSBotify commented Mar 10, 2023

OSBotify commented Mar 13, 2023

fix: prevent redirecting to / on form submit #15240

fix: prevent redirecting to / on form submit #15240

Conversation

mdneyazahmad commented Feb 17, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Santhosh-Sellavel Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Santhosh-Sellavel Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

thienlnam left a comment

Choose a reason for hiding this comment

luacmartins commented Mar 6, 2023

Santhosh-Sellavel commented Mar 6, 2023 • edited Loading

Santhosh-Sellavel commented Mar 8, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

Santhosh-Sellavel commented Mar 8, 2023

Santhosh-Sellavel commented Mar 8, 2023 • edited Loading

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

thienlnam commented Mar 9, 2023

OSBotify commented Mar 9, 2023

OSBotify commented Mar 10, 2023

OSBotify commented Mar 13, 2023

mdneyazahmad commented Feb 17, 2023 •

edited

Loading

Santhosh-Sellavel Feb 24, 2023 •

edited

Loading

Santhosh-Sellavel Mar 8, 2023 •

edited

Loading

Santhosh-Sellavel commented Mar 6, 2023 •

edited

Loading

Santhosh-Sellavel commented Mar 8, 2023 •

edited

Loading

Santhosh-Sellavel commented Mar 8, 2023 •

edited

Loading