HTML 5 form validation #10108

tyagiarpit · 2016-03-21T20:17:37Z

Implemented the stub methods for HTML5 Form validations.

This change is


        Added validation methods to struct


        Constructor modified, enum added in ValidityState


        Defined as_maybe_validatable on Element


        Defined as_maybe_validatable on Element


        Implement HTML5 form validation

highfive · 2016-03-21T20:17:43Z

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

highfive · 2016-03-21T20:17:45Z

Warning

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

jdm · 2016-03-21T20:53:23Z

Two issues that are being caught by TravisCI for us - be sure to run ./mach test-tidy and fix the stylistic errors that it reports. Additionally, the changes as written do not compile due to these errors:

/home/travis/build/servo/servo/components/script/dom/htmlbuttonelement.rs:271:55: 271:65 error: multiple applicable items in scope [E0034]
/home/travis/build/servo/servo/components/script/dom/htmlbuttonelement.rs:271             .map(|s| synthetic_click_activation(s.r().as_element(),
                                                                                                                                    ^~~~~~~~~~
/home/travis/build/servo/servo/components/script/dom/htmlbuttonelement.rs:271:55: 271:65 help: run `rustc --explain E0034` to see a detailed explanation
/home/travis/build/servo/servo/components/script/dom/htmlbuttonelement.rs:208:5: 210:6 note: candidate #1 is defined in an impl of the trait `dom::validation::Validatable` for the type `dom::htmlbuttonelement::HTMLButtonElement`
/home/travis/build/servo/servo/components/script/dom/htmlbuttonelement.rs:208     fn as_element(&self) -> &Element {
/home/travis/build/servo/servo/components/script/dom/htmlbuttonelement.rs:209         self.upcast()
/home/travis/build/servo/servo/components/script/dom/htmlbuttonelement.rs:210     }
/home/travis/build/servo/servo/components/script/dom/htmlbuttonelement.rs:219:5: 221:6 note: candidate #2 is defined in an impl of the trait `dom::activation::Activatable` for the type `dom::htmlbuttonelement::HTMLButtonElement`
/home/travis/build/servo/servo/components/script/dom/htmlbuttonelement.rs:219     fn as_element(&self) -> &Element {
/home/travis/build/servo/servo/components/script/dom/htmlbuttonelement.rs:220         self.upcast()
/home/travis/build/servo/servo/components/script/dom/htmlbuttonelement.rs:221     }
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:1111:55: 1111:65 error: multiple applicable items in scope [E0034]
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:1111                     synthetic_click_activation(button.as_element(),
                                                                                                                                    ^~~~~~~~~~
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:1111:55: 1111:65 help: run `rustc --explain E0034` to see a detailed explanation
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:913:5: 915:6 note: candidate #1 is defined in an impl of the trait `dom::validation::Validatable` for the type `dom::htmlinputelement::HTMLInputElement`
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:913     fn as_element(&self) -> &Element {
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:914         self.upcast()
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:915     }
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:927:5: 929:6 note: candidate #2 is defined in an impl of the trait `dom::activation::Activatable` for the type `dom::htmlinputelement::HTMLInputElement`
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:927     fn as_element(&self) -> &Element {
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:928         self.upcast()
/home/travis/build/servo/servo/components/script/dom/htmlinputelement.rs:929     }
error: aborting due to 2 previous errors
Build failed, waiting for other jobs to finish...
Could not compile `script`.


        Removed as_element from Validatable trait


        Fixed issues reported by tidy

jdm · 2016-03-22T19:51:59Z

Great work! There are some more changes we can make to increase the clarity of the new code, but this is looking good!
-S-awaiting-review +S-needs-code-changes

Reviewed 1 of 2 files at r1, 1 of 6 files at r2, 13 of 13 files at r4.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

components/script/dom/element.rs, line 1918 [r4] (raw file):
Let's add a // https://html.spec.whatwg.org/multipage/#category-submit comment above this.

components/script/dom/element.rs, line 1928 [r4] (raw file):
This isn't necessary, since anchor elements aren't submittable elements and can't be validated.

components/script/dom/element.rs, line 1932 [r4] (raw file):
Same as above.

components/script/dom/element.rs, line 1949 [r4] (raw file):
The specification doesn't list any exceptions for an element being considered submittable or not, so let's remove this check. We can just return element directly.

components/script/dom/htmlinputelement.rs, line 939 [r4] (raw file):
Let's remove these extraneous newlines.

components/script/dom/validation.rs, line 6 [r4] (raw file):
This method isn't necessary. In the future we'll want to add things like is_barred_from_constraint_validation and satisfies_constraints (ie. step 3 of (the spec)[https://html.spec.whatwg.org/multipage/forms.html#statically-validate-the-constraints]) but right now it's not needed.

components/script/dom/validitystate.rs, line 14 [r4] (raw file):
Let's add a /// https://html.spec.whatwg.org/multipage/#validity-states comment above this.

components/script/dom/validitystate.rs, line 33 [r4] (raw file):
Let's make state a ValidityStatus field instead, and default to Valid.

components/script/dom/validitystate.rs, line 55 [r4] (raw file):
https://html.spec.whatwg.org/multipage/#dom-validitystate-valuemissing

components/script/dom/validitystate.rs, line 60 [r4] (raw file):
https://html.spec.whatwg.org/multipage/#dom-validitystate-typemismatch

etc. for the remainder of the method comments here.

Comments from the review on Reviewable.io

jdm · 2016-03-22T19:52:15Z

@bors-servo: try

bors-servo · 2016-03-22T19:52:19Z

⌛ Trying commit 2300431 with merge 936b707...


        Auto merge of #10108 - tyagiarpit:master, r=<try>

HTML 5 form validation Implemented the stub methods for HTML5 Form validations.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10108)


        Merge remote-tracking branch 'upstream/master'

bors-servo · 2016-03-22T20:27:46Z

💔 Test failed - status-appveyor

srm09 · 2016-03-23T22:41:44Z

@jdm Tried squashing the commits into a single one, it shows some code which does not belong to From Validation feature. Not sure how to get rid of it!
What do you suggest? Should we recreate a new PR with a single commit

jdm · 2016-03-23T22:44:27Z

@srm912 It looks like it worked fine to me!

jdm · 2016-03-23T22:44:43Z

@bors-servo: r+

bors-servo · 2016-03-23T22:44:44Z

📌 Commit 13fac5d has been approved by jdm

bors-servo · 2016-03-24T00:54:04Z

⌛ Testing commit 13fac5d with merge c68e19f...


        Auto merge of #10108 - tyagiarpit:master, r=jdm

HTML 5 form validation Implemented the stub methods for HTML5 Form validations.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10108)

bors-servo · 2016-03-24T00:58:57Z

💔 Test failed - linux-dev

jdm · 2016-03-24T02:28:37Z

Ah, my mistake. It appears there were rebasing errors that undid recent changes in htmlinputelement.rs and htmllabelelement.rs. Those will need to be undone in order for the changes to merge.

jdm · 2016-03-24T13:09:07Z

Rebased in #10169. Thanks!


        Auto merge of #10169 - jdm:formvalidation, r=jdm

Implement initial pieces of form validation. Rebase of #10108.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10169)

srm09 and others added 5 commits Mar 18, 2016

Added validation methods to struct

830c6f7

Constructor modified, enum added in ValidityState

001c3ba

Defined as_maybe_validatable on Element

bf9ba4b

Defined as_maybe_validatable on Element

0abd332

Implement HTML5 form validation

Loading status checks…

93fb010

highfive assigned asajeffrey Mar 21, 2016

highfive added the S-awaiting-review label Mar 21, 2016

jdm assigned jdm and unassigned asajeffrey Mar 21, 2016

jdm added S-needs-code-changes S-fails-tidy and removed S-awaiting-review labels Mar 21, 2016

tyagiarpit added 2 commits Mar 21, 2016

Removed as_element from Validatable trait

ea3ebaa

Fixed issues reported by tidy

Loading status checks…

2300431

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 21, 2016

KiChjang changed the title ~~HTML 5 from validation~~ HTML 5 form validation Mar 21, 2016

frewsxcv removed the S-fails-tidy label Mar 22, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Mar 22, 2016

Merge remote-tracking branch 'upstream/master'

1a63bfb

highfive added the S-tests-failed label Mar 22, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Mar 23, 2016

srm09 force-pushed the tyagiarpit:master branch from 3e4d7e1 to 99f5d8a Mar 23, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 23, 2016

srm09 force-pushed the tyagiarpit:master branch 3 times, most recently from 742aaaf to 13fac5d Mar 23, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Mar 23, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 24, 2016

tyagiarpit force-pushed the tyagiarpit:master branch from 13fac5d to 7fdf055 Mar 24, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Mar 24, 2016

tyagiarpit force-pushed the tyagiarpit:master branch 2 times, most recently from 8c5785a to 7fdf055 Mar 24, 2016

jdm mentioned this pull request Mar 24, 2016

Implement initial pieces of form validation. #10169

Merged

jdm closed this Mar 24, 2016

servo / servo

HTML 5 form validation #10108

HTML 5 form validation #10108

tyagiarpit commented Mar 21, 2016

highfive commented Mar 21, 2016

highfive commented Mar 21, 2016

jdm commented Mar 21, 2016

jdm commented Mar 22, 2016

jdm commented Mar 22, 2016

bors-servo commented Mar 22, 2016

bors-servo commented Mar 22, 2016

srm09 commented Mar 23, 2016

jdm commented Mar 23, 2016

jdm commented Mar 23, 2016

bors-servo commented Mar 23, 2016

bors-servo commented Mar 24, 2016

bors-servo commented Mar 24, 2016

jdm commented Mar 24, 2016

jdm commented Mar 24, 2016

servo / servo

Join GitHub today

HTML 5 form validation #10108

HTML 5 form validation #10108

Conversation

tyagiarpit commented Mar 21, 2016

highfive commented Mar 21, 2016

highfive commented Mar 21, 2016

jdm commented Mar 21, 2016

jdm commented Mar 22, 2016

jdm commented Mar 22, 2016

bors-servo commented Mar 22, 2016

bors-servo commented Mar 22, 2016

srm09 commented Mar 23, 2016

jdm commented Mar 23, 2016

jdm commented Mar 23, 2016

bors-servo commented Mar 23, 2016

bors-servo commented Mar 24, 2016

bors-servo commented Mar 24, 2016

jdm commented Mar 24, 2016

jdm commented Mar 24, 2016