Implement value sanitization on HTMLInputElement #18262

KiChjang · 2017-08-27T19:26:05Z

https://html.spec.whatwg.org/multipage/input.html#value-sanitization-algorithm

This change is

highfive · 2017-08-27T19:26:10Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/htmlinputelement.rs
@KiChjang: components/script/dom/htmlinputelement.rs

KiChjang · 2017-09-29T06:07:46Z

@bors-servo try

bors-servo · 2017-09-29T06:07:51Z

⌛ Trying commit a81a95c with merge 6a5654f...


        Auto merge of #18262 - KiChjang:value-sanitization, r=<try>

Implement value sanitization on HTMLInputElement https://html.spec.whatwg.org/multipage/input.html#value-sanitization-algorithm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18262)

bors-servo · 2017-09-29T06:43:53Z

💔 Test failed - mac-rel-wpt1

KiChjang · 2017-10-08T18:47:48Z

Ok, so the timeouts are actually moving us in the correct direction, the problem is simply that we have a buggy selection algorithm, causing select events to not fire when they're supposed to.

KiChjang · 2017-10-09T23:53:21Z

r? @nox

nox · 2017-10-19T08:56:08Z

components/script/dom/htmlinputelement.rs

@@ -176,7 +176,7 @@ impl HTMLInputElement {
            .map_or_else(|| atom!(""), |a| a.value().as_atom().to_owned())
    }

-    // https://html.spec.whatwg.org/multipage/#input-type-attr-summary
+    // https://html.spec.whatwg.org/multipage/#common-input-element-apis


Because the old link doesn't point to somewhere that can explain value modes.

https://html.spec.whatwg.org/multipage/input.html#dom-input-value is a better link, then.

nox · 2017-10-19T08:56:08Z

components/script/dom/htmlinputelement.rs

@@ -410,8 +410,19 @@ impl HTMLInputElementMethods for HTMLInputElement {
    fn SetValue(&self, value: DOMString) -> ErrorResult {
        match self.value_mode() {
            ValueMode::Value => {
+                // Step 1.
+                let old_value = self.textinput.borrow().get_content();


Merge these 2 steps together so that the older value doesn't get cloned.

We need to clone the old value so that we may compare it in step 5.

No you don't, merge these two steps and use mem::replace and you don't need the clone.

nox · 2017-10-19T08:56:08Z

components/script/dom/htmlinputelement.rs

+                // Step 4.
+                self.sanitize_value();
+                // Step 5.
+                if self.textinput.borrow().get_content() != old_value {


This clones the new value for no reason.

Which method call is a clone?

get_content.

Then I'm not sure which other method gets the content of the text input without using get_content.

nox · 2017-10-19T08:56:08Z

components/script/dom/htmlinputelement.rs

+    fn sanitize_value(&self) {
+        let new_value = match self.type_() {
+            atom!("text") | atom!("search") | atom!("tel") | atom!("password") => {
+                DOMString::from(self.textinput.borrow().get_content()


Make a helper function for https://infra.spec.whatwg.org/#strip-newlines.

nox · 2017-10-19T08:56:08Z

components/script/dom/htmlinputelement.rs

+                                                       .collect::<String>())
+            }
+            atom!("url") => {
+                DOMString::from(self.textinput.borrow().get_content()


Make a helper function for https://infra.spec.whatwg.org/#strip-leading-and-trailing-ascii-whitespace.

nox · 2017-10-19T08:56:08Z

components/script/dom/htmlinputelement.rs

+                                                       .chars()
+                                                       .filter(|&c| c != '\r' && c != '\n')
+                                                       .collect::<String>()
+                                                       .trim_matches(char::is_whitespace))


This is wrong, that should trim only ASCII whitespace.

Your suggestion doesn't match up with the spec: https://html.spec.whatwg.org/multipage/input.html#url-state-(type=url).

What?

The value sanitization algorithm is as follows: Strip newlines from the value, then strip leading and trailing ASCII whitespace from the value.

nox · 2017-10-19T08:56:08Z

components/script/dom/htmlinputelement.rs

+    fn sanitize_value(&self) {
+        let new_value = match self.type_() {
+            atom!("text") | atom!("search") | atom!("tel") | atom!("password") => {
+                DOMString::from(self.textinput.borrow().get_content()


This clones the current value for no reason.

Which method call is a clone?

nox · 2017-10-19T08:56:08Z

components/script/dom/htmlinputelement.rs

+                                                       .collect::<String>())
+            }
+            atom!("url") => {
+                DOMString::from(self.textinput.borrow().get_content()


This clones the current value for no reason.

Which method call is a clone?

KiChjang · 2017-10-21T12:36:47Z

@bors-servo try

bors-servo · 2017-10-21T12:36:51Z

⌛ Trying commit 1cafcc4 with merge beeea1f...


        Auto merge of #18262 - KiChjang:value-sanitization, r=<try>

Implement value sanitization on HTMLInputElement https://html.spec.whatwg.org/multipage/input.html#value-sanitization-algorithm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18262)

bors-servo · 2017-10-21T13:25:34Z

💔 Test failed - linux-rel-wpt

KiChjang · 2017-11-08T21:58:10Z

  ▶ TIMEOUT [expected OK] /html/semantics/forms/textfieldselection/selection-start-end.html
  │ 
  │ VMware, Inc.
  │ softpipe
  └ 3.3 (Core Profile) Mesa 17.3.0-devel

So the test harness does actually timeout on this test... will investigate.

bors-servo · 2017-11-08T23:26:11Z

⌛ Testing commit cf1bd6b with merge 8038197...


        Auto merge of #18262 - KiChjang:value-sanitization, r=nox

Implement value sanitization on HTMLInputElement https://html.spec.whatwg.org/multipage/input.html#value-sanitization-algorithm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18262)

bors-servo · 2017-11-09T00:02:17Z

💔 Test failed - linux-rel-wpt

KiChjang · 2017-11-10T00:07:25Z

Ok, so it looks like the test SHOULD time out with these changes, because our setSelectionStart and setSelectionEnd implementation isn't exactly up to spec, since they both don't fire a select event at the input element when they're supposed to.

KiChjang · 2017-11-10T00:16:27Z

Filed #19171 for the problem I mentioned above.


        Implement value sanitization on HTMLInputElement

KiChjang · 2017-11-10T00:35:03Z

@bors-servo r=nox

bors-servo · 2017-11-10T00:35:04Z

📌 Commit 8203605 has been approved by nox

bors-servo · 2017-11-10T00:35:08Z

⌛ Testing commit 8203605 with merge 338e2ae...


        Auto merge of #18262 - KiChjang:value-sanitization, r=nox

Implement value sanitization on HTMLInputElement https://html.spec.whatwg.org/multipage/input.html#value-sanitization-algorithm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18262)

bors-servo · 2017-11-10T01:49:03Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: nox
Pushing 338e2ae to master...


        Implement value sanitization on HTMLInputElement

Upstreamed from servo/servo#18262 [ci skip]

highfive assigned emilio Aug 27, 2017

highfive added the S-awaiting-review label Aug 27, 2017

KiChjang force-pushed the KiChjang:value-sanitization branch 2 times, most recently from a1e26c5 to a81a95c Aug 27, 2017

highfive added the S-tests-failed label Sep 29, 2017

KiChjang force-pushed the KiChjang:value-sanitization branch from a81a95c to a188073 Oct 8, 2017

highfive removed the S-tests-failed label Oct 8, 2017

KiChjang force-pushed the KiChjang:value-sanitization branch from a188073 to 835c244 Oct 8, 2017

highfive assigned nox and unassigned emilio Oct 9, 2017

nox requested changes Oct 19, 2017

View changes

nox added S-needs-code-changes and removed S-awaiting-review labels Oct 19, 2017

KiChjang force-pushed the KiChjang:value-sanitization branch from 835c244 to 1cafcc4 Oct 21, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 21, 2017

highfive added the S-tests-failed label Oct 21, 2017

KiChjang force-pushed the KiChjang:value-sanitization branch from 1cafcc4 to 2b3e94f Oct 22, 2017

highfive removed the S-tests-failed label Oct 22, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 8, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 9, 2017

KiChjang force-pushed the KiChjang:value-sanitization branch from cf1bd6b to d53a729 Nov 9, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Nov 9, 2017

Implement value sanitization on HTMLInputElement

Loading status checks…

8203605

KiChjang force-pushed the KiChjang:value-sanitization branch from d53a729 to 8203605 Nov 10, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 10, 2017

bors-servo merged commit 8203605 into servo:master Nov 10, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 10, 2017

bors-servo mentioned this pull request Nov 10, 2017

[WIP] Finish implementation of HTML5 form validation. #17657

Closed

3 of 11 tasks complete

KiChjang mentioned this pull request Nov 10, 2017

[meta] Implement value sanitizations for different input modes #19172

Closed

7 of 8 tasks complete

jdm added a commit to web-platform-tests/wpt that referenced this pull request Nov 15, 2017

Implement value sanitization on HTMLInputElement

40ee9a4

Upstreamed from servo/servo#18262 [ci skip]

servo-wpt-sync mentioned this pull request Nov 15, 2017

Implement value sanitization on HTMLInputElement web-platform-tests/wpt#8266

Merged

tigercosmos mentioned this pull request Nov 26, 2017

implement "Date type inputs", "Month type inputs" #19385

Merged

3 of 3 tasks complete

KiChjang deleted the KiChjang:value-sanitization branch Mar 31, 2018

servo / servo

Join GitHub today

Implement value sanitization on HTMLInputElement #18262

Implement value sanitization on HTMLInputElement #18262

Conversation

KiChjang commented Aug 27, 2017 • edited by SimonSapin

highfive commented Aug 27, 2017

KiChjang commented Sep 29, 2017

bors-servo commented Sep 29, 2017

bors-servo commented Sep 29, 2017

KiChjang commented Oct 8, 2017

KiChjang commented Oct 9, 2017

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.

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.

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Oct 21, 2017

bors-servo commented Oct 21, 2017

bors-servo commented Oct 21, 2017

KiChjang commented Nov 8, 2017

bors-servo commented Nov 8, 2017

bors-servo commented Nov 9, 2017

KiChjang commented Nov 10, 2017

KiChjang commented Nov 10, 2017

KiChjang commented Nov 10, 2017

bors-servo commented Nov 10, 2017

bors-servo commented Nov 10, 2017

bors-servo commented Nov 10, 2017

KiChjang commented Aug 27, 2017 •

edited by SimonSapin