Issue 21810/improve validation methods #24626

glowe · 2019-11-03T14:08:34Z

This is a start at addressing #21810. I'm putting these changes out early to get some feedback on the following items:

I added unit tests for the validation methods mentioned in #21810, because I couldn't tell whether any of the existing WPT tests covered them. Are these tests worthwhile? Are any of them unnecessary?
I changed the implementation for is_valid_floating_point_number_string so that it passed the tests. The previous version of the function wasn't restrictive enough (it allowed certain whitespace characters before the number string).
I changed the catch-all condition in htmlinputelement.rs to account for the remaining input types that don't have a value sanitization algorithm. This last change seems good to me since we won't be able to add a new input type without adding it to the case and checking the spec for an algorithm.

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

There are tests for these changes

highfive · 2019-11-03T14:08:42Z

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

highfive · 2019-11-03T14:08:43Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmlinputelement.rs, components/script/dom/bindings/str.rs
@KiChjang: components/script/dom/htmlinputelement.rs, components/script/dom/bindings/str.rs

jdm

I'm in favour of unit tests for small, self-contained routines like this.

jdm · 2019-11-12T19:47:36Z

components/script/dom/bindings/str.rs

    /// https://html.spec.whatwg.org/multipage/#valid-month-string
    pub fn is_valid_month_string(&self) -> bool {
-        parse_month_string(&self.0).is_ok()


Whoops, I missed reading this commit. While in general I'm in favour of using state machines for clarity of purpose and avoiding invalid states, I find this one to be difficult to follow. I'm not sure if it's the names of the states, or the repetition between them, or something else, but my eyes keep glazing over when I try to follow the logic. Given how straightforward the specification text is, I'm not exactly sure what to recommend.

My reading of the specification is that the parsing algorithm for month strings is the same as the validation algorithm for month strings, so I don't see a good reason to reimplement this particular algorithm. Presumably there are other input types where they diverge?

My reading of the specification is that the parsing algorithm for month strings is the same as the validation algorithm for month strings, so I don't see a good reason to reimplement this particular algorithm. Presumably there are other input types where they diverge?

Actually, the tests passed for the original implementations of the date validation functions, so I judge them to be equivalent to the specifications. The original issue #21810 recommended reimplementing all of these functions, but I think that recommendation was based on the premise that the functions might be incorrect. I've gone ahead and reverted the change to is_valid_month_string.

The lone validation function with an issue was is_valid_floating_point_number_string, so I've fixed it. Please take another look and let me know what you think.

jdm · 2019-11-20T14:48:22Z

@bors-servo r+
Thanks!

bors-servo · 2019-11-20T14:48:24Z

📌 Commit 8abbae1 has been approved by jdm

bors-servo · 2019-11-20T16:15:03Z

⌛ Testing commit 8abbae1 with merge da3f021...


        Auto merge of #24626 - glowe:issue-21810/improve-validation-methods, …

…r=jdm Issue 21810/improve validation methods  This is a start at addressing #21810. I'm putting these changes out early to get some feedback on the following items: 1. I added unit tests for the validation methods mentioned in #21810, because I couldn't tell whether any of the existing WPT tests covered them. Are these tests worthwhile? Are any of them unnecessary? 2. I changed the implementation for `is_valid_floating_point_number_string` so that it passed the tests. The previous version of the function wasn't restrictive enough (it allowed certain whitespace characters before the number string). 3. I changed the catch-all condition in `htmlinputelement.rs` to account for the remaining input types that don't have a value sanitization algorithm. This last change seems good to me since we won't be able to add a new input type without adding it to the case and checking the spec for an algorithm. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21810  - [x] There are tests for these changes

bors-servo · 2019-11-20T17:31:23Z

💔 Test failed - status-taskcluster

jdm · 2019-11-20T17:42:28Z

Hmm, looks like we now fail a test that we used to pass:

{
    "status": "FAIL", 
    "group": "default", 
    "message": "assert_equals: expected \"\" but got \"2e308\"", 
    "stack": "@http://web-platform.test:8000/html/semantics/forms/the-input-element/number.html:50:7\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1931:25\ntest@http://web-platform.test:8000/resources/testharness.js:544:30\n@http://web-platform.test:8000/html/semantics/forms/the-input-element/number.html:43:5\n", 
    "subtest": "value >= Number.MAX_VALUE", 
    "test": "/html/semantics/forms/the-input-element/number.html", 
    "line": 60091, 
    "action": "test_result", 
    "expected": "PASS"
}

This can be reproduced with ./mach test-wpt tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/number.html.

glowe · 2019-11-27T04:23:01Z

This can be reproduced with ./mach test-wpt tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/number.html.

Thanks for identifying the failing test for me.

We are now failing on a bounds check: 2e308 exceeds Number.MAX_VALUE (1.7976931348623157e+308). I could be reading this wrong, but the specification for a valid floating point number (https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#valid-floating-point-number) doesn't mention a bounds. However the specification for parsing (https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#rules-for-parsing-floating-point-number-values) does mention it in that the converted value must be a IEEE 754 double-precision floating-point value.

Interestingly, I tried this specific test on Firefox 70.0.1 and it failed (http://wpt.live/html/semantics/forms/the-input-element/number.html). However, this test passes in the latest Chrome and Safari browsers.

Here's what I propose we do next (let me know what you think):

1.) I'll update the code to handle the bounds check, probably by delegating to the parsing code after verifying that there's no leading whitespace.
2.) I'm now wondering about the value of the unit tests I added. I didn't notice these web platform tests before and it seems like the unit tests are duplicative and potentially misleading. I'm leaning towards dropping the unit tests for this PR.
3.) The web platform tests for number inputs are incomplete for the leading whitespace check:

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

Line 38 in b62d1d4

{value: " 1", expected: "", testname: "value with a leading whitespace"},

The specification mentions that the whitespace characters can be TAB, LF, FF, CR, or SPACE (https://infra.spec.whatwg.org/#ascii-whitespace). Our previous implementation only handled SPACEs. Would it be worth me filing an issue (and potentially a PR) to add tests for the other characters?

EDIT:
I've dealt with item 1. For item 2, I removed all unit tests except the one that demonstrates the existing issue with is_valid_floating_point_number_string (i.e., allowing other leading whitespace characters).


        Remove catch-all case for input sanitization

Replaced catch-all with explicit case for inputs that do not have a value sanitization algorithm. This should prevent us from forgetting to implement a sanitization for an input, since they must all be accounted for in the match expression.


        Add DOMString floating point string test and fix

Fixes an issue where DOMString::is_valid_floating_point_number_string was returning true for strings that began with whitespace characters- TAB, LF, FF, or CR. Also added a unit test to cover this since the corresponding web-platform-tests are incomplete.

jdm · 2019-12-03T00:08:00Z

Those choices seem reasonable! You can also include the WPT changes in this PR, because we automatically sync them upstream when they merge in our vendored copy.

jdm · 2019-12-03T00:11:07Z

And FYI https://wpt.fyi/results/html/semantics/forms/the-input-element/number.html?label=master&label=experimental&aligned&q=number.html backs up your results with respect to Firefox's test results.


        Enhance wpt for number input

Add cases for all leading ASCII whitespace characters.


        Remove redundant domstring unit test

This test is no longer necessary since the cases tested are also tested by the number input web platform test.

glowe · 2019-12-03T03:54:49Z

Those choices seem reasonable! You can also include the WPT changes in this PR, because we automatically sync them upstream when they merge in our vendored copy.

Thanks! I did not know that. I've enhanced the number input WPT to include those additional cases and deleted the unit test since it's redundant coverage.

servo-wpt-sync · 2019-12-03T03:54:59Z

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#20575.

jdm · 2019-12-03T17:13:21Z

@bors-servo r+
Thanks!

bors-servo · 2019-12-03T17:13:22Z

📌 Commit b9ec6f9 has been approved by jdm

bors-servo · 2019-12-03T18:11:27Z

⌛ Testing commit b9ec6f9 with merge 7ba88e8...


        Auto merge of #24626 - glowe:issue-21810/improve-validation-methods, …

…r=jdm Issue 21810/improve validation methods  This is a start at addressing #21810. I'm putting these changes out early to get some feedback on the following items: 1. I added unit tests for the validation methods mentioned in #21810, because I couldn't tell whether any of the existing WPT tests covered them. Are these tests worthwhile? Are any of them unnecessary? 2. I changed the implementation for `is_valid_floating_point_number_string` so that it passed the tests. The previous version of the function wasn't restrictive enough (it allowed certain whitespace characters before the number string). 3. I changed the catch-all condition in `htmlinputelement.rs` to account for the remaining input types that don't have a value sanitization algorithm. This last change seems good to me since we won't be able to add a new input type without adding it to the case and checking the spec for an algorithm. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21810  - [x] There are tests for these changes

bors-servo · 2019-12-03T19:31:03Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 7ba88e8 to master...

highfive assigned asajeffrey Nov 3, 2019

highfive added the S-awaiting-review label Nov 3, 2019

jdm approved these changes Nov 12, 2019

View changes

jdm reviewed Nov 12, 2019

View changes

glowe force-pushed the glowe:issue-21810/improve-validation-methods branch from 1986ddb to 8abbae1 Nov 20, 2019

glowe changed the title ~~[WIP] Issue 21810/improve validation methods~~ Issue 21810/improve validation methods Nov 20, 2019

glowe marked this pull request as ready for review Nov 20, 2019

jdm approved these changes Nov 20, 2019

View changes

highfive assigned jdm and unassigned asajeffrey Nov 20, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 20, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 20, 2019

glowe added 2 commits Dec 1, 2019

Remove catch-all case for input sanitization

23359c5

Replaced catch-all with explicit case for inputs that do not have a value sanitization algorithm. This should prevent us from forgetting to implement a sanitization for an input, since they must all be accounted for in the match expression.

glowe force-pushed the glowe:issue-21810/improve-validation-methods branch from 8abbae1 to 576f51f Dec 1, 2019

highfive added S-awaiting-review and removed S-tests-failed labels Dec 1, 2019

jdm approved these changes Dec 3, 2019

View changes

glowe added 2 commits Dec 3, 2019

Enhance wpt for number input

b8a8c2b

Add cases for all leading ASCII whitespace characters.

Remove redundant domstring unit test

Loading status checks…

b9ec6f9

This test is no longer necessary since the cases tested are also tested by the number input web platform test.

servo-wpt-sync mentioned this pull request Dec 3, 2019

Issue 21810/improve validation methods web-platform-tests/wpt#20575

Merged

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 3, 2019

bors-servo merged commit b9ec6f9 into servo:master Dec 3, 2019
1 of 2 checks passed

1 of 2 checks passed

Community-TC (pull_request) TaskGroup: failure
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 3, 2019

servo / servo

Issue 21810/improve validation methods #24626

Issue 21810/improve validation methods #24626

glowe commented Nov 3, 2019 •

edited

highfive commented Nov 3, 2019

highfive commented Nov 3, 2019

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Nov 20, 2019

bors-servo commented Nov 20, 2019

bors-servo commented Nov 20, 2019

bors-servo commented Nov 20, 2019

jdm commented Nov 20, 2019

glowe commented Nov 27, 2019 •

edited

jdm commented Dec 3, 2019

jdm commented Dec 3, 2019

glowe commented Dec 3, 2019

servo-wpt-sync commented Dec 3, 2019

jdm commented Dec 3, 2019

bors-servo commented Dec 3, 2019

bors-servo commented Dec 3, 2019

bors-servo commented Dec 3, 2019

servo / servo

Join GitHub today

Issue 21810/improve validation methods #24626

Issue 21810/improve validation methods #24626

Conversation

glowe commented Nov 3, 2019 • edited

highfive commented Nov 3, 2019

highfive commented Nov 3, 2019

jdm left a comment

This comment has been minimized.

jdm Nov 12, 2019

This comment has been minimized.

jdm Nov 12, 2019

This comment has been minimized.

glowe Nov 20, 2019 • edited

jdm commented Nov 20, 2019

bors-servo commented Nov 20, 2019

bors-servo commented Nov 20, 2019

bors-servo commented Nov 20, 2019

jdm commented Nov 20, 2019

glowe commented Nov 27, 2019 • edited

jdm commented Dec 3, 2019

jdm commented Dec 3, 2019

glowe commented Dec 3, 2019

servo-wpt-sync commented Dec 3, 2019

jdm commented Dec 3, 2019

bors-servo commented Dec 3, 2019

bors-servo commented Dec 3, 2019

bors-servo commented Dec 3, 2019

glowe commented Nov 3, 2019 •

edited

glowe Nov 20, 2019 •

edited

glowe commented Nov 27, 2019 •

edited