HTML5 Form Validation - Subsequent Steps

[vinay92]

Implemented the subsequent steps mentioned in the spec.

---

This change is

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @mbrubeck (or someone else) soon.

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/script/dom/htmltextareaelement.rs, components/script/dom/htmlbuttonelement.rs, components/script/dom/validitystate.rs, components/script/dom/validation.rs, components/script/dom/htmlformelement.rs, components/script/dom/htmlselectelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/htmlobjectelement.rs

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[KiChjang]

Review status: 0 of 8 files reviewed at latest revision, 1 unresolved discussion.

---

components/script/dom/htmlformelement.rs, line 812 [r1] (raw file):
I'm quite sad that nobody realized that I left a comment here that makes life easier when implementing form validations for form control elements.

---

Comments from Reviewable

[srm09]

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions.

---

components/script/dom/htmlformelement.rs, line 377 [r1] (raw file):
Follow snake casing (check_validity) for function and variable names

---

Comments from Reviewable

[jdm]

r? @KiChjang
Please reassign if you don't have time :)

[KiChjang]

-S-awaiting-review +S-needs-code-changes

As I've noted previously, we really should make the best use of the existing FormControl infrastructure, this would save you a lot of headaches :). Most of my comments revolve around how exactly we should be utilizing it. Don't hesitate to ask questions if it's still unclear!

---

Reviewed 6 of 8 files at r1, 3 of 24 files at r4, 1 of 1 files at r6, 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 17 unresolved discussions.

---

components/script/dom/htmlformelement.rs, line 358 [r1] (raw file):
What we should do instead is to upcast err[0] into an HTMLElement, and then call .Focus(). I'm also not sure whether this is necessary. @jdm or @Ms2ger, should the first invalid form element request focus on itself?

---

components/script/dom/htmlformelement.rs, line 367 [r1] (raw file):
This also isn't how I imagine the check for validation candidate should be implemented. As I've noted further down in this file, we can just add member methods on implementors of FormControl.

---

components/script/dom/htmlformelement.rs, line 377 [r1] (raw file):
This shouldn't be implemented here; this is part of the constraint validation API, which should live under impl HTMLFormElementMethods for HTMLFormElement.

---

components/script/dom/htmlformelement.rs, line 386 [r1] (raw file):
Ditto about being under impl HTMLFormElementMethods for HTMLFormElement.

---

components/script/dom/htmlformelement.rs, line 393 [r1] (raw file):
Ditto about member methods on implementors of FormControl.

---

components/script/dom/htmlformelement.rs, line 442 [r1] (raw file):
nit: Ending brace on newline.

---

components/script/dom/htmlinputelement.rs, line 903 [r1] (raw file):
It doesn't sound quite right that the form element requests focus on the first errored element, only then to have all other invalid elements to request focus as well. I'd like to see the spec for this.

---

components/script/dom/htmlselectelement.rs, line 237 [r6] (raw file):
Where is this being called?

---

components/script/dom/htmltextareaelement.rs, line 373 [r1] (raw file):
Spec for this behaviour, please.

---

components/script/dom/validation.rs, line 8 [r1] (raw file):
I'm actually not convinced that this trait (or this file for the matter) should exist. Following on my coment with FormControls, it seems unnecessary to add another trait which all FormControls should implement.

---

components/script/dom/validitystate.rs, line 47 [r7] (raw file):
Revert this and add that newline back, please.

---

components/script/dom/validitystate.rs, line 74 [r7] (raw file):
I'm not convinced that this is the optimal way of handling form control validity. The code here is divorced from the actual elements that implements the constraint validation API. I propose to add value_missing, type_mismatch and other methods to the FormControl trait, and in here, downcast self.element to FormControl and call the methods appropriately.

---

components/script/dom/validitystate.rs, line 649 [r7] (raw file):
nit: Wrong indentation and space before |.

---

components/compositing/compositor.rs, line 1308 [r4] (raw file):
This seems unrelated to your changes, an error from rebasing?

---

components/compositing/constellation.rs, line 890 [r4] (raw file):
Same for this file. Error from rebasing?

---

components/gfx/font_cache_thread.rs, line 173 [r4] (raw file):
Same for this file. Error from rebasing?

---

Comments from Reviewable

[KiChjang]

@vinay92 Is this ready for review? If not, just ping me whenever you want me to look at it.

[vinay92]

Hey Keith,

We are currently working on the changes you mentioned. We have an exam
tomorrow, once that is done, we will incorporate the remaining changes you
mentioned and ping you when it is ready for review.

Thank You,
Vinay.

On Thu, Apr 28, 2016 at 8:44 PM, Keith Yeung notifications@github.com
wrote:

@vinay92 https://github.com/vinay92 Is this ready for review? If not,
just ping me whenever you want me to look at it.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#10843 (comment)

[bors-servo]

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

[srm09]

Fixed errors while rebasing. No new code changes committed. Please do not review.

[raviflipsyde]

Hi @KiChjang ,
I am trying to fix the focus issue, when there is one or more invalid form element - user agent needs to focus on one of them. (spec: https://html.spec.whatwg.org/multipage/forms.html#interactively-validate-the-constraints)
as u advised, I've upcasted err[0] into an HTMLElement, and then call .Focus() on it.
The order in which the elements are getting focused is not in order though.

[KiChjang]

@raviflipsyde Is it because other invalid form elements are requesting focus as well? I had concerns about that in my review. Take a look at htmlinputelement.rs for example, it is requesting for focus when an "invalid" event is fired, causing it to conflict with your changes.

[raviflipsyde]

I guess that needs to be removed from there.

[highfive]

New code was committed to pull request.

[highfive]

New code was committed to pull request.

[highfive]

New code was committed to pull request.

[highfive]

New code was committed to pull request.

[vinay92]

@KiChjang We have made the changes you mentioned in the review. The branch is now ready for review. Thanks.

[KiChjang]

I don't think I have time for reviewing this in the near future. r? @jdm

[bors-servo]

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

[jdm]

I have the following concerns about this pull request:

• there is a lot of duplicated code (such as candidate_for_validation and satisfies_constraints)
• the mechanism of splitting each possible constraint into a separate function yields a lot of stub methods that just return false; it might make more sense to have a single method that returns a bit flags that represent failed constraints.
• there are several places that downcast to a particular element type and call the same sets of methods for each one - this would be better handled by obtaining a trait object and invoking the methods on that instead (like how as_maybe_activatable is used)
• we probably don't want to use regular expressions for the number, email, and URL input types since there are a lot of edge cases possible
• many of the validation methods could benefit from sharing common code (eg. TooShort, RangeUnderflow, RangeOverflow in HTMLInputElement)
• there are a number of instances where we parse strings as numbers and unwrap the result, which will panic for non-numeric values
• I'm not sure why the handle_event method is added for HTMLSelectElement

@KiChjang may have other more general architectural concerns. I'm closing this PR because I suspect that the original authors are not planning to continue working on it; if that assumption is untrue, they are welcome to do so and address the aforementioned concerns! I'm linking to this work from #11444 as the basis for any future implementation of the form validation.

[jdm]

That being said - thank you for doing this work and sharing your efforts! This will help us make better decisions when the time comes for someone else to finish your work.