Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent redirecting to / on form submit #15240

Merged
merged 1 commit into from
Mar 9, 2023
Merged

fix: prevent redirecting to / on form submit #15240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/components/SignInPageForm/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ import React from 'react';
import FormElement from '../FormElement';

class Form extends React.Component {
constructor(props) {
super(props);

this.preventFormDefault = this.preventFormDefault.bind(this);
}

componentDidMount() {
if (!this.form) {
return;
Expand All @@ -10,6 +16,22 @@ class Form extends React.Component {
// 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);
}
Comment on lines 16 to +21
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cc: @luacmartins would you like to add anything?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, but it does not work.

Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


componentWillUnmount() {
if (!this.form) {
return;
}

this.form.removeEventListener('submit', this.preventFormDefault);
}

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();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional right, why prevent that?

cc: @thienlnam

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not use the Form here, curious why?

@luacmartins or @thienlnam ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Auto-submit the form when entering a 6-digit 2FA code
  2. This dark magic (here and here) to keep both LoginForm and PasswordForm mounted, but visibly hidden


render() {
Expand Down
1 change: 1 addition & 0 deletions src/pages/SetPasswordPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ class SetPasswordPage extends Component {
message={error}
isAlertVisible={!_.isEmpty(error)}
isDisabled={!this.state.isFormValid}
disablePressOnEnter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enter should submit the form, right? Can you clarify

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/>
</View>
</FormSubmit>
Expand Down