Implemented sanitize_value for time input #19379

SWW13 · 2017-11-26T14:27:35Z

Implemented value sanitization for <input type=time/>.
The value has the be valid time string (https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#valid-time-string) or set to empty string.

The following test results look expected to me, but I'm not sure:

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  │ FAIL [expected PASS] change state from time to text
  │   → assert_equals: input.value should be   foobar   after change of state expected "  foobar  " but got ""
  │ FAIL [expected PASS] change state from time to search
  │   → assert_equals: input.value should be   foobar   after change of state expected "  foobar  " but got ""
  │ FAIL [expected PASS] change state from time to tel
  │   → assert_equals: input.value should be   foobar   after change of state expected "  foobar  " but got ""
  │ FAIL [expected PASS] change state from time to url
  │   → assert_equals: input.value should be foobar after change of state expected "foobar" but got ""
  │ FAIL [expected PASS] change state from time to password
  │   → assert_equals: input.value should be   foobar   after change of state expected "  foobar  " but got ""
  │
  │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:53:15
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  │ FAIL [expected PASS] change state from color to time
  │   → assert_equals: input.value should be #000000 after change of state expected "#000000" but got ""
  │
  │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:55:15
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9

All other tests do now PASS instead of FAIL.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix part of #19172
There are tests for these changes
- All tests PASS

This change is

KiChjang

This is a good start! Could you move the validation methods to components/script/dom/bindings/str.rs and create a method called is_valid_time_string for DOMString instead?

KiChjang · 2017-11-27T06:06:08Z

components/script/dom/htmlinputelement.rs

+                let is_hour = |h: u8| h <= 23;
+                let is_minute = |m: u8| m <= 59;
+                let is_second = |s: u8| s <= 59;
+                let is_ms = |ms: u16| ms <= 999;


Why are these closures necessary? Comparing the characters directly should be sufficient.

They are not necessary but I started with an version without parse and they made the code easy to read IMHO. The compiler should inline them anyway.

KiChjang · 2017-11-27T06:06:08Z

components/script/dom/htmlinputelement.rs

+                    len >= 5 &&
+                        // HH:mm
+                        is_hour(content[0..2].parse().unwrap_or(99)) &&
+                        is_colon(&content[2..3]) &&


This looks like a longer version of &content[2].

&content[2] used to return a char, but that was removed b/c it doesn't work as expected when dealing with utf8. It would be fine for use as the only valid content is ASCII, but that's an edge case we're hitting here.
Maybe working with the underlying byte slice would work, but that's a dirty solution IMHO.

KiChjang · 2017-11-27T06:06:08Z

components/script/dom/htmlinputelement.rs

+
+                    len >= 5 &&
+                        // HH:mm
+                        is_hour(content[0..2].parse().unwrap_or(99)) &&


I don't think it's necessary to parse the integer here (and below). We're simply trying to validate the string, not generate a numerical representation of it.

I'll revert to my early try, but the string validation is a bit of a mess b/c of the utf8 encoding of the strings in rust.

KiChjang · 2017-11-27T09:50:06Z

It would also be great if you can link to the spec here and provide step annotations on your code, so that future readers of the code would not be too surprised by what's happening in the code.

KiChjang · 2017-11-27T16:53:37Z

@bors-servo try

bors-servo · 2017-11-27T16:53:41Z

⌛ Trying commit 938b3f6 with merge 52601a5...


        Auto merge of #19379 - SWW13:htmlinput_sanitize_time, r=<try>

Implemented sanitize_value for time input Implemented value sanitization for `<input type=time/>`. The value has the be valid time string (https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#valid-time-string) or set to empty string. --- The following test results look expected to me, but I'm not sure: ``` ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html: │ FAIL [expected PASS] change state from time to text │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ FAIL [expected PASS] change state from time to search │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ FAIL [expected PASS] change state from time to tel │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ FAIL [expected PASS] change state from time to url │ → assert_equals: input.value should be foobar after change of state expected "foobar" but got "" │ FAIL [expected PASS] change state from time to password │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:53:15 │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20 │ test@http://web-platform.test:8000/resources/testharness.js:511:9 └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9 ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html: │ FAIL [expected PASS] change state from color to time │ → assert_equals: input.value should be #000000 after change of state expected "#000000" but got "" │ │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:55:15 │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20 │ test@http://web-platform.test:8000/resources/testharness.js:511:9 └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9 ``` All other tests do now `PASS` instead of `FAIL`. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix *part of* #19172 - [x] There are tests for these changes - [ ] All tests `PASS`  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19379)

bors-servo · 2017-11-27T17:32:55Z

💔 Test failed - mac-rel-wpt3

jdm · 2017-11-27T17:57:22Z


  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/time-2.html:
  └ PASS [expected FAIL] Invalid value: fraction should have one, two or three ASCII digits. Value should be empty

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/time-2.html:
  └ PASS [expected FAIL] Invalid value: hour should have two ASCII digits. Value should be empty

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/time-2.html:
  └ PASS [expected FAIL] Invalid value: minutes should have two ASCII digits. Value should be empty

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/time-2.html:
  └ PASS [expected FAIL] Invalid value: seconds should have two ASCII digits. Value should be empty

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/time-2.html:
  └ PASS [expected FAIL] Invalid value: hour > 23. Value should be empty

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/time-2.html:
  └ PASS [expected FAIL] Invalid value: minute > 59. Value should be empty

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/time-2.html:
  └ PASS [expected FAIL] Invalid value: second > 59. Value should be empty

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from hidden to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from text to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from search to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from tel to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from url to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from email to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from password to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from datetime-local to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from date to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from month to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from week to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from time to hidden

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from time to datetime-local

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from time to date

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from time to month

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from time to week

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from time to number

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from time to submit

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from time to image

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from time to reset

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from time to button

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from number to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from range to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from checkbox to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from radio to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from submit to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from image to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from reset to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  └ PASS [expected FAIL] change state from button to time

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  │ FAIL [expected PASS] change state from time to text
  │   → assert_equals: input.value should be   foobar   after change of state expected "  foobar  " but got ""
  │ FAIL [expected PASS] change state from time to search
  │   → assert_equals: input.value should be   foobar   after change of state expected "  foobar  " but got ""
  │ FAIL [expected PASS] change state from time to tel
  │   → assert_equals: input.value should be   foobar   after change of state expected "  foobar  " but got ""
  │ FAIL [expected PASS] change state from time to url
  │   → assert_equals: input.value should be foobar after change of state expected "foobar" but got ""
  │ FAIL [expected PASS] change state from time to password
  │   → assert_equals: input.value should be   foobar   after change of state expected "  foobar  " but got ""
  │ 
  │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:53:15
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
  │ FAIL [expected PASS] change state from color to time
  │   → assert_equals: input.value should be #000000 after change of state expected "#000000" but got ""
  │ 
  │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:55:15
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/valueMode.html:
  └ PASS [expected FAIL] value IDL attribute of input type time without value attribute

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/valueMode.html:
  └ PASS [expected FAIL] value IDL attribute of input type time with value attribute

  ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/time.html:
  └ PASS [expected FAIL] set value on not time format value

SWW13 · 2017-11-29T22:21:53Z

Are these test results expected or not?

KiChjang · 2017-11-29T22:36:43Z

They were expected to fail, but after your changes, they now pass, which is a positive sign! All you need to do now is to update the test expectations and address my comments, and we can finally land this!

KiChjang · 2017-11-29T22:56:16Z

components/script/dom/bindings/str.rs

+            // Step 2 ":"
+            is_colon(bytes[2]) &&
+            // Step 3 "mm"
+            is_minute(&bytes[3..5]) &&


I can't shake the feeling that the code as it is written is brittle. At the very least though, I can safely say that this isn't as maintainable as it can be, since there's a lot of index calculations based on the positions of the characters.

What particularly makes me nervous is when we're indexing the arrays directly in such a manner, because array indexing can panic during runtime, and if we're not careful enough to validate our assumptions, we would encounter a bug and the thread would panic.

If we can instead iterate through the characters and returning early when we encounter a wrong character, that would be safer and I would feel a bit more relaxed since the language itself would guarantee some invariants that we hold in our assumptions.

Any good idea how to avoid indexing without bloating the code?

I propose using Iterator::fold and keeping a state machine with the accumulator.

SWW13 · 2017-11-30T09:26:50Z

New tests still work in progress.

KiChjang

This is looking really good! I only have a couple of small comments.

KiChjang · 2017-11-30T11:04:00Z

components/script/dom/bindings/str.rs

+        }
+        let next_state = |valid: bool, next: State| -> State { if valid { next } else { State::Error } };
+
+        let state = self.chars().fold(State::HourHigh, |state, c| {


One thing that we didn't check here is whether the character is ASCII. If at any point we run into a non-ASCII character, we would want to error out. In addition, I recall that iterating over chars is actually a bit slow, due to the fact that it needs to ensure UTF-8 well-formedness on each iteration.

So instead, maybe we could iterate over the underlying u8 data type by calling as_bytes, and importing AsciiExt so that we can use the .is_ascii() function to check whether the character is ASCII on each iteration.

I don't know if Unicode has other digits that are valid for is_digit, if not we are already check for ASCII implicit.
When using bytes (u8) there should be no need to check for ASCII as we already check for specific values (0-9, :, .) which implies there are only ASCII values.

Good point. I'll leave it up to you whether you want to use u8 or char in this PR.

KiChjang · 2017-11-30T11:04:00Z

tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html

            }
+
+            // type[j] sanitization
+            if ((types[i].type !== "color" && types[j].sanitizedValue) || types[j].sanitizedValue === "") {


The check for the color type is actually an incorrect change that I wrongly approved of. This should instead just check whether the sanitizedValue exists on the types object.

KiChjang · 2017-11-30T11:04:00Z

tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html

-              assert_equals(input.value, "  foo\rbar  ", "input.value should be '  foo\\rbar  ' after change of state");
+
+            // type[i] sanitization
+            if (types[i].sanitizedValue || types[i].sanitizedValue === "") {


The ordering on this test looks wrong. If I'm reading correctly, j represents the destination input type that this input element is changing to, so the value sanitization algorithm should run for the input type described by types[j] first.

I'm still not sure how to do this right. We have an input sanitization which may set the value to empty string, trims it somehow or replace it with a default value on source input type and then afterwards another that possibly do the same with that value on the destination input type.

So the part of the spec that specifies this behaviour is located here https://html.spec.whatwg.org/multipage/input.html#input-type-change, and in the code it's this line:

servo/components/script/dom/htmlinputelement.rs

Line 1016 in b7bb6ff

self.sanitize_value();

... which will get called whenever the value content attribute is modified.

I think we're getting near the expected test behavior, the only thing left is:

▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html: │ FAIL [expected PASS] change state from time to checkbox │ → assert_equals: input.value should be '' after change of state expected "" but got "on" │ FAIL [expected PASS] change state from time to radio │ → assert_equals: input.value should be '' after change of state expected "" but got "on"

BTW: There are many tests that pass who should fail (many date / number related input type changes).

Ick, this is because the default value for radio and checkbox is "on" instead of the empty string:

servo/components/script/dom/htmlinputelement.rs

Line 396 in b7bb6ff

.map_or(DOMString::from("on"),

So, perhaps instead of having a default boolean field on the test object, we should have a defaultValue string.

The more I try to find a nice solution the more I break :/ The additional defaultValue is ugly, but at least the tests work now.

tigercosmos · 2017-11-30T16:25:23Z

tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html

          input.type = types[i].type;
          if (types[i].type === "file") {
            assert_throws("INVALID_STATE_ERR", function() {
-              input.value = "  foo\rbar  ";
+              input.value = expected;


why set input.value = expected; here?
I think keep expected = " foo\rbar " is fine.

That's fine, it deduplicates the repeated construction of a new string.

Moving input.value = " foo\rbar " is good.
But input.value = expected; seems unnecessary.

I'm not following. This subtest in particular throws an exception when input.value is being assigned to. Are you saying that we should rather duplicate writing " foo\rbar " here?

NO, we have moved input.value = " foo\rbar " to the front of if section.
input.value is set, and not need to set input.value = expected;

You've lost me. The change that this patch has is var expected = " foo\rbar "; instead of input.value = " foo\rbar ";

tigercosmos · 2017-12-01T09:32:29Z

tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/type-change-state.html

+              }
+            }
+
+            assert_equals(input.value, expected, "input.value should be '" + expected + "' after change of state");


@KiChjang which mean this line is unnecessary to change to expected, just keep the origin " foo\rbar "

No, this is clearly not the case, because the expected value may change depending on whether the destination type has a sanitizedValue as defined above in this test file.

Keeping " foo\rbar " is wrong, we are not always expecting " foo\rbar ", sometimes it's "#000000", "foo\rbar" or "".

Yeah. Got your point.

tigercosmos · 2017-12-01T09:51:13Z

Seems #19379 and #19385 overlap at type-change-state.html and self.sanitize_value();
Maybe I wait for this one? @KiChjang

KiChjang · 2017-12-01T09:58:44Z

@tigercosmos Not necessary, whichever PR lands last would simply need to rebase against master.

KiChjang · 2017-12-04T11:59:22Z

@bors-servo r+

Thanks!

bors-servo · 2017-12-04T11:59:23Z

📌 Commit a999239 has been approved by KiChjang

bors-servo · 2017-12-04T12:48:59Z

⌛ Testing commit a999239 with merge 07192a1...


        Auto merge of #19379 - SWW13:htmlinput_sanitize_time, r=KiChjang

Implemented sanitize_value for time input Implemented value sanitization for `<input type=time/>`. The value has the be valid time string (https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#valid-time-string) or set to empty string. --- The following test results look expected to me, but I'm not sure: ``` ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html: │ FAIL [expected PASS] change state from time to text │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ FAIL [expected PASS] change state from time to search │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ FAIL [expected PASS] change state from time to tel │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ FAIL [expected PASS] change state from time to url │ → assert_equals: input.value should be foobar after change of state expected "foobar" but got "" │ FAIL [expected PASS] change state from time to password │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:53:15 │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20 │ test@http://web-platform.test:8000/resources/testharness.js:511:9 └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9 ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html: │ FAIL [expected PASS] change state from color to time │ → assert_equals: input.value should be #000000 after change of state expected "#000000" but got "" │ │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:55:15 │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20 │ test@http://web-platform.test:8000/resources/testharness.js:511:9 └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9 ``` All other tests do now `PASS` instead of `FAIL`. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix *part of* #19172 - [x] There are tests for these changes - [x] All tests `PASS`  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19379)

bors-servo · 2017-12-04T14:44:00Z

💔 Test failed - linux-rel-wpt

jdm · 2017-12-04T18:12:49Z

@bors-servo: retry

bors-servo · 2017-12-04T19:41:00Z

⌛ Testing commit a999239 with merge 8e7a079...


        Auto merge of #19379 - SWW13:htmlinput_sanitize_time, r=KiChjang

Implemented sanitize_value for time input Implemented value sanitization for `<input type=time/>`. The value has the be valid time string (https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#valid-time-string) or set to empty string. --- The following test results look expected to me, but I'm not sure: ``` ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html: │ FAIL [expected PASS] change state from time to text │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ FAIL [expected PASS] change state from time to search │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ FAIL [expected PASS] change state from time to tel │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ FAIL [expected PASS] change state from time to url │ → assert_equals: input.value should be foobar after change of state expected "foobar" but got "" │ FAIL [expected PASS] change state from time to password │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:53:15 │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20 │ test@http://web-platform.test:8000/resources/testharness.js:511:9 └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9 ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html: │ FAIL [expected PASS] change state from color to time │ → assert_equals: input.value should be #000000 after change of state expected "#000000" but got "" │ │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:55:15 │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20 │ test@http://web-platform.test:8000/resources/testharness.js:511:9 └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9 ``` All other tests do now `PASS` instead of `FAIL`. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix *part of* #19172 - [x] There are tests for these changes - [x] All tests `PASS`  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19379)

bors-servo · 2017-12-04T20:36:28Z

💔 Test failed - linux-rel-css

jdm · 2017-12-04T20:44:16Z

@bors-servo retry

#19486

bors-servo · 2017-12-04T21:45:01Z

⌛ Testing commit a999239 with merge eed3adc...


        Auto merge of #19379 - SWW13:htmlinput_sanitize_time, r=KiChjang

Implemented sanitize_value for time input Implemented value sanitization for `<input type=time/>`. The value has the be valid time string (https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#valid-time-string) or set to empty string. --- The following test results look expected to me, but I'm not sure: ``` ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html: │ FAIL [expected PASS] change state from time to text │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ FAIL [expected PASS] change state from time to search │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ FAIL [expected PASS] change state from time to tel │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ FAIL [expected PASS] change state from time to url │ → assert_equals: input.value should be foobar after change of state expected "foobar" but got "" │ FAIL [expected PASS] change state from time to password │ → assert_equals: input.value should be foobar after change of state expected " foobar " but got "" │ │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:53:15 │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20 │ test@http://web-platform.test:8000/resources/testharness.js:511:9 └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9 ▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html: │ FAIL [expected PASS] change state from color to time │ → assert_equals: input.value should be #000000 after change of state expected "#000000" but got "" │ │ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:55:15 │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20 │ test@http://web-platform.test:8000/resources/testharness.js:511:9 └ @http://web-platform.test:8000/html/semantics/forms/the-input-element/type-change-state.html:37:9 ``` All other tests do now `PASS` instead of `FAIL`. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix *part of* #19172 - [x] There are tests for these changes - [x] All tests `PASS`  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19379)

bors-servo · 2017-12-04T23:19:41Z

☀️ 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: KiChjang
Pushing eed3adc to master...


        Added time input sanitization:

- Implemented is_valid_time_string for DOMString. - Use is_valid_time_string for sanitize_value with time input. - Improved input type change test Upstreamed from servo/servo#19379 [ci skip]

KiChjang requested changes Nov 27, 2017

View changes

KiChjang self-assigned this Nov 27, 2017

KiChjang mentioned this pull request Nov 27, 2017

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

Closed

7 of 8 tasks complete

SWW13 force-pushed the SWW13:htmlinput_sanitize_time branch from f50fa79 to 8448363 Nov 27, 2017

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

SWW13 force-pushed the SWW13:htmlinput_sanitize_time branch 2 times, most recently from f1105c5 to 938b3f6 Nov 27, 2017

KiChjang added S-needs-code-changes and removed S-awaiting-review labels Nov 27, 2017

highfive added the S-tests-failed label Nov 27, 2017

tigercosmos mentioned this pull request Nov 29, 2017

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

Merged

3 of 3 tasks complete

KiChjang reviewed Nov 29, 2017

View changes

SWW13 force-pushed the SWW13:htmlinput_sanitize_time branch from 938b3f6 to de954f9 Nov 30, 2017

highfive added S-awaiting-review and removed S-tests-failed S-needs-code-changes labels Nov 30, 2017

KiChjang requested changes Nov 30, 2017

View changes

tigercosmos reviewed Nov 30, 2017

View changes

tigercosmos reviewed Dec 1, 2017

View changes

SWW13 force-pushed the SWW13:htmlinput_sanitize_time branch from 0a83969 to a999239 Dec 4, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 4, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 4, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 4, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 4, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 4, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 4, 2017

bors-servo merged commit a999239 into servo:master Dec 4, 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 Dec 4, 2017

servo-wpt-sync mentioned this pull request Jan 4, 2018

Added time input sanitization: web-platform-tests/wpt#8911

Merged

servo / servo

Join GitHub today

Implemented sanitize_value for time input #19379

Implemented sanitize_value for time input #19379

Conversation

SWW13 commented Nov 26, 2017 • edited

KiChjang 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.

This comment has been minimized.

KiChjang commented Nov 27, 2017

KiChjang commented Nov 27, 2017

bors-servo commented Nov 27, 2017

bors-servo commented Nov 27, 2017

jdm commented Nov 27, 2017

SWW13 commented Nov 29, 2017

KiChjang commented Nov 29, 2017

This comment has been minimized.

KiChjang Nov 29, 2017 • edited

This comment has been minimized.

This comment has been minimized.

SWW13 commented Nov 30, 2017

KiChjang 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.

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 Nov 30, 2017 • edited

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.

tigercosmos commented Dec 1, 2017

KiChjang commented Dec 1, 2017

KiChjang commented Dec 4, 2017

bors-servo commented Dec 4, 2017

bors-servo commented Dec 4, 2017

bors-servo commented Dec 4, 2017

jdm commented Dec 4, 2017

bors-servo commented Dec 4, 2017

bors-servo commented Dec 4, 2017

jdm commented Dec 4, 2017

bors-servo commented Dec 4, 2017

bors-servo commented Dec 4, 2017

SWW13 commented Nov 26, 2017 •

edited

KiChjang Nov 29, 2017 •

edited

KiChjang Nov 30, 2017 •

edited