Implement minlength attribute for text inputs #13315

Phrohdoh · 2016-09-19T04:46:59Z

This is not ready to be merged:
I need help writing tests as I am not familiar with the methods used in the maxlength tests (introduced in tests/unit/script/textinput.rs).

I also need to write the minlength-manual test.

Closes #13313
This depends on #13314 (and includes the commit from it so will need to be rebased once that patch lands).

I am just looking for a quick review to make sure I am on the right path.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13313

There ~~are~~ will be tests for these changes

This change is

highfive · 2016-09-19T04:47:05Z

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

highfive · 2016-09-19T04:47:06Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/htmltextareaelement.rs, components/script/dom/webidls/HTMLInputElement.webidl, components/script/dom/webidls/HTMLTextAreaElement.webidl, components/script/dom/htmlinputelement.rs, components/script/textinput.rs

emilio · 2016-09-19T13:08:12Z

tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/minlength.html

+
+    <div style="display: none">
+      <input id="none" />
+      <input id="negative" type="-5" />


I think here you want minlength=-5 instead of type?

I thought the same but just blindly followed the maxlength example.

I've fixed this locally.

cbrewster

Thanks for doing this!
Overall this looks good to me, just one question.

cbrewster · 2016-09-19T19:17:05Z

components/script/dom/htmlinputelement.rs

@@ -479,6 +487,12 @@ impl HTMLInputElementMethods for HTMLInputElement {
    // https://html.spec.whatwg.org/multipage/#dom-input-maxlength
    make_limited_int_setter!(SetMaxLength, "maxlength", DEFAULT_MAX_LENGTH);

+    // https://html.spec.whatwg.org/multipage/#dom-input-minlength // This doesn't exist?


The spec link exists for me.

cbrewster · 2016-09-19T19:17:05Z

tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/minlength.html

+            let minGtMax = document.getElementById("min-greater-than-max");
+            assert_equals(minGtMax.minLength, 0);
+            assert_equals(minGtMax.minLength, minGtMax.maxLength);
+          }, "A minLength value greater than maxLength results in minLength = maxLength = 0");


Where is this logic happening? I might just be missing it.

I have not implemented this yet because TextInput::replace_selection(DOMString) is quite complex and I haven't worked out what it will need to look like (after doing this the rest should come easily).

Ok, we can leave the test expectation as failing if you want to open an issue for it.

cbrewster · 2016-09-19T19:18:00Z

Also it looks like there are some unit tests that need to be updated after these changes.

cbrewster · 2016-09-19T22:15:13Z

Ahh also, we should actually enforce the min length and add unit tests. (Sorry for missing that, its a Monday).

relevant code: components/script/textinput.rs

Phrohdoh · 2016-09-19T23:12:02Z

Indeed tests are desired, but

I need help writing tests as I am not familiar with the methods used in the maxlength tests

Regarding enforcing minlength @jdm and I spoke about that on IRC today and I believe the consensus was to only enforce this on form validation.

If you have a greater-than-zero minLength editing the text becomes unnecessarily complex once the current content's length is equal to minLength.

Is form validation not built into Servo at all at this point?

cbrewster · 2016-09-20T00:48:01Z

@Phrohdoh fair enough. I believe this is the latest efforts on form validation #10169. So I believe you won't need to worry about that right now.

If you squash this, fix those comments, and update the unit tests (so they compile), we should be good to go!

jdm · 2016-09-20T03:50:22Z

Form validation is being tracked in #11444 and should be implemented by a group of students at NCSU this term.

Phrohdoh · 2016-09-20T14:24:44Z

Excellent, I will try to get the unit tests working again this afternoon!

Phrohdoh · 2016-09-20T19:56:42Z

@ConnorGBrewster The unit tests have been fixed.

jdm · 2016-09-20T20:17:26Z

tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/minlength.html

+          function() {
+            document.getElementById("assign-non-numeric").minLength = "not-a-number";
+            assert_equals(document.getElementById("assign-non-numeric").minLength, 0);
+          }, "Assigning non-numeric to minlength sets minlength to 0");


As mentioned on IRC, this behaviour cannot be exposed to the web since it's not part of the specification.

Sorry, misread the code. Ignore this comment.

cbrewster · 2016-09-20T21:51:52Z

Thank you!
@bors-servo r+

bors-servo · 2016-09-20T21:51:53Z

📌 Commit 54e83bd has been approved by ConnorGBrewster

bors-servo · 2016-09-20T21:51:56Z

⌛ Testing commit 54e83bd with merge b0da5d5...


        Auto merge of #13315 - Phrohdoh:textinput-minlength-13313, r=ConnorGB…

…rewster Implement minlength attribute for text inputs  **This is not ready to be merged: I need help writing tests as I am not familiar with the methods used in the `maxlength` tests (introduced in tests/unit/script/textinput.rs).** I also need to write the `minlength-manual` test. Closes #13313 This depends on #13314 (and includes the commit from it so will need to be rebased once that patch lands). I am just looking for a quick review to make sure I am on the right path. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13313  - [X] There ~~are~~ *will be* tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13315)

bors-servo · 2016-09-20T22:42:09Z

💔 Test failed - linux-rel

cbrewster · 2016-09-20T22:45:40Z

Looks like some test expectations will need updated

Tests with unexpected results:
  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: attribute minLength

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: document.createElement("input") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("text") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("hidden") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("search") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("tel") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("url") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("email") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("password") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("date") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("month") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("week") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("time") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("datetime-local") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("number") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("range") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("color") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("checkbox") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("radio") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("file") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("submit") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("image") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("reset") must inherit property "minLength" with the proper type (22)

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  └ PASS [expected FAIL] HTMLInputElement interface: createInput("button") must inherit property "minLength" with the proper type (22)

KiChjang · 2016-09-20T23:29:36Z

@bors-servo r=ConnorGBrewster

bors-servo · 2016-09-20T23:29:37Z

📌 Commit 90cd009 has been approved by ConnorGBrewster

bors-servo · 2016-09-21T01:28:45Z

⌛ Testing commit 90cd009 with merge 7320195...


        Auto merge of #13315 - Phrohdoh:textinput-minlength-13313, r=ConnorGB…

…rewster Implement minlength attribute for text inputs  **This is not ready to be merged: I need help writing tests as I am not familiar with the methods used in the `maxlength` tests (introduced in tests/unit/script/textinput.rs).** I also need to write the `minlength-manual` test. Closes #13313 This depends on #13314 (and includes the commit from it so will need to be rebased once that patch lands). I am just looking for a quick review to make sure I am on the right path. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13313  - [X] There ~~are~~ *will be* tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13315)

bors-servo · 2016-09-21T02:40:00Z

💔 Test failed - linux-rel

Phrohdoh · 2016-09-21T02:41:45Z

Well.. that is an odd failure.

~~I thought I had fixed this.~~

Looks like I need to update the proto.
If someone could point me in the right direction that would be much appreciated.

Ran 7569 tests finished in 1025.0 seconds.
  • 7568 ran as expected. 1678 tests skipped.
  • 1 tests had unexpected subtest results

Tests with unexpected results:
  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  │ FAIL [expected PASS] HTMLTextAreaElement interface: document.createElement("textarea") must inherit property "minLength" with the proper type (8)
  │   → assert_inherits: property "minLength" not found in prototype chain
  │ 
  │ IdlInterface.prototype.test_interface_of/<@http://web-platform.test:8000/resources/idlharness.js:1549:25
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1401:20
  │ test@http://web-platform.test:8000/resources/testharness.js:501:9
  │ IdlInterface.prototype.test_interface_of@http://web-platform.test:8000/resources/idlharness.js:1543:13
  │ IdlInterface.prototype.test_object@http://web-platform.test:8000/resources/idlharness.js:1455:9
  │ IdlArray.prototype.test/<@http://web-platform.test:8000/resources/idlharness.js:403:17
  │ IdlArray.prototype.test@http://web-platform.test:8000/resources/idlharness.js:401:13
  └ window.onload@http://web-platform.test:8000/html/dom/interfaces.html:208:3

  ▶ Unexpected subtest result in /html/dom/interfaces.html:
  │ FAIL [expected PASS] HTMLTextAreaElement interface: attribute minLength
  │   → assert_true: The prototype object must have a property "minLength" expected true got false
  │ 
  │ IdlInterface.prototype.test_member_attribute/<@http://web-platform.test:8000/resources/idlharness.js:1158:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1401:20
  │ test@http://web-platform.test:8000/resources/testharness.js:501:9
  │ IdlInterface.prototype.test_member_attribute@http://web-platform.test:8000/resources/idlharness.js:1113:5
  │ IdlInterface.prototype.test_members@http://web-platform.test:8000/resources/idlharness.js:1395:17
  │ IdlInterface.prototype.test@http://web-platform.test:8000/resources/idlharness.js:740:5
  │ IdlArray.prototype.test@http://web-platform.test:8000/resources/idlharness.js:398:9
  └ window.onload@http://web-platform.test:8000/html/dom/interfaces.html:208:3

jdm · 2016-09-21T12:22:24Z

You only changed HTMLInputElement; those failure relate to HTMLTextAreaElement.

Phrohdoh · 2016-09-21T12:38:26Z

Indeed but these are not the same errors.
These errors fail but were expected to pass while the previous ones passed but expected to fail.
I do not know where the prototype definitions live so that I may modify them.


        Implement minlength for text inputs

jdm · 2016-09-21T13:04:46Z

@bors-servo: r=ConnorGBrewster

bors-servo · 2016-09-21T13:04:47Z

📌 Commit 2cb5adf has been approved by ConnorGBrewster


        Auto merge of #13315 - Phrohdoh:textinput-minlength-13313, r=ConnorGB…

…rewster Implement minlength attribute for text inputs  **This is not ready to be merged: I need help writing tests as I am not familiar with the methods used in the `maxlength` tests (introduced in tests/unit/script/textinput.rs).** I also need to write the `minlength-manual` test. Closes #13313 This depends on #13314 (and includes the commit from it so will need to be rebased once that patch lands). I am just looking for a quick review to make sure I am on the right path. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13313  - [X] There ~~are~~ *will be* tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13315)

bors-servo · 2016-09-21T13:49:15Z

⌛ Testing commit 2cb5adf with merge c0bcd6f...

bors-servo · 2016-09-21T15:09:56Z

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

Phrohdoh · 2016-09-21T16:12:50Z

Awesome! 🎉

highfive assigned cbrewster Sep 19, 2016

highfive added the S-awaiting-review label Sep 19, 2016

emilio reviewed Sep 19, 2016

View changes

emilio added the C-looking for assistance label Sep 19, 2016

cbrewster reviewed Sep 19, 2016

View changes

cbrewster added the S-needs-squash label Sep 20, 2016

jdm removed C-looking for assistance S-awaiting-review labels Sep 20, 2016

highfive added the S-awaiting-review label Sep 20, 2016

jdm reviewed Sep 20, 2016

View changes

cbrewster removed the S-needs-squash label Sep 20, 2016

highfive removed the S-awaiting-review label Sep 20, 2016

highfive added the S-awaiting-merge label Sep 20, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 20, 2016

KiChjang removed the S-tests-failed label Sep 20, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 20, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 20, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 21, 2016

Implement minlength for text inputs

Loading status checks…

2cb5adf

highfive added S-awaiting-review and removed S-tests-failed labels Sep 21, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 21, 2016

bors-servo merged commit 2cb5adf into servo:master Sep 21, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 21, 2016

bors-servo mentioned this pull request Sep 21, 2016

Use fetch infrastructure to load external scripts #12472

Merged

Phrohdoh deleted the Phrohdoh:textinput-minlength-13313 branch Sep 21, 2016

servo / servo

Implement minlength attribute for text inputs #13315

Implement minlength attribute for text inputs #13315

Phrohdoh commented Sep 19, 2016 •

edited

highfive commented Sep 19, 2016

highfive commented Sep 19, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cbrewster left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cbrewster commented Sep 19, 2016

cbrewster commented Sep 19, 2016 •

edited

Phrohdoh commented Sep 19, 2016

cbrewster commented Sep 20, 2016 •

edited

jdm commented Sep 20, 2016

Phrohdoh commented Sep 20, 2016

Phrohdoh commented Sep 20, 2016

This comment has been minimized.

This comment has been minimized.

cbrewster commented Sep 20, 2016

bors-servo commented Sep 20, 2016

bors-servo commented Sep 20, 2016

bors-servo commented Sep 20, 2016

cbrewster commented Sep 20, 2016

KiChjang commented Sep 20, 2016

bors-servo commented Sep 20, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

Phrohdoh commented Sep 21, 2016 •

edited

jdm commented Sep 21, 2016

Phrohdoh commented Sep 21, 2016 •

edited

jdm commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

Phrohdoh commented Sep 21, 2016 •

edited

servo / servo

Join GitHub today

Implement minlength attribute for text inputs #13315

Implement minlength attribute for text inputs #13315

Conversation

Phrohdoh commented Sep 19, 2016 • edited

highfive commented Sep 19, 2016

highfive commented Sep 19, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cbrewster left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Phrohdoh Sep 19, 2016 • edited

This comment has been minimized.

cbrewster commented Sep 19, 2016

cbrewster commented Sep 19, 2016 • edited

Phrohdoh commented Sep 19, 2016

cbrewster commented Sep 20, 2016 • edited

jdm commented Sep 20, 2016

Phrohdoh commented Sep 20, 2016

Phrohdoh commented Sep 20, 2016

This comment has been minimized.

This comment has been minimized.

cbrewster commented Sep 20, 2016

bors-servo commented Sep 20, 2016

bors-servo commented Sep 20, 2016

bors-servo commented Sep 20, 2016

cbrewster commented Sep 20, 2016

KiChjang commented Sep 20, 2016

bors-servo commented Sep 20, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

Phrohdoh commented Sep 21, 2016 • edited

jdm commented Sep 21, 2016

Phrohdoh commented Sep 21, 2016 • edited

jdm commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

bors-servo commented Sep 21, 2016

Phrohdoh commented Sep 21, 2016 • edited

Phrohdoh commented Sep 19, 2016 •

edited

Phrohdoh Sep 19, 2016 •

edited

cbrewster commented Sep 19, 2016 •

edited

cbrewster commented Sep 20, 2016 •

edited

Phrohdoh commented Sep 21, 2016 •

edited

Phrohdoh commented Sep 21, 2016 •

edited

Phrohdoh commented Sep 21, 2016 •

edited