Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upimplement "Date type inputs", "Month type inputs" #19385
Conversation
highfive
commented
Nov 26, 2017
|
Heads up! This PR modifies the following files:
|
|
Sorry to do this to you, but I've come to believe that this code should instead be implemented on Also, I see that |
| let month = value_vec[1]; | ||
| let day = value_vec[2]; | ||
|
|
||
| // setp 1 |
This comment has been minimized.
This comment has been minimized.
| return true; | ||
| } | ||
|
|
||
| /// Valid month string should be "YYYY-MM" |
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 27, 2017
Member
This does not look correct, since step 1 clearly says that the year string can be 4 digits or more.
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 27, 2017
Author
Collaborator
um.. how about use
Valid month string should be "YYYY-MM", YYYY must be four or more
| pub fn is_valid_month_string(value: &str) -> bool { | ||
| // step 2 | ||
| let value_vec: Vec<&str> = value.split('-').collect(); | ||
| if !(value_vec.len() == 2) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| let year = value_vec[0]; | ||
| let month = value_vec[1]; | ||
|
|
||
| // setp 1 |
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.
CYBAI
Nov 27, 2017
Collaborator
I think @KiChjang means there's a typo and have the comment with first character capitalized.
| // setp 1 | ||
| if !(year.len() >= 4 && | ||
| year.chars().all(|c| c.is_digit(10)) && | ||
| year.parse::<i32>().unwrap() > 0) { |
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 27, 2017
Member
I don't think it's necessary to parse it in order to see whether the resulting i32 is larger than 0.
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 27, 2017
Author
Collaborator
yes, we only need to prevent the case "0000".
In this case, not parse to int is fine.
| } | ||
|
|
||
| // step 3 | ||
| let month_int = month.parse::<i32>().unwrap(); |
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 27, 2017
Member
This can panic if the month string is not convertible to i32. I also think it's not necessary to parse the month string.
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 27, 2017
Author
Collaborator
I have checked if the string is 0~9 previously.
change to int will let the code clear.
We also can use, such as
month[0] == '1' && month[1]== '0'
but I think this is hard code
| let day = value_vec[2]; | ||
|
|
||
| // setp 1 | ||
| let month_string = format!("{}-{}", year, month); |
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 27, 2017
Member
This is pretty awkward, we've just split the string on -, but here we're heap-allocating a String with a - to parse a month string.
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 27, 2017
Author
Collaborator
YYYY-MM-DD
I want to get YYYY-MM. I don't know how to write it prettier.
|
|
||
| // setp 1 | ||
| let month_string = format!("{}-{}", year, month); | ||
| if !is_valid_month_string(&*month_string) { |
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 27, 2017
Member
If it helps to split the responsibilities between is_valid_month_string and is_valid_date_string more cleanly, we can instead implement parse a month component, and have that algorithm return a Result<(u32, u32), ParseError>.
This comment has been minimized.
This comment has been minimized.
| } | ||
| match month_int { | ||
| 1|3|5|7|8|10|12 => { | ||
| if day_int > 31 { |
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 27, 2017
Member
Let's instead extract the number of days in month of year into its own function, and have it return a u32 representing maxday.
@KiChjang Change to there is OK, but you said in #19172
How do you think? I can put where you think better. Update |
|
Yeah sorry, forget about what I said in the original issue, that doesn't look like it's a good place to put the validation logic. |
|
@KiChjang Where should I put the unit test at? |
|
I think a WPT test would suffice for this; in fact, the tests under |
|
@KiChjang I got a problem
It will still return the origin value, like
|
|
Can you push your code to this branch? It's hard to debug without any context. |
| } | ||
| atom!("month") => { | ||
| let mut textinput = self.textinput.borrow_mut(); | ||
| if ! textinput.single_line_content_mut().is_valid_month_string() { |
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 29, 2017
Author
Collaborator
@KiChjang
even I set
atom!("month") => {
let mut textinput = self.textinput.borrow_mut();
textinput.set_content("".into());
}
It will still return the origin value, like
▶ Unexpected subtest result in /html/semantics/forms/the-input-element/month.html:
│ FAIL [expected PASS] Month should be > 0: If the value of the element is not a valid month string, then set it to the empty string instead.>
│ → assert_equals: expected "" but got "2013-00"
│
│ @http://web-platform.test:8000/html/semantics/forms/the-input-element/month.html:64:9
│ 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/month.html:63:7
▶ Unexpected subtest result in /html/semantics/forms/the-input-element/month.html:
│ FAIL [expected PASS] Month should be <= 13: If the value of the element is not a valid month string, then set it to the empty string instead.
│ → assert_equals: expected "" but got "2013-13"
│
│ @http://web-platform.test:8000/html/semantics/forms/the-input-element/month.html:60:9
│ 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/month.html:59:7
This comment has been minimized.
This comment has been minimized.
CYBAI
Nov 29, 2017
•
Collaborator
I just saw there's a space between ! and textinput.
Not sure if it will be an issue in Rust?
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 29, 2017
Member
So, my guess here is that the value content attribute is not properly sanitized when it is first parsed. In other words, bind_to_tree needs to call sanitize_value.
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 29, 2017
Member
@CYBAI Although this is not proper style, there's no problem with adding a whitespace in-between ! and the expression that it is trying to negate, see https://play.rust-lang.org/?gist=d27e1e8f115cb01c70bf97b8c5312c60&version=stable.
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 29, 2017
•
Author
Collaborator
@KiChjang I just follow color method(same code textinput.set_content("".into());), and seems color implement is correct (It passes tests). So..?
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 29, 2017
Author
Collaborator
@KiChjang You are right. Initialization works.
But I am wondering why color is fine lol
This comment has been minimized.
This comment has been minimized.
| @@ -21,6 +21,9 @@ | |||
| [[month\] stepUp method support on input 'month' element] | |||
| expected: FAIL | |||
|
|
|||
| [[month\] The value must be a value that is a valid global date and time string] | |||
This comment has been minimized.
This comment has been minimized.
| @@ -1,8 +0,0 @@ | |||
| [url.html] | |||
This comment has been minimized.
This comment has been minimized.
| @@ -1,11 +0,0 @@ | |||
| [telephone.html] | |||
This comment has been minimized.
This comment has been minimized.
| @@ -1110,7 +1122,8 @@ impl VirtualMethods for HTMLInputElement { | |||
| if let Some(ref s) = self.super_type() { | |||
| s.bind_to_tree(tree_in_doc); | |||
| } | |||
|
|
|||
This comment has been minimized.
This comment has been minimized.
|
I also believe these two are wrong test.
|
|
The change in previous PR #19330 is wrong |
|
r? @KiChjang |
| let day = value_vec[2]; | ||
|
|
||
| // Step 1 | ||
| let month_string = format!("{}-{}", year, month); |
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 29, 2017
Member
This is still not OK. format! heap-allocates a String, and I believe we don't need any heap allocation at all.
| } | ||
| let year_int = year.parse::<i32>().unwrap(); | ||
| let month_int = month.parse::<i32>().unwrap(); | ||
| let day_int = day.parse::<i32>().unwrap(); |
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 29, 2017
Member
Sorry if I wasn't clear on my last request, but what I mean is that check_valid_month_string should instead be parse_month_string, which returns a type of Result<(u32, u32), ()> that contains a 2-tuple of a year-month pair for the success variant, and just an empty error for the other variant. That way, you don't need to parse here.
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 30, 2017
Author
Collaborator
@KiChjang
There is still year need to parse:
let year_int = year.parse::<i32>().unwrap();
If we parse the three(year, month, day) together would let the code more clear?
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 30, 2017
Member
That's a code duplication that I would like to avoid, since you should have already parsed the month and year in the parse_month_string algorithm.
| } else if day_int > self.max_day_in_month(year_int, month_int) { | ||
| return false; | ||
| } | ||
| return true; |
This comment has been minimized.
This comment has been minimized.
|
|
||
| /// Valid month string should be "YYYY-MM" | ||
| /// https://html.spec.whatwg.org/multipage/#valid-month-string | ||
| pub fn is_valid_month_string(&mut self) -> bool { |
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 29, 2017
Member
What's the purpose for this function? It looks to me as an unnecessary indirection.
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 30, 2017
Author
Collaborator
@KiChjang
because the origin usage is:
DOMString.is_valid_month_string()
The logic in it can be use in is_valid_date_string()
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 30, 2017
Member
So perhaps my question is better phrased as "why is is_valid_month_string and check_valid_month_string" not the same function?
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 30, 2017
•
Author
Collaborator
@KiChjang
when we call is_valid_month_string will deal the self.0 string.
But when we call is_valid_date_string and want to use logic in is_valid_month_string, we cannot directly call is_valid_month_string in is_valid_date_string, because at that time self.0 might be 1990-12-20 and is_valid_month_string will return false.
That's why I make the logic out to check_valid_month_string(value: &str). We can give value string for two case usage.
Maybe there is a better way to do this.
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 30, 2017
•
Member
I think we need to be careful when reading the spec here - according to my reading, the spec does not fail on parsing a month component if the string contains a date at the end; it simply does not parse the rest of the string, and returns year and month to the caller of this algorithm.
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 30, 2017
•
Author
Collaborator
@KiChjang
According to the spec:
A string is a valid month string representing a year year and month month if it consists of the following components in the given order:
- Four or more ASCII digits, representing year, where year > 0
- A U+002D HYPHEN-MINUS character (-)
- Two ASCII digits, representing the month month, in the range 1 ≤ month ≤ 12
Which means only "YYYY-MM" is valid to is_valid_month_string function
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 30, 2017
•
Member
Note that it says "consists of the following components" - it's missing the only keyword. In other words, date strings are also valid month strings.
Furthermore, I believe we have agreed to use the parse a month component algorithm instead of the validation algorithm, so this part of the spec shouldn't be relevant to your implementation.
| } | ||
| }, | ||
| _ => { | ||
| panic!("Month number must be in range 1~12.") |
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 29, 2017
Member
Instead of panicking here, we can choose to return a Result<u32, ()> instead, and return Err(()) here.
| <input id="invalid_value_with_two_digits_year" type="month" value="13-06" /> | ||
| <input id="invalid_value_is_set" type="month" /> | ||
| <input id="step_attribute_is_invalid_value" type="month" value="2013-06" step="invalid_step_value" /> | ||
| <input id="invalid_month_too_high" type="month" value="2013-13" /> | ||
| <input id="invalid_month_too_low" type="month" value="2013-00" /> | ||
| <input id="invalid_year_all_zerro" type="month" value="0000-10" /> |
This comment has been minimized.
This comment has been minimized.
| <input id="valid" type="month" value="2011-11" min="2011-01" max="2011-12" /> | ||
| <input id="invalid_value" type="month" value="invalid-month" min="2011-01" max="2011-12"/> | ||
| <input id="value_can_be_empty_string" type="month" value="2013-06" /> | ||
| <input id="value_can_be_empty_string" type="month" value="" /> |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 30, 2017
Author
Collaborator
The purpose in this test, is testing value to be empty string. But it set value for it.
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 30, 2017
Member
It sets the value as an empty string in the test below:
document.getElementById("value_can_be_empty_string").value = "";
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 30, 2017
Author
Collaborator
Why set empty below not set directly? I think it is not intuitive
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 30, 2017
Member
You may contact the author of this test and ask them this question. However I can venture a guess: the value of the value content attribute is the empty string at initialization, and setting it directly via the IDL setter ensures that we are testing specifically for the value sanitization algorithm.
| </div> | ||
|
|
||
| <div id="log"></div> | ||
|
|
||
| <script> | ||
| test(function() { | ||
| assert_equals(document.getElementById("valid_value_1").value, "20133-12") | ||
| }, "year can be more than four number"); |
This comment has been minimized.
This comment has been minimized.
|
|
||
| test(function() { | ||
| assert_equals(document.getElementById("valid_value_3").value, "0003-01") | ||
| }, "year must be four or more number even if it is zero"); |
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 29, 2017
Member
"year can contain prefixes of zero, as long as there are at least four digits"
|
|
||
| test(function() { | ||
| assert_equals(document.getElementById("invalid_month_with_one_number").value, ""); | ||
| }, "Month should be two number: If the value of the element is not a valid month string, then set it to the empty string instead.>"); |
This comment has been minimized.
This comment has been minimized.
| @@ -49,12 +49,12 @@ | |||
| } else { | |||
| input.value = " foo\rbar "; | |||
| input.type = types[j].type; // change state | |||
| if (types[i].type !== "color" && (types[j].sanitizedValue || types[j].sanitizedValue === "")) { | |||
| assert_equals(input.value, types[j].sanitizedValue, "input.value should be " + types[j].sanitizedValue + " after change of state"); | |||
| if (types[j].sanitizedValue || types[j].sanitizedValue === "") { | |||
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 30, 2017
Member
I haven't looked closely enough on this change yet; will need some time before I can determine whether this is the actual change we want.
This comment has been minimized.
This comment has been minimized.
tigercosmos
Nov 30, 2017
Author
Collaborator
This is in #19330
https://github.com/servo/servo/pull/19330/files#diff-a04db4d0de89c245a2878d38e99d51bdL52
will cause
▶ Unexpected subtest result in /html/semantics/forms/the-input-element/type-change-state.html:
│ FAIL [expected PASS] change state from color to date
│ → assert_equals: input.value should be "#000000" after change of state expected "#000000" but got ""
│ FAIL [expected PASS] change state from color to month
│ → assert_equals: input.value should be "#000000" after change of state expected "#000000" but got ""
But actually it should pass
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 30, 2017
Member
You are correct. This part of the spec makes it clear that the value sanitization algorithm should be run when there is a change of state.
This comment has been minimized.
This comment has been minimized.
KiChjang
Dec 1, 2017
Member
Uh, sorry, that was not the full story. Because the value sanitization algorithm is ran when the input.value is set, color type inputs will sanitize their values, and if it's a " foo\rbar " string, it would instead set the value as "#000000". This makes the expected value "#000000" even when you change the input type to text.
For date and month type inputs however, you're correct that it should expect the empty string instead.
I'll note that removing this check here isn't the correct fix though, we need to think of a more comprehensive way of testing this correctly -- the other PR is currently working on this.
This comment has been minimized.
This comment has been minimized.
KiChjang
Dec 4, 2017
Member
Since the other PR is already fixing this, we should revert the change here.
|
@KiChjang please check. |
|
|
||
| // Step 1, 2 | ||
| let year_int = year.parse::<u32>().map_err(|_| ())?; | ||
| if year.len() < 4 || year_int > 0 { |
This comment has been minimized.
This comment has been minimized.
KiChjang
Dec 3, 2017
Member
If you check whether the year is greater than 0 then all valid years will be rejected.
This comment has been minimized.
This comment has been minimized.
|
After reverting the changes to |
|
@KiChjang Done . Thanks. |
|
@tigercosmos I don't seem to see the changes in |
|
Sorry. I lose reverting! |
|
@KiChjang Reverted! Thanks for your patient |
|
|
|
@tigercosmos Can you squash this into one commit? After that, this can finally land! |
|
@KiChjang done |
|
|
||
| /// Validates this `DOMString` is a time string according to | ||
| /// <https://html.spec.whatwg.org/multipage/#valid-time-string>. | ||
| pub fn is_valid_time_string(&self) -> bool { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
@bors-servo r+ Thanks! |
|
|
implement "Date type inputs", "Month type inputs" <!-- Please describe your changes on the following line: --> implement "Date type inputs", "Month type inputs" --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] `./mach test-unit` does not report any errors <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19385) <!-- Reviewable:end -->
|
|
|
@tigercosmos @KiChjang WOW! Congrats! |
|
@CYBAI thanks! Finally… |
Upstreamed from servo/servo#19385 [ci skip]
tigercosmos commentedNov 26, 2017
•
edited
implement "Date type inputs", "Month type inputs"
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors./mach test-unitdoes not report any errorsThis change is