Make WordPress Core

Opened 16 years ago

Closed 16 years ago

#5299 closed defect (bug) (fixed)

JS/AJAX form validation

Reported by: mdawaffe's profile mdawaffe Owned by: mdawaffe's profile mdawaffe
Milestone: 2.5 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

Due to the new jQuery list manipulation code, there are a few forms in WP that, when submitted with no data (e.g. with a stray click or return), respond with "AJAX is teh b0rked".

That's a error message not meant to be seen by users :)

Currently, the jQuery wp-lists JS offers a few hooks where custom, per-form JS can be written to do arbitrary form validation and to prevent bad submissions. But repeating for each form "don't bother to submit if the form is empty. Display feedback as necessary instead" is obnoxious.

Also, WP can already do arbitrary form validation using the WP_Error class (witness the Add User form), but could be touched up a bit to give better feedback.

Proposed :

  1. Via JS: If a form element is empty and has some ancestor element with class "required", that ancestor element gets a new "invalid" class and the form is not submitted. Rationale: We know right away something is wrong, so we give feedback without having to send a request to the server.
  2. Via AJAX: If a WP_Error object is returned in the AJAX response (via the WP_Ajax_Response class), match up the error codes with the form elements and give a new "invalid" class to the first ancestor element with class "form-field" for each of those matching form elements. Also display the error messages as we currently do. Rationale: Since we can't know client side that the data is valid (without writing a lot of extra JS), we submit via AJAX and give feedback based on any WP_Error objects.

Example :

<form method="post" action="">
<table>
  <tr class="form-field required">
    <td>Name</td>
    <td><input type="text" name="name" /></td>
  </tr>
  <tr class="form-field">
    <td>Age</td>
    <td><input type="text" name="age" /></td>
  </tr>
</table>
</form>

If a user tries to submit an empty form, the first TR will immediately gets the "invalid" class and the form is not submitted. Why? It has the "required" class and one of it's descendants is an empty form element.

If the user enters a "Shirley" into the name field and "young" in the age field and submits, suppose a WP_Error object is returned:

object(wp_error)(2) {
  ["errors"]=>
  array(1) {
    ["age"]=>
    array(1) {
      [0]=>
      string(31) "You must give your age in years"
    }
  }
  ["error_data"]=>
  array(0) {
  }
}

The TR with the age field in it is given the "invalid class" and the error message is shown. Why? Because the returned WP_Error object contains an error with code "age".

So we'd have 3 methods of form validation.

  1. Current hooks in wp-lists JS that allow submission to be canceled/relegated to traditional (non-AJAX) request.
  2. Looking for blank inputs in elements with the "required" class.
  3. Interpreting WP_Error objects sent back in the AJAX response.

Attached :

  1. Implements proposal.
  2. Uses method 1 (current hooks) to prevent submission of blank on-the-fly AJAX category adding to posts on the Write screen.
  3. Uses methods 2 (required) and 3 (WP_Error) in concert to validate other AJAX based forms: Category adding from Manage -> Categories and Blogroll -> Categories, User adding from Users.

To Test :

  1. Hit enter in a blank Add New User form. Name, email, password should turn red.
  2. Enter " " as name, " " as email, "1" as password 1, "2" as password 2 in Add New User form. Submit. Fields should turn red, and detailed explanations are given.
  3. Hit enter in blank ajax-cat form on Write -> Post screen. Nothing should happen.
  4. Enter ",," (three blank categories) into ajax-cat form. Submit. Request is actually submitted, but nothing will happen server side (good), and nothing will happen after response comes through (good).
  5. Submit blank Add New Category form (Manage -> Categories). Name should turn red.
  6. Enter " " as name in Add New Category form. Submit. Name should turn red, should get error message.
  7. Repeat 5, 6 for Blogroll -> Categories form.

Attachments (3)

5299.diff (17.0 KB) - added by mdawaffe 16 years ago.
5299b.diff (17.0 KB) - added by mdawaffe 16 years ago.
5299c.diff (17.4 KB) - added by mdawaffe 16 years ago.

Download all attachments as: .zip

Change History (9)

@mdawaffe
16 years ago

#1 @ryan
16 years ago

Lines 150 and 151 of admin-ajax.php have a syntax error. 150 is missing a semi-colon and 151 seems out-of-place. I removed 151 and put a semi on 150 and everything worked as described. Big ++.

We need to return WP_Error from more of the functions used in admin-ajax.php. Some of them still return 0 for back compat reasons. We could add an arg to them that allows the caller to request WP_Error return.

@mdawaffe
16 years ago

#2 @mdawaffe
16 years ago

  • Keywords has-patch 2nd-opinion added
  • Owner changed from anonymous to mdawaffe
  • Status changed from new to assigned

5299b.diff

  1. Fixes syntax error on line 150.
  2. Line 151 is kept. It skips the addition of a category if it's name is ''.

#3 @mdawaffe
16 years ago

While they're up for grabs, what do people think about the class names?

invalid = some input in this box is no good
form-field = this box contains some inputs (that might end up being invalid)
required = this box contains required inputs

Should we standardize them?

form-invalid
form-field
form-required

input-invalid
input-field / input-holder
input-required

I guess I like the form- prefixed variants the best.

#4 @ryan
16 years ago

Prefixed ones sound good to me.

@mdawaffe
16 years ago

#5 @mdawaffe
16 years ago

5299c.diff

  1. form- prefixed classes.
  2. Fix line 151 for realies. Thanks rboren :)
  3. Bug in on the fly link category addition.

#6 @ryan
16 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [6303]) JS/AJAX form validation from mdawaffe. fixes #5299

Note: See TracTickets for help on using tickets.