The value sanitization algorithm for input type=url should strip leading and trailing whitespaces. See https://html.spec.whatwg.org/multipage/forms.html#url-state-(type=url) This bug was found by the newly added test: LayoutTests/http/tests/w3c/html/semantics/forms/the-input-element/url.html
<rdar://problem/22589358>
For reference, from https://html.spec.whatwg.org/multipage/forms.html#url-state-(type=url) : The value sanitization algorithm is as follows: Strip line breaks from the value, then strip leading and trailing whitespace from the value.
Added URLInputType::sanitizeValue to strip leading and trailing spaces.
Created attachment 263059 [details] Patch
Comment on attachment 263059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263059&action=review > Source/WebCore/ChangeLog:8 > + Please add a link to the spec and indicate the behavior of other major browsers. > LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state-expected.txt:87 > +FAIL change state from url to text assert_equals: input.value should be foobar after change of state expected " foobar " but got "foobar" There are new failures here. I think we should handle these cases properly in this patch.
> There are new failures here. I think we should handle these cases properly in this patch. The failures are a confusing part of the test. They look like errors, but they seem to be part of capturing correct/expected results. What's happening in the test is that there is a double-nested loop, each loop iterating over each possible type of input (hidden, email, url, number, color, etc.). The core of the loop sets the type of the input element to the type controlled by the outer loop, sets the input element's value to a test string (" foobar "), sets the type of the input element to the type controlled by the inner loop, and checks the input element's new value. In order to check that the new value is correct, the test would need to encode the full matrix of initial-type -> new-type pairs, along with the expected new value for that pair (i.e., {"url/hidden/<result>", "url/text/<result>", "url/search/<result>", ... }). There are around 22 different types, meaning 484 pairs that would need to be captured in the test. This seems onerous and error-prone to generate and maintain. Therefore (and, since I didn't originally write this test, I'm only inferring here), it seems that the creator of this test captured the expected results in the *-expected.txt file by allowing both PASS and FAIL lines. The PASS lines cover test pairs where the result can trivially be validated. The FAIL lines cover test pairs where the result cannot be trivially be determined. But since the FAIL line contains information on what was actually produced in the conversion from one type to another, if a test run continues to match that FAIL line, then the test is correct and passes. If we start failing in a different way, then something has changed that needs to be investigated. That's what happened with the patch that sanitizes input[type=url].value. The resulting value is now different, leading to some FAIL tests to now fail in a different way, some PASS tests to now fail, and some FAIL tests to now PASS. I went through all the tests that now produce different results to verify that the new results are expected and that the *-expected.txt file contains the patterns we now want. Please let me know if it's OK to keep this current approach, or if we should implement a new approach (e.g., writing some script to generate and maintain the full set of 484 expected results).
Comment on attachment 263059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263059&action=review r=me but please update the Change Log as suggested before landing. >> LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/type-change-state-expected.txt:87 >> +FAIL change state from url to text assert_equals: input.value should be foobar after change of state expected " foobar " but got "foobar" > > There are new failures here. I think we should handle these cases properly in this patch. Ok, never mind this comment.
Created attachment 263281 [details] Patch
Comment on attachment 263281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263281&action=review > Source/WebCore/ChangeLog:7 > + Reviewed by cdumez@apple.com. email won't work. > Source/WebCore/ChangeLog:12 > + Chrome also has the same issue with url.html. Firefox passes. All extra space before 'All'. > LayoutTests/imported/w3c/ChangeLog:7 > + Reviewed by cdumez@apple.com. ditto.
Created attachment 263288 [details] Patch
Comment on attachment 263288 [details] Patch Clearing flags on attachment: 263288 Committed r191188: <http://trac.webkit.org/changeset/191188>
All reviewed patches have been landed. Closing bug.
The landed patch looks wrong to me. Please refer to https://code.google.com/p/chromium/issues/detail?id=446108#c10
Comment on attachment 263288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263288&action=review > Source/WebCore/html/URLInputType.cpp:69 > + return stripLeadingAndTrailingHTMLSpaces(BaseTextInputType::sanitizeValue(proposedValue)); TextFieldInputType::sanitizeValue() is not called. Now 'value' IDL property can have CR/LF wrongly.
Comment on attachment 263288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263288&action=review >> Source/WebCore/html/URLInputType.cpp:69 >> + return stripLeadingAndTrailingHTMLSpaces(BaseTextInputType::sanitizeValue(proposedValue)); > > TextFieldInputType::sanitizeValue() is not called. Now 'value' IDL property can have CR/LF wrongly. I don't understand this comment. We're calling BaseTextInputType::sanitizeValue(), which ends up calling TextFieldInputType::sanitizeValue() (because BaseTextInputType is a subclass of TextFieldInputType and BaseTextInputType does not override sanitizeValue().
Comment on attachment 263288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263288&action=review >>> Source/WebCore/html/URLInputType.cpp:69 >>> + return stripLeadingAndTrailingHTMLSpaces(BaseTextInputType::sanitizeValue(proposedValue)); >> >> TextFieldInputType::sanitizeValue() is not called. Now 'value' IDL property can have CR/LF wrongly. > > I don't understand this comment. We're calling BaseTextInputType::sanitizeValue(), which ends up calling TextFieldInputType::sanitizeValue() (because BaseTextInputType is a subclass of TextFieldInputType and BaseTextInputType does not override sanitizeValue(). My bad. You're right.