Example document (extract) <form> <input type="email" multiple name="emails" /> <input type="submit" label="Submit" /> </form> When typing in the input field: "email@hostname.com, email2@hostname.com" and submitting the form a popup/tooltip appears with the text: "Enter a list of emails spearated by comma's" (manually translated from dutch, the correct english sentence will probably be different). The text message is compliant with the spec (a list of emails separated by commas), but this is exactly what I did, so it shouldn't complain. This happens in Chrome 10 (10.0.648.127, windows) and Chrome 11 (11.0.695.0, linux), but NOT in chrome 9 (9.0.597.107, linux). It works in firefox 4, and it should work as I see the spec. Also it says what you should do, but the check itself seems to be incorrect.
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.