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.

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

only use event.preventDefault() in submitHandler if form invalid #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xini
Copy link

@xini xini commented Oct 11, 2021

This makes sure that the original submit event is passed on and the submit button name is submitted to the backend.

This makes sure that the original submit event is passed on and the submit button name is submitted to the backend.
@cferdinandi
Copy link
Owner

Can you say a bit more? Open to this PR, just want to understand it.

@xini
Copy link
Author

xini commented Oct 11, 2021

Hi Chris,

Yes, of course. Sorry.

The current submitHandler catches the default submit event and creates a new submit event using js if the form is valid. This causes two issues:

  1. When a form has multiple submit buttons, because the original submit event is caught and replaced with a generic js event, the name of the submit button that was clicked is not submitted with the request.

  2. Say you want to submit the form via AJAX. Because the original submit event was caught and replaced with a js event, any other event listener can't intercept that event anymore. JS submits can't be intercepted, which means that when you use the validator, you can't have AJAX forms.

Does this make it any clearer?

I have tested my changes and as far as I can tell they work fine.

Cheers Flo

@cferdinandi
Copy link
Owner

Thank you. Yes, that makes perfect sense, and I appreciate the clarification.

I'll test this locally, run an updated build, and merge later when I get a spare moment. Thanks for this!

@xini
Copy link
Author

xini commented Oct 12, 2021

Thank you very much.

xini added a commit to xini/silverstripe-form-validation that referenced this pull request Oct 12, 2021
@QJan84
Copy link

QJan84 commented Apr 27, 2022

Is this problem solved? I have a form with multiple submits (formaction). The submits also have GET parameters (e.g. ?nextPage=43535) that are not submitted.

var submitHandler = function (event) {

  // Only run on matching elements
  if (!event.target.matches(selector)) return;
  
  // Prevent form submission
  event.preventDefault(); // <------------ does this kill my GET parameter?
  
  // Validate each field
  var errors = publicAPIs.validateAll(event.target);
  
  // If there are errors, focus on the first one
  if (errors.length > 0) {
  // event.preventDefault(); // <------------ Shouldn't that be here?
  errors[0].focus();
  emitEvent(event.target, 'bouncerFormInvalid', {errors: errors});
  return;
  }
  
  // Otherwise, submit if not disabled
  if (!settings.disableSubmit) {
  event.target.submit();
 }

  
  // Emit custom event
  if (settings.emitEvents) {
  emitEvent(event.target, 'bouncerFormValid');
  }
  
};

@QJan84
Copy link

QJan84 commented Apr 27, 2022

Thank you. Yes, that makes perfect sense, and I appreciate the clarification.

I'll test this locally, run an updated build, and merge later when I get a spare moment. Thanks for this!

Can you please add this to the master brunch?

Copy link

@QJan84 QJan84 left a comment

Choose a reason for hiding this comment

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

Works fine!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants