Bug 55987

Yeah, the current WebKit implementation doesn't allow spaces between addresses. The specification was updated recently to accept spaces. We need to update the implementation.

FWIW, I believe the specs have always accepted spaces between emails in <input type='email'> and the specs recently change so the submitted value doesn't contain spaces, IIRC.

Created attachment 98463 [details] Patch

Created attachment 98464 [details] Patch

Comment on attachment 98464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98464&action=review Thank you for working on this. However, the code change is not enough. We should allow whitespace in user-input, but we should not send whitespace on form submission and HTMLInputElement::value should not return a string with whitespace. Please see http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html#e-mail-state We need to override TextFieldInputType::sanitizeValue() to strip whitespace. > LayoutTests/fast/forms/resources/ValidityState-typeMismatch-email.js:75 > +emailCheck(" a @p.com , b@p.com ", true); > +emailCheck(" a@p.com , b @p.com ", true); We should have tests for space characters other than U+0020. U+0009 U+000A U+000C U+000D should be stripped, and U+2003 and U+3000 should not be stripped. http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#space-character > Source/WebCore/html/EmailInputType.cpp:-66 > - if (value.isEmpty()) > - return false; Why did you remove these lines?

(In reply to comment #5) > We need to override TextFieldInputType::sanitizeValue() to strip whitespace. InputType::convertFromVisibleValue() too.

Created attachment 98685 [details] Patch

Comment on attachment 98464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98464&action=review >> LayoutTests/fast/forms/resources/ValidityState-typeMismatch-email.js:75 >> +emailCheck(" a@p.com , b @p.com ", true); > > We should have tests for space characters other than U+0020. > U+0009 U+000A U+000C U+000D should be stripped, and U+2003 and U+3000 should not be stripped. > > http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#space-character Added those tests. >> Source/WebCore/html/EmailInputType.cpp:-66 >> - return false; > > Why did you remove these lines? Sorry but this is my mistake. I reverted it back to the original.

Comment on attachment 98685 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98685&action=review > LayoutTests/fast/forms/ValidityState-typeMismatch-email-expected.txt:6 > +PASS "something@something.com" is a correct valid email address. It was sanitized to "something@something.com". The test result readability is not good. How about - Remove 'correct' and 'incorrect' We show 'PASS' or 'FAIL'. It's enough. - Show sanitized value only if the expected sanitized value is not identical to the input value or a sanitization test fails. > LayoutTests/fast/forms/resources/ValidityState-typeMismatch-email.js:21 > // VALID It's good to print such headings. You can debug('Valid single addresses:'); or something. > LayoutTests/fast/forms/resources/ValidityState-typeMismatch-email.js:22 > +emailCheck("something@something.com", "something@something.com", false, false); Boolean parameters are not good for readability. I'd like to do: var expectValid = false; var expectInvalid = true; var multiple = true; emailCheck("something@something.com", "something@something.com", expectValid); // We can omit the trailing false parameter. emailCheck(" a@p.com,b@p.com", "a@p.com,b@p.com", expectInvalid, multiple); > Source/JavaScriptCore/wtf/ASCIICType.h:147 > + // White spaces in the HTML specification. > + inline bool isASCIISpaceExceptVT(char c) { return c == ' ' || (c <= 0xD && c >= 0x9 && c != 0xB); } > + inline bool isASCIISpaceExceptVT(unsigned short c) { return c == ' ' || (c <= 0xD && c >= 0x9 && c != 0xB); } > +#if !COMPILER(MSVC) || defined(_NATIVE_WCHAR_T_DEFINED) > + inline bool isASCIISpaceExceptVT(wchar_t c) { return c == ' ' || (c <= 0xD && c >= 0x9 && c != 0xB); } > +#endif > + inline bool isASCIISpaceExceptVT(int c) { return c == ' ' || (c <= 0xD && c >= 0x9 && c != 0xB); } > + inline bool isASCIISpaceExceptVT(unsigned c) { return c == ' ' || (c <= 0xD && c >= 0x9 && c != 0xB); } > + > + inline bool isASCIILineBreak(char c) { return c == 0xA || c == 0xD; } > + inline bool isASCIILineBreak(unsigned short c) { return c == 0xA || c == 0xD; } > +#if !COMPILER(MSVC) || defined(_NATIVE_WCHAR_T_DEFINED) > + inline bool isASCIILineBreak(wchar_t c) { return c == 0xA || c == 0xD; } > +#endif > + inline bool isASCIILineBreak(int c) { return c == 0xA || c == 0xD; } > + inline bool isASCIILineBreak(unsigned c) { return c == 0xA || c == 0xD; } > + These functions are HTML-specific, and not useful for JavaScriptCode. So I think they should be in WebCore/html/parser/HTMLParserIdioms.{cpp,h}. > Source/JavaScriptCore/wtf/text/StringImpl.cpp:351 > + // skip white space from start Usually comments should be sentences. "// Skip white space from start." Anyway, this comment seems not helpful. So remove this comment please. > Source/JavaScriptCore/wtf/text/StringImpl.h:294 > + PassRefPtr<StringImpl> stripWhiteSpaceExceptVT(); > + PassRefPtr<StringImpl> stripLineBreaks(); ditto. Move them to HTMLParserIdioms. > Source/WebCore/html/EmailInputType.cpp:97 > +String EmailInputType::convertFromVisibleValue(const String& visibleValue) const > +{ > + return visibleValue; > +} > + It's ok to remove this function. > Source/WebCore/html/EmailInputType.cpp:111 > + String strippedValue; > + for (unsigned i = 0; i < addresses.size(); ++i) { > + if (i > 0) > + strippedValue += String(","); > + strippedValue += addresses[i].stripWhiteSpaceExceptVT(); > + } > + return strippedValue; You had better use wtf/textStirngBuilder.

(In reply to comment #9) > You had better use wtf/textStirngBuilder. wtf/text/StringBuilder

Created attachment 98855 [details] Patch

Comment on attachment 98685 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98685&action=review >> LayoutTests/fast/forms/ValidityState-typeMismatch-email-expected.txt:6 >> +PASS "something@something.com" is a correct valid email address. It was sanitized to "something@something.com". > > The test result readability is not good. > How about > - Remove 'correct' and 'incorrect' > We show 'PASS' or 'FAIL'. It's enough. > - Show sanitized value only if the expected sanitized value is not identical to the input value or a sanitization test fails. Done. >> LayoutTests/fast/forms/resources/ValidityState-typeMismatch-email.js:21 >> // VALID > > It's good to print such headings. You can > debug('Valid single addresses:'); > or something. Done. >> LayoutTests/fast/forms/resources/ValidityState-typeMismatch-email.js:22 >> +emailCheck("something@something.com", "something@something.com", false, false); > > Boolean parameters are not good for readability. > I'd like to do: > > var expectValid = false; > var expectInvalid = true; > var multiple = true; > > emailCheck("something@something.com", "something@something.com", expectValid); // We can omit the trailing false parameter. > emailCheck(" a@p.com,b@p.com", "a@p.com,b@p.com", expectInvalid, multiple); Done. >> Source/JavaScriptCore/wtf/ASCIICType.h:147 >> + > > These functions are HTML-specific, and not useful for JavaScriptCode. So I think they should be in WebCore/html/parser/HTMLParserIdioms.{cpp,h}. Moved them. >> Source/JavaScriptCore/wtf/text/StringImpl.cpp:351 >> + // skip white space from start > > Usually comments should be sentences. "// Skip white space from start." > Anyway, this comment seems not helpful. So remove this comment please. I removed this method since I found that stripLeadingAndTrailingHTMLSpaces() is doing the same thing. >> Source/JavaScriptCore/wtf/text/StringImpl.h:294 >> + PassRefPtr<StringImpl> stripLineBreaks(); > > ditto. Move them to HTMLParserIdioms. Done. >> Source/WebCore/html/EmailInputType.cpp:97 >> + > > It's ok to remove this function. Done. >> Source/WebCore/html/EmailInputType.cpp:111 >> + return strippedValue; > > You had better use wtf/textStirngBuilder. Done.

Comment on attachment 98855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98855&action=review > Source/WebCore/html/EmailInputType.h:49 > + virtual String sanitizeValue(const String& proposedValue); No need to write the argument name. > Source/WebCore/html/parser/HTMLParserIdioms.cpp:72 > + StringBuilder builder; > + for (unsigned i = 0; i < length; i++) { > + if (!isHTMLLineBreak(string[i])) > + builder.append(string[i]); > + } > + return builder.toString(); - You can avoid to build StringBuilder if the input string has no line breaks. - You had better call builder.reserveCapacity(length).

Comment on attachment 98855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98855&action=review > Source/WebCore/html/parser/HTMLParserIdioms.cpp:68 > + for (unsigned i = 0; i < length; i++) { We prefer "++i" to "i++". You use ++i in another function. it's inconsistent.

Created attachment 98861 [details] Patch

Comment on attachment 98855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98855&action=review >> Source/WebCore/html/EmailInputType.h:49 >> + virtual String sanitizeValue(const String& proposedValue); > > No need to write the argument name. Done. >> Source/WebCore/html/parser/HTMLParserIdioms.cpp:72 >> + return builder.toString(); > > - You can avoid to build StringBuilder if the input string has no line breaks. > - You had better call builder.reserveCapacity(length). Done.

Comment on attachment 98861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98861&action=review > Source/WebCore/html/parser/HTMLParserIdioms.cpp:69 > +String stripHTMLLineBreaks(const String& string) > +{ > + unsigned length = string.length(); > + if (!length) > + return String(); > + > + unsigned i; > + for (i = 0; i < length; i++) { > + if (isHTMLLineBreak(string[i])) I'm sorry that I have just remembered we have String::removeCharacters(). So, we can replace stipHTMLLineBreaks() calls with proposedValue.removeCharacters(isHTMLLineBreak), and need no stripHTMLLineBreaks().

Created attachment 98882 [details] Patch

Comment on attachment 98882 [details] Patch Great patch. Thank you!

Comment on attachment 98882 [details] Patch Clearing flags on attachment: 98882 Committed r89915: <http://trac.webkit.org/changeset/89915>

All reviewed patches have been landed. Closing bug.