Subsequent step for implementing HTML form validation #14566

xilaili · 2016-12-13T08:51:46Z

Form validation project. NCSU M1653 student project.

./mach build -d does not report any errors
./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


        Subsequent step for implementing HTML form validation

highfive · 2016-12-13T08:51:51Z

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

highfive · 2016-12-13T08:51:52Z

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 · 2016-12-13T08:51:53Z

Warning

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

Manishearth · 2016-12-13T18:11:31Z

components/script/dom/htmlformelement.rs

+            Err(err) => {
+                match err[0] {
+                    FormSubmittableElement::InputElement(ref i) =>
+                        println!("First element didn't pass: {}",


Please don't print to stdout.

@jdm Thought on how this should be reported?

Stdout is all we've got at the moment. We should be calling Focus() on the element, however.

Manishearth · 2016-12-13T18:12:18Z

r? @jdm

jdm

This is a good start! Please let us know if you intend to continue working on this or if we should find someone else to take over this work, now that the term is over!

jdm · 2016-12-15T16:52:36Z

components/script/dom/htmlformelement.rs

+            Err(err) => {
+                match err[0] {
+                    FormSubmittableElement::InputElement(ref i) =>
+                        println!("First element didn't pass: {}",


Stdout is all we've got at the moment. We should be calling Focus() on the element, however.

jdm · 2016-12-15T16:52:36Z

components/script/dom/htmlformelement.rs

+                        i.upcast::<Element>().get_string_attribute(&local_name!("name"))),
+                    FormSubmittableElement::ObjectElement(ref i) =>
+                        println!("First element didn't pass: {}",
+                        i.upcast::<Element>().get_string_attribute(&local_name!("name"))),


As discussed in the evaluation, we should find a way to reduce code duplication here. The reporting belongs in the code beneath here, where it should replace the TODO.

jdm · 2016-12-15T16:52:36Z

components/script/dom/htmlinputelement.rs

+        let window = window_from_node(self);
+        let el = self.upcast::<Element>();
+        let vs = ValidityState::new(&window, el);
+        if vs.ValueMissing() {


I know the comment above says to call methods in validityState, and I apologize for missing that in the previous group's work. However, the model in this PR is backwards - validation code for a particular element type belongs in this validate method, and the methods in ValidityState should be calling this method and passing appropriate ValidationFlags. This method should be checking the state of validate_flags and performing the appropriate validation checks that are requested.

jdm · 2016-12-15T16:52:36Z

components/script/dom/validitystate.rs

+                    "email" => Regex::new(r"^\w+@[a-zA-Z_]+?\.[a-zA-Z]{2,3}$").unwrap(),
+                    "url" => Regex::new(r"^[a-zA-Z0-9\-\.]+\.(com|org|net|mil|edu|COM|ORG|NET|MIL|EDU)$").unwrap(),
+                    "date" => Regex::new(r"^\d{1,2}\/\d{1,2}\/\d{4}$").unwrap(),
+                    "tel" => Regex::new(r"^\d{3}-\d{2}-\d{4}$").unwrap(),


Unfortunately these regexes need to be either much more complex, or much less strict. I lean towards much less strict - URLs can check if there's a period present; emails can check if there's an @; telephone numbers should not contain letters.

jdm · 2016-12-15T16:52:36Z

components/script/dom/validitystate.rs

+            if l > Max.parse::<i32>().unwrap() {
+                println!("{} is too long for {}, maxlength is {} characters.", v, n, Max);
+                return true;
+            }


HTMLInputElement already contains the maxlength information in a field, so this check will be more straightforward in HTMLInputElement::validate.

xilaili · 2016-12-15T17:16:49Z

Hi, Josh, Thanks for you comments. I will discuss these with my teammate and let you know what we can do later. Regards, Xilai

…

On Thu, Dec 15, 2016 at 11:52 AM, Josh Matthews ***@***.***> wrote: ***@***.**** requested changes on this pull request. This is a good start! Please let us know if you intend to continue working on this or if we should find someone else to take over this work, now that the term is over! ------------------------------ In components/script/dom/htmlformelement.rs <#14566 (review)>: > @@ -455,7 +455,27 @@ impl HTMLFormElement { // Step 1-3 let _unhandled_invalid_controls = match self.static_validation() { Ok(()) => return Ok(()), - Err(err) => err + //Err(err) => err, + Err(err) => { + match err[0] { + FormSubmittableElement::InputElement(ref i) => + println!("First element didn't pass: {}", Stdout is all we've got at the moment. We should be calling Focus() on the element, however. ------------------------------ In components/script/dom/htmlformelement.rs <#14566 (review)>: > + match err[0] { + FormSubmittableElement::InputElement(ref i) => + println!("First element didn't pass: {}", + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), + FormSubmittableElement::ButtonElement(ref i) => + println!("First element didn't pass: {}", + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), + FormSubmittableElement::SelectElement(ref i) => + println!("First element didn't pass: {}", + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), + FormSubmittableElement::TextAreaElement(ref i) => + println!("First elment didn't pass: {}", + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), + FormSubmittableElement::ObjectElement(ref i) => + println!("First element didn't pass: {}", + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), As discussed in the evaluation, we should find a way to reduce code duplication here. The reporting belongs in the code beneath here, where it should replace the TODO. ------------------------------ In components/script/dom/htmlinputelement.rs <#14566 (review)>: > @@ -1139,6 +1141,21 @@ impl Validatable for HTMLInputElement { } fn validate(&self, _validate_flags: ValidationFlags) -> bool { // call stub methods defined in validityState.rs file here according to the flags set in validate_flags + let window = window_from_node(self); + let el = self.upcast::<Element>(); + let vs = ValidityState::new(&window, el); + if vs.ValueMissing() { I know the comment above says to call methods in validityState, and I apologize for missing that in the previous group's work. However, the model in this PR is backwards - validation code for a particular element type belongs in this validate method, and the methods in ValidityState should be calling this method and passing appropriate ValidationFlags. This method should be checking the state of validate_flags and performing the appropriate validation checks that are requested. ------------------------------ In components/script/dom/validitystate.rs <#14566 (review)>: > false } // https://html.spec.whatwg.org/multipage/#dom-validitystate-typemismatch fn TypeMismatch(&self) -> bool { + if self.element.has_attribute(&local_name!("type")) { + let content_type = self.element.get_string_attribute(&local_name!("type")); + let content_value = self.element.get_string_attribute(&local_name!("value")); + let re = match content_type.to_string().as_ref() { + "email" => ***@***.***A-Z_]+?\.[a-zA-Z]{2,3}$").unwrap(), + "url" => Regex::new(r"^[a-zA-Z0-9\-\.]+\.(com|org|net|mil|edu|COM|ORG|NET|MIL|EDU)$").unwrap(), + "date" => Regex::new(r"^\d{1,2}\/\d{1,2}\/\d{4}$").unwrap(), + "tel" => Regex::new(r"^\d{3}-\d{2}-\d{4}$").unwrap(), Unfortunately these regexes need to be either much more complex, or much less strict. I lean towards much less strict - URLs can check if there's a period present; emails can check if there's an @; telephone numbers should not contain letters. ------------------------------ In components/script/dom/validitystate.rs <#14566 (review)>: > @@ -84,11 +111,33 @@ impl ValidityStateMethods for ValidityState { // https://html.spec.whatwg.org/multipage/#dom-validitystate-toolong fn TooLong(&self) -> bool { + if self.element.has_attribute(&local_name!("maxlength")) { + let Max = self.element.get_string_attribute(&local_name!("maxlength")).to_string(); + let v = self.element.get_string_attribute(&local_name!("value")).to_string(); + let n = self.element.get_string_attribute(&local_name!("name")); + let l = v.len().to_i32().unwrap(); + if l > Max.parse::<i32>().unwrap() { + println!("{} is too long for {}, maxlength is {} characters.", v, n, Max); + return true; + } HTMLInputElement already contains the maxlength information in a field, so this check will be more straightforward in HTMLInputElement::validate. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14566 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANV-klrMwZp8RLXVMpGbCsMuJffx4k33ks5rIXBagaJpZM4LLefK> .

-- Xilai Li Research Assistant Department of Electrical and Computer Engineering North Carolina State University Raleigh, NC, USA Email: xli47@ncsu.edu Tel(mobile): 919- <919-389-1113>666-7899

xilaili · 2016-12-19T19:15:16Z

Hi, Josh, We intend to continue working on this project. We will improve our modification according to your comments. Xilai

…

On Thu, Dec 15, 2016 at 11:52 AM, Josh Matthews ***@***.***> wrote: ***@***.**** requested changes on this pull request. This is a good start! Please let us know if you intend to continue working on this or if we should find someone else to take over this work, now that the term is over! ------------------------------ In components/script/dom/htmlformelement.rs <#14566 (review)>: > @@ -455,7 +455,27 @@ impl HTMLFormElement { // Step 1-3 let _unhandled_invalid_controls = match self.static_validation() { Ok(()) => return Ok(()), - Err(err) => err + //Err(err) => err, + Err(err) => { + match err[0] { + FormSubmittableElement::InputElement(ref i) => + println!("First element didn't pass: {}", Stdout is all we've got at the moment. We should be calling Focus() on the element, however. ------------------------------ In components/script/dom/htmlformelement.rs <#14566 (review)>: > + match err[0] { + FormSubmittableElement::InputElement(ref i) => + println!("First element didn't pass: {}", + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), + FormSubmittableElement::ButtonElement(ref i) => + println!("First element didn't pass: {}", + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), + FormSubmittableElement::SelectElement(ref i) => + println!("First element didn't pass: {}", + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), + FormSubmittableElement::TextAreaElement(ref i) => + println!("First elment didn't pass: {}", + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), + FormSubmittableElement::ObjectElement(ref i) => + println!("First element didn't pass: {}", + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), As discussed in the evaluation, we should find a way to reduce code duplication here. The reporting belongs in the code beneath here, where it should replace the TODO. ------------------------------ In components/script/dom/htmlinputelement.rs <#14566 (review)>: > @@ -1139,6 +1141,21 @@ impl Validatable for HTMLInputElement { } fn validate(&self, _validate_flags: ValidationFlags) -> bool { // call stub methods defined in validityState.rs file here according to the flags set in validate_flags + let window = window_from_node(self); + let el = self.upcast::<Element>(); + let vs = ValidityState::new(&window, el); + if vs.ValueMissing() { I know the comment above says to call methods in validityState, and I apologize for missing that in the previous group's work. However, the model in this PR is backwards - validation code for a particular element type belongs in this validate method, and the methods in ValidityState should be calling this method and passing appropriate ValidationFlags. This method should be checking the state of validate_flags and performing the appropriate validation checks that are requested. ------------------------------ In components/script/dom/validitystate.rs <#14566 (review)>: > false } // https://html.spec.whatwg.org/multipage/#dom-validitystate-typemismatch fn TypeMismatch(&self) -> bool { + if self.element.has_attribute(&local_name!("type")) { + let content_type = self.element.get_string_attribute(&local_name!("type")); + let content_value = self.element.get_string_attribute(&local_name!("value")); + let re = match content_type.to_string().as_ref() { + "email" => ***@***.***A-Z_]+?\.[a-zA-Z]{2,3}$").unwrap(), + "url" => Regex::new(r"^[a-zA-Z0-9\-\.]+\.(com|org|net|mil|edu|COM|ORG|NET|MIL|EDU)$").unwrap(), + "date" => Regex::new(r"^\d{1,2}\/\d{1,2}\/\d{4}$").unwrap(), + "tel" => Regex::new(r"^\d{3}-\d{2}-\d{4}$").unwrap(), Unfortunately these regexes need to be either much more complex, or much less strict. I lean towards much less strict - URLs can check if there's a period present; emails can check if there's an @; telephone numbers should not contain letters. ------------------------------ In components/script/dom/validitystate.rs <#14566 (review)>: > @@ -84,11 +111,33 @@ impl ValidityStateMethods for ValidityState { // https://html.spec.whatwg.org/multipage/#dom-validitystate-toolong fn TooLong(&self) -> bool { + if self.element.has_attribute(&local_name!("maxlength")) { + let Max = self.element.get_string_attribute(&local_name!("maxlength")).to_string(); + let v = self.element.get_string_attribute(&local_name!("value")).to_string(); + let n = self.element.get_string_attribute(&local_name!("name")); + let l = v.len().to_i32().unwrap(); + if l > Max.parse::<i32>().unwrap() { + println!("{} is too long for {}, maxlength is {} characters.", v, n, Max); + return true; + } HTMLInputElement already contains the maxlength information in a field, so this check will be more straightforward in HTMLInputElement::validate. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14566 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANV-klrMwZp8RLXVMpGbCsMuJffx4k33ks5rIXBagaJpZM4LLefK> .

-- Xilai Li Research Assistant Department of Electrical and Computer Engineering North Carolina State University Raleigh, NC, USA Email: xli47@ncsu.edu Tel(mobile): 919- <919-389-1113>666-7899

jdm · 2016-12-19T19:17:00Z

Wonderful! I look forward to seeing the changes!

xilaili · 2016-12-19T21:01:12Z

Currently many of us are having vacation, we might probably resume after the break. Thank you, Zhongcan P.S. I'm not able to access the comments on the pull request, so Josh could you add me to that, if convenient. If it's not very convenient its OK, we just will continue using Xilai's account. My id is: canhsiao.

…

On Mon, Dec 19, 2016 at 2:15 PM, Xilai Li ***@***.***> wrote: Hi, Josh, We intend to continue working on this project. We will improve our modification according to your comments. Xilai On Thu, Dec 15, 2016 at 11:52 AM, Josh Matthews ***@***.***> wrote: > ***@***.**** requested changes on this pull request. > > This is a good start! Please let us know if you intend to continue > working on this or if we should find someone else to take over this work, > now that the term is over! > ------------------------------ > > In components/script/dom/htmlformelement.rs > <#14566 (review)>: > > > @@ -455,7 +455,27 @@ impl HTMLFormElement { > // Step 1-3 > let _unhandled_invalid_controls = match self.static_validation() { > Ok(()) => return Ok(()), > - Err(err) => err > + //Err(err) => err, > + Err(err) => { > + match err[0] { > + FormSubmittableElement::InputElement(ref i) => > + println!("First element didn't pass: {}", > > Stdout is all we've got at the moment. We should be calling Focus() on > the element, however. > ------------------------------ > > In components/script/dom/htmlformelement.rs > <#14566 (review)>: > > > + match err[0] { > + FormSubmittableElement::InputElement(ref i) => > + println!("First element didn't pass: {}", > + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), > + FormSubmittableElement::ButtonElement(ref i) => > + println!("First element didn't pass: {}", > + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), > + FormSubmittableElement::SelectElement(ref i) => > + println!("First element didn't pass: {}", > + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), > + FormSubmittableElement::TextAreaElement(ref i) => > + println!("First elment didn't pass: {}", > + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), > + FormSubmittableElement::ObjectElement(ref i) => > + println!("First element didn't pass: {}", > + i.upcast::<Element>().get_string_attribute(&local_name!("name"))), > > As discussed in the evaluation, we should find a way to reduce code > duplication here. The reporting belongs in the code beneath here, where it > should replace the TODO. > ------------------------------ > > In components/script/dom/htmlinputelement.rs > <#14566 (review)>: > > > @@ -1139,6 +1141,21 @@ impl Validatable for HTMLInputElement { > } > fn validate(&self, _validate_flags: ValidationFlags) -> bool { > // call stub methods defined in validityState.rs file here according to the flags set in validate_flags > + let window = window_from_node(self); > + let el = self.upcast::<Element>(); > + let vs = ValidityState::new(&window, el); > + if vs.ValueMissing() { > > I know the comment above says to call methods in validityState, and I > apologize for missing that in the previous group's work. However, the model > in this PR is backwards - validation code for a particular element type > belongs in this validate method, and the methods in ValidityState should > be calling this method and passing appropriate ValidationFlags. This method > should be checking the state of validate_flags and performing the > appropriate validation checks that are requested. > ------------------------------ > > In components/script/dom/validitystate.rs > <#14566 (review)>: > > > false > } > > // https://html.spec.whatwg.org/multipage/#dom-validitystate-typemismatch > fn TypeMismatch(&self) -> bool { > + if self.element.has_attribute(&local_name!("type")) { > + let content_type = self.element.get_string_attribute(&local_name!("type")); > + let content_value = self.element.get_string_attribute(&local_name!("value")); > + let re = match content_type.to_string().as_ref() { > + "email" => ***@***.***A-Z_]+?\.[a-zA-Z]{2,3}$").unwrap(), > + "url" => Regex::new(r"^[a-zA-Z0-9\-\.]+\.(com|org|net|mil|edu|COM|ORG|NET|MIL|EDU)$").unwrap(), > + "date" => Regex::new(r"^\d{1,2}\/\d{1,2}\/\d{4}$").unwrap(), > + "tel" => Regex::new(r"^\d{3}-\d{2}-\d{4}$").unwrap(), > > Unfortunately these regexes need to be either much more complex, or much > less strict. I lean towards much less strict - URLs can check if there's a > period present; emails can check if there's an @; telephone numbers should > not contain letters. > ------------------------------ > > In components/script/dom/validitystate.rs > <#14566 (review)>: > > > @@ -84,11 +111,33 @@ impl ValidityStateMethods for ValidityState { > > // https://html.spec.whatwg.org/multipage/#dom-validitystate-toolong > fn TooLong(&self) -> bool { > + if self.element.has_attribute(&local_name!("maxlength")) { > + let Max = self.element.get_string_attribute(&local_name!("maxlength")).to_string(); > + let v = self.element.get_string_attribute(&local_name!("value")).to_string(); > + let n = self.element.get_string_attribute(&local_name!("name")); > + let l = v.len().to_i32().unwrap(); > + if l > Max.parse::<i32>().unwrap() { > + println!("{} is too long for {}, maxlength is {} characters.", v, n, Max); > + return true; > + } > > HTMLInputElement already contains the maxlength information in a field, > so this check will be more straightforward in HTMLInputElement::validate. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#14566 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ANV-klrMwZp8RLXVMpGbCsMuJffx4k33ks5rIXBagaJpZM4LLefK> > . > -- Xilai Li Research Assistant Department of Electrical and Computer Engineering North Carolina State University Raleigh, NC, USA Email: ***@***.*** Tel(mobile): 919- <919-389-1113>666-7899

jdm · 2016-12-19T22:37:24Z

That's fine! I'm not sure what you mean about accessing the comments, though. They should appear in the github UI for anybody to view them and/or respond.

KiChjang · 2016-12-19T22:56:41Z

I suppose they're using the email interface instead of the GitHub UI? If so, the URL link to the pull request is #14566. It's possible that they do not have a GitHub account, barring them from commenting on this PR using the GitHub UI.

bors-servo · 2017-01-11T23:34:10Z

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

jdm · 2017-01-27T19:10:39Z

@xilaili Are you still planning to finish this work?

xilaili · 2017-01-27T19:55:57Z

Sorry, Josh, We probably cannot continue the work. Xilai

On Fri, Jan 27, 2017 at 2:11 PM Josh Matthews ***@***.***> wrote: @xilaili <https://github.com/xilaili> Are you still planning to finish this work? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14566 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANV-kjy-zbcAj8WlQbofdkLtBQrXY72cks5rWkE8gaJpZM4LLefK> .

-- null

jdm · 2017-01-27T20:03:14Z

Ok! We'll keep tracking the efforts to finish this work in #11444.

Subsequent step for implementing HTML form validation

Loading status checks…

aafbcc2

highfive assigned Manishearth Dec 13, 2016

highfive added the S-awaiting-review label Dec 13, 2016

Manishearth reviewed Dec 13, 2016

View changes

highfive assigned jdm and unassigned Manishearth Dec 13, 2016

jdm requested changes Dec 15, 2016

View changes

jdm added S-awaiting-answer S-needs-code-changes and removed S-awaiting-review labels Dec 15, 2016

jdm removed the S-awaiting-answer label Dec 19, 2016

highfive added the S-needs-rebase label Jan 11, 2017

jdm mentioned this pull request Jan 27, 2017

Implement HTML5 form validation #11444

Closed

jdm closed this Jan 27, 2017

servo / servo

Subsequent step for implementing HTML form validation #14566

Subsequent step for implementing HTML form validation #14566

xilaili commented Dec 13, 2016 •

edited by larsbergstrom

highfive commented Dec 13, 2016

highfive commented Dec 13, 2016

highfive commented Dec 13, 2016

This comment has been minimized.

This comment has been minimized.

Manishearth commented Dec 13, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

xilaili commented Dec 15, 2016

xilaili commented Dec 19, 2016

jdm commented Dec 19, 2016

xilaili commented Dec 19, 2016

jdm commented Dec 19, 2016

KiChjang commented Dec 19, 2016

bors-servo commented Jan 11, 2017

jdm commented Jan 27, 2017

xilaili commented Jan 27, 2017

jdm commented Jan 27, 2017

servo / servo

Join GitHub today

Subsequent step for implementing HTML form validation #14566

Subsequent step for implementing HTML form validation #14566

Conversation

xilaili commented Dec 13, 2016 • edited by larsbergstrom

highfive commented Dec 13, 2016

highfive commented Dec 13, 2016

highfive commented Dec 13, 2016

This comment has been minimized.

Manishearth Dec 13, 2016

This comment has been minimized.

jdm Dec 15, 2016

Manishearth commented Dec 13, 2016

jdm left a comment

This comment has been minimized.

jdm Dec 15, 2016

This comment has been minimized.

jdm Dec 15, 2016

This comment has been minimized.

jdm Dec 15, 2016

This comment has been minimized.

jdm Dec 15, 2016

This comment has been minimized.

jdm Dec 15, 2016

xilaili commented Dec 15, 2016

xilaili commented Dec 19, 2016

jdm commented Dec 19, 2016

xilaili commented Dec 19, 2016

jdm commented Dec 19, 2016

KiChjang commented Dec 19, 2016

bors-servo commented Jan 11, 2017

jdm commented Jan 27, 2017

xilaili commented Jan 27, 2017

jdm commented Jan 27, 2017

xilaili commented Dec 13, 2016 •

edited by larsbergstrom