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

[WIP] Finish implementation of HTML5 form validation. #17657

Closed
wants to merge 1 commit into from

Conversation

@gpoesia
Copy link
Contributor

gpoesia commented Jul 11, 2017

This is still work in progress, but comments would be highly appreciated.

Some questions:

1- The standard doesn't mention date and telephone validation (I checked which sections refer to the "type mismatch" validity state, and only URL and Email were there). That's why I left it out, but the last PR (#10843) had some validation for them. Should I bring it back?

2- In URL validation, I've checked whether there is either a '.' or a '/' (since for instance http://localhost is a valid URL). Is that fine?

3- For emails, I've only checked whether there is an '@' in the text. However, there's a suggested regex in the standard. Should we use it?

4- In the 'pattern mismatch' error, how do I use JavaScript's Regex engine? I'm guessing the regex crate might differ.

5- How do I handle 'multiple' inputs? Since there's only one value in an input element, I'm not sure how multiple inputs should be validated.

6- What does /tests/html/form_html5_validations.html actually test? Or is it simply a page for manual testing?

TODO:

  • Check for missing validity flags in HTMLInputElement: range errors (underflow/overflow), step mismatch, bad input, custom error
  • Implement HTMLSelectElement::validate
  • Implement HTMLTextAreaElement::validate
  • Implement the constraint validation API
  • Write WPT tests
  • Anything else?

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #11444 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented Jul 11, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/htmlformelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/validitystate.rs
  • @KiChjang: components/script/dom/htmlformelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/validitystate.rs
@highfive
Copy link

highfive commented Jul 11, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

The latest upstream changes (presumably #17822) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

KiChjang left a comment

This is just a prelimiary review and I'm only highlighting things that really jumped out to me.

The most surprising part here is the validation logic. I had expected that most of the validation logic to be self-contained in validitystate.rs instead of being shoved all into the validate method in htmlinputelement.rs.

@@ -1188,7 +1191,63 @@ impl Validatable for HTMLInputElement {
true
}
fn validate(&self, _validate_flags: ValidationFlags) -> bool {

This comment has been minimized.

@KiChjang

KiChjang Aug 3, 2017

Member

The _ prefix can be removed from validate_flags.

use dom::validitystate::*;

let value = self.Value();
let value_str: &str = value.borrow();

This comment has been minimized.

@KiChjang

KiChjang Aug 3, 2017

Member

Why is this call to borrow necessary?

@KiChjang
Copy link
Member

KiChjang commented Oct 23, 2017

@gpoesia Are you still working on this?

@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2017

The latest upstream changes (presumably #18262) made this pull request unmergeable. Please resolve the merge conflicts.

@KiChjang
Copy link
Member

KiChjang commented Nov 10, 2017

Closing due to inactivity.

@KiChjang KiChjang closed this Nov 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.