html form validation initial steps with test html file #13969

bbansalWolfPack · 2016-10-29T01:24:28Z

Added code for initial steps in html form validation.

Added methods for trait validatable
implemented stub methods for elements like HTMLInputElement, HTMLButtonElement, etc

3. Added code to call methods from static validation inHTMLFormElement.rs file

[X ] ./mach build -d does not report any errors
[ X] ./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-10-29T01:24:33Z

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

highfive · 2016-10-29T01:24:34Z

Heads up! This PR modifies the following files:

@fitzgen: 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
@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 · 2016-10-29T01:24:35Z

Warning

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

jdm

Good start! Let me know if any of my comments are unclear!

jdm · 2016-10-31T15:58:00Z

components/script/dom/htmlbuttonelement.rs

@@ -22,6 +22,7 @@ use dom::node::{Node, UnbindContext, document_from_node, window_from_node};
 use dom::nodelist::NodeList;
 use dom::validation::Validatable;
 use dom::validitystate::ValidityState;
+use dom::validitystate::ValidityStatus;


This can be combined with the previous line: use dom::validitystate::{ValidityState, ValidityStatus};

Thanks for pointing out this. Have made changes everywhere where we added two lines instead of one.

jdm · 2016-10-31T16:00:44Z

components/script/dom/validation.rs


-pub trait Validatable {}
+pub trait Validatable {
+    fn as_element_validatable(&self) -> &Element;


This method does not belong in this trait. We want a method on Element like as_maybe_activatable that returns an instance of the Validatable trait. This will allow us to invoke methods via that trait.

Method as_maybe_validatable is defined in element.rs file which returns an instance of Validatable trait. Attaching the screenshot for the same. Please verify once.

Yes, that looks right!

jdm · 2016-10-31T16:00:54Z

components/script/dom/validation.rs

@@ -1,5 +1,10 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+use dom::element::Element;
+use dom::validitystate::ValidityStatus;


jdm · 2016-10-31T16:01:48Z

components/script/dom/validitystate.rs

@@ -7,6 +7,7 @@ use dom::bindings::codegen::Bindings::ValidityStateBinding::ValidityStateMethods
 use dom::bindings::js::{JS, Root};


This file changed mode from 644 to 755, which is weird.

jdm · 2016-10-31T16:01:55Z

components/script/dom/validitystate.rs

@@ -7,6 +7,7 @@ use dom::bindings::codegen::Bindings::ValidityStateBinding::ValidityStateMethods
 use dom::bindings::js::{JS, Root};
 use dom::bindings::reflector::{Reflector, reflect_dom_object};
 use dom::element::Element;
+use dom::validation::Validatable;


jdm · 2016-10-31T16:05:49Z

components/script/dom/htmlinputelement.rs

+    fn as_element_validatable(&self) -> &Element {
+        self.upcast()
+    }
+    fn is_instance_validatable(&self) -> bool {


This method likely needs to implement the various cases where input elements are barred from constraint validation (click the bold text to see a list of places where the term is used).

jdm · 2016-10-31T16:08:41Z

components/script/dom/htmlformelement.rs

@@ -465,6 +466,15 @@ impl HTMLFormElement {
    /// https://html.spec.whatwg.org/multipage/#statically-validate-the-constraints
    fn static_validation(&self) -> Result<(), Vec<FormSubmittableElement>> {
        let node = self.upcast::<Node>();
+        for child in node.traverse_preorder() {
+            match child.downcast::<Element>() {


if let Some(el) = child.downcast::<Element>() { if el.disabled_state() { continue; } let validatable = match el.as_maybe_validatable() { Some(v) => v, None => continue, }; if !validatable.is_instance_validatable() { continue; } // invoke some other method that returns true if the form element validates, false otherwise. }

Thank you very much for this, got a better idea about the flow now from static validation to individual elements validation. Will soon post the changes.

jdm · 2016-10-31T16:09:42Z

components/script/dom/validation.rs

+pub trait Validatable {
+    fn as_element_validatable(&self) -> &Element;
+    fn is_instance_validatable(&self) -> bool;
+}


We will need a method added here that performs the appropriate validation checks, returns true if the element validates, and false otherwise.

Hello

As described in the initial steps for the project, we have some doubts. Is it possible for you to give your email id so that we can have a discussion there?

Regards
Bhavya

jdm

On the right track!

jdm · 2016-11-07T14:24:56Z

components/script/dom/htmlformelement.rs

+                let mut validate_flags: ValidationFlags = ValidationFlags::empty();
+                // Need match and insert ValidationFlags later
+                validatable.validate(validate_flags);
+                // Invoke validata here that returns true if the form element validates, false otherwise


This comment seems unnecessary now.

removed unnecessary comments as of now.

jdm · 2016-11-07T14:25:20Z

components/script/dom/htmlformelement.rs

+                }
+                let mut validate_flags: ValidationFlags = ValidationFlags::empty();
+                // Need match and insert ValidationFlags later
+                validatable.validate(validate_flags);


I would go ahead and call validatable.validate(ValidationFlags::empty()) for the time being.

change updated.

jdm · 2016-11-07T14:25:55Z

components/script/dom/htmlinputelement.rs

@@ -1162,6 +1159,11 @@ impl Validatable for HTMLInputElement {
        }
        true
    }
+    fn validate(&self, validate_flags: ValidationFlags) -> bool {
+        if validate_flags.is_empty() {}
+        // Need more flag check for different validation types later


I think the stub implementations should just consist of true, nothing else.

stub implementation updated

returning only true in here, leaves unused variable names warning. Any ideas or we can avoid that for time being?

Add a _ prefix to variables that don't get used.

jdm · 2016-11-07T14:26:51Z

components/script/dom/validitystate.rs

+        const STEP_MISMATCH    = 0b0010000000,
+        const BAD_INPUT        = 0b0100000000,
+        const CUSTOM_ERROR     = 0b1000000000,
+        const VALID            = 0b0000000000,


This flag doesn't make sense here.

Are you referring to VALID Flag?

jdm · 2016-11-07T14:28:05Z

components/script/dom/htmlinputelement.rs

+            InputType::InputCheckbox => {},
+            InputType::InputRadio => {},
+            InputType::InputPassword => {},
+        }


We should either return real values for these types or remove the match until we actually use it.

jdm · 2016-11-07T14:28:42Z

components/script/dom/htmlformelement.rs

+                if el.disabled_state() {
+                    continue;
+                }
+                let validatable = match el.as_maybe_validatable() {


as_maybe_validatable is still missing.

as_maybe_validatable method is added and defined in element.rs file. Kindly refer below link:

https://github.com/servo/servo/blob/master/components/script/dom/element.rs#L2296

Oh, in future linking to the code in question rather than providing screenshots will make that more clear :)

Sure. Thanks for the suggestion :)

jdm

Sorry, two more requests for changes. I missed some code before that we should integrate with, and there's no need to have all the duplicated stub method implementations right now.

jdm · 2016-11-08T22:37:14Z

components/script/dom/validation.rs

-pub trait Validatable {}
+pub trait Validatable {
+    fn is_instance_validatable(&self) -> bool;
+    fn validate(&self, validate_flags: ValidationFlags) -> bool;


Given that every implementation is a stub right now, I propose we do:

pub trait Validatable { fn is_instance_validatable(&self) -> bool { true } fn validate(&self, _validate_flags: ValidationFlags) -> bool { true } }

and remove the stub implementations from the other files:

impl Validatable for HTMLTextAreaElement {}

jdm · 2016-11-08T22:40:20Z

components/script/dom/htmlformelement.rs

+                // pass empty flags initially and set appropriate flags according to the type of element called
+                validatable.validate(ValidationFlags::empty());
+            }
+        }


Oh, I didn't notice previous that there is code below this that we should be integrating into instead. We'll want to return Some(el) if validate returns false, and None otherwise (since it's collecting elements that do not validate).

Hello Josh,

Trying to integrate the code, but every time it is giving some different kind of error on compilation.

Please correct me if my approach is wrong here:

This section of code

servo/components/script/dom/htmlformelement.rs

Line 469 in 234951e

let invalid_controls = node.traverse_preorder().filter_map(|field| {

will traverse through the page and then the code which we wrote for calling validate method will be written in that section. if validate method returns true then we return None, else return Some(el) which is the element for which validation got failed. And collect will collect those Some(el) {elements}.

Now, I tried writing the code but I am struggling with rust specific errors here.

Please guide me here.

Could you please post your code somewhere and the errors it outputs?

@Manishearth Thanks for helping out. Shall I give you the code file I wrote to integrate the two parts here or do you want me to email you that particular section?

Please push the code to a git branch somewhere, and link to it (as well as mentioning the error text) here. It's also fine if you just push it as a new commit on this same pull request.

hello

Pushing as a new commit with unsuccessful compilation, that will create issues. If you are ok, I can share the portion of code file here by attaching a document and also mention what kind of error I am getting. Actually error is related to rust syntax and I believe its nothing major.

Also, if you are ok sharing your email ID, I can email you the details.

Regards
Bhavya

Pushing as a new commit with unsuccessful compilation, that will create issues.

It's really fine to push non-compiling code to a PR; I do it all the time. And you can always push to a different branch

git checkout -b temp-branch (make the commit) git push origin temp-branch git checkout <name of original branch>

But if you feel you need to email it, myusername@gmail.com will work.

jdm · 2016-11-17T02:36:35Z

@Manishearth can you take a look at the most recent response in my absence as well?

Manishearth · 2016-11-17T02:40:47Z

will do

KiChjang · 2016-11-23T19:12:45Z

components/script/dom/htmltextareaelement.rs

-        if validate_flags.is_empty() {}
-        // Need more flag check for different validation types later
+    fn validate(&self, _validate_flags: ValidationFlags) -> bool {
+        // call stub methods defined in validityState.rs file here according to the flags set in validate_flags
        true


I think there's a miscommunication here, the idea is to just have impl Validatable for HTMLTextAreaElement {} instead of explicitly writing out the fn definitions out.

Updated, thanks for pointing it out.

jdm

Just a couple last stylistic nits and we can merge this!

jdm · 2016-11-23T19:12:15Z

components/script/dom/htmlformelement.rs

+                   let validatable = match el.as_maybe_validatable() {
+                   Some(v) => v,
+                   None => return { None }
+                };


nit: indentation here is weird.

let validatable = match el.as_maybe_validatable() { Some(v) => v, None => return None, };

jdm · 2016-11-23T19:12:39Z

components/script/dom/htmlformelement.rs

+                    if !validatable.is_instance_validatable() {
+                        None
+                    }
+                    else if validatable.validate(ValidationFlags::empty()) {


nit: we use } else if rather than splitting them on separate lines.

Code indentation done as per feedback. Thanks

jdm · 2016-11-23T19:14:18Z

components/script/dom/validation.rs

-pub trait Validatable {}
+pub trait Validatable {
+    fn is_instance_validatable(&self) -> bool { true}
+    fn validate(&self, _validate_flags: ValidationFlags) -> bool { true}


nit: add a space before } on these lines.

jdm · 2016-11-23T20:29:26Z

Looks good! Could you squash the commits into one?

…thods, added code to handle validations

jdm · 2016-11-25T14:49:40Z

@bors-servo: r+
Thanks!

bors-servo · 2016-11-25T14:49:41Z

📌 Commit 2a40877 has been approved by jdm

bors-servo · 2016-11-25T14:49:45Z

⌛ Testing commit 2a40877 with merge beec035...

html form validation initial steps with test html file  Added code for initial steps in html form validation. 1. Added methods for trait validatable 2. implemented stub methods for elements like HTMLInputElement, HTMLButtonElement, etc  - [X ] `./mach build -d` does not report any errors - [ X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13969)

bors-servo · 2016-11-25T15:40:46Z

💔 Test failed - mac-rel-wpt1

jdm · 2016-11-25T16:05:22Z

@bors-servo: retry

Intermittent crash of /_mozilla/mozilla/mozbrowser/mozbrowserlocationchange_event.html and /html/browsers/browsing-the-web/navigating-across-documents/javascript-url-return-value-handling.html and /html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/allow-scripts-flag-changing-2.html #14338

bors-servo · 2016-11-25T16:05:24Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt2, windows-dev are reusable. Rebuilding only mac-rel-wpt1...

bors-servo · 2016-11-25T16:56:34Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned jdm Oct 29, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 29, 2016

jdm requested changes Oct 31, 2016

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 31, 2016

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 5, 2016

jdm requested changes Nov 7, 2016

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 7, 2016

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 8, 2016

jdm requested changes Nov 8, 2016

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 8, 2016

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 23, 2016

KiChjang reviewed Nov 23, 2016

View reviewed changes

jdm requested changes Nov 23, 2016

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 23, 2016

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 23, 2016

jdm added the S-needs-squash Some (or all) of the commits in the PR should be combined. label Nov 23, 2016

jdm removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 23, 2016

html form validation initial steps with test html file, added stub me…

2a40877

…thods, added code to handle validations

bbansalWolfPack force-pushed the master branch from 9bff9f3 to 2a40877 Compare November 25, 2016 14:35

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 25, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Nov 25, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 25, 2016

bors-servo merged commit 2a40877 into servo:master Nov 25, 2016

bors-servo mentioned this pull request Nov 25, 2016

Implement "list of options" concept on HTMLSelectElement. #14085

Merged

		@@ -7,6 +7,7 @@ use dom::bindings::codegen::Bindings::ValidityStateBinding::ValidityStateMethods
		use dom::bindings::js::{JS, Root};

html form validation initial steps with test html file #13969

html form validation initial steps with test html file #13969

Conversation

bbansalWolfPack commented Oct 29, 2016 • edited by larsbergstrom

3. Added code to call methods from static validation inHTMLFormElement.rs file

highfive commented Oct 29, 2016

highfive commented Oct 29, 2016

highfive commented Oct 29, 2016

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbansalWolfPack Nov 2, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbansalWolfPack Nov 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbansalWolfPack Nov 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Nov 17, 2016

Manishearth commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Nov 23, 2016

jdm commented Nov 25, 2016

bors-servo commented Nov 25, 2016

bors-servo commented Nov 25, 2016

bors-servo commented Nov 25, 2016

jdm commented Nov 25, 2016

bors-servo commented Nov 25, 2016

bors-servo commented Nov 25, 2016

bbansalWolfPack commented Oct 29, 2016 •

edited by larsbergstrom

bbansalWolfPack Nov 2, 2016 •

edited

bbansalWolfPack Nov 8, 2016 •

edited

bbansalWolfPack Nov 16, 2016 •

edited