Bug 55987

Summary: input type=email and multiple is not compliant
Product: WebKit Reporter: Nathan Samson <nathansamson+webkit>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, mounir, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 19264    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Nathan Samson 2011-03-08 17:24:34 PST
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.
Comment 1 Kent Tamura 2011-03-09 01:09:28 PST
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.
Comment 2 Mounir Lamouri 2011-03-09 07:06:54 PST
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.
Comment 3 Kentaro Hara 2011-06-23 23:40:30 PDT
Created attachment 98463 [details]
Patch
Comment 4 Kentaro Hara 2011-06-23 23:46:08 PDT
Created attachment 98464 [details]
Patch
Comment 5 Kent Tamura 2011-06-24 03:58:08 PDT
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?
Comment 6 Kent Tamura 2011-06-24 04:07:17 PDT
(In reply to comment #5)
> We need to override TextFieldInputType::sanitizeValue() to strip whitespace.

InputType::convertFromVisibleValue() too.
Comment 7 Kentaro Hara 2011-06-27 01:58:27 PDT
Created attachment 98685 [details]
Patch
Comment 8 Kentaro Hara 2011-06-27 02:00:28 PDT
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 9 Kent Tamura 2011-06-27 02:50:37 PDT
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.
Comment 10 Kent Tamura 2011-06-27 02:52:55 PDT
(In reply to comment #9)
> You had better use wtf/textStirngBuilder.

wtf/text/StringBuilder
Comment 11 Kentaro Hara 2011-06-27 21:41:41 PDT
Created attachment 98855 [details]
Patch
Comment 12 Kentaro Hara 2011-06-27 21:42:17 PDT
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 13 Kent Tamura 2011-06-27 21:56:49 PDT
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 14 Kent Tamura 2011-06-27 22:20:46 PDT
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.
Comment 15 Kentaro Hara 2011-06-27 22:43:34 PDT
Created attachment 98861 [details]
Patch
Comment 16 Kentaro Hara 2011-06-27 22:44:53 PDT
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 17 Kent Tamura 2011-06-27 22:48:53 PDT
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().
Comment 18 Kentaro Hara 2011-06-28 02:10:16 PDT
Created attachment 98882 [details]
Patch
Comment 19 Kent Tamura 2011-06-28 02:11:51 PDT
Comment on attachment 98882 [details]
Patch

Great patch.  Thank you!
Comment 20 WebKit Review Bot 2011-06-28 02:46:21 PDT
Comment on attachment 98882 [details]
Patch

Clearing flags on attachment: 98882

Committed r89915: <http://trac.webkit.org/changeset/89915>
Comment 21 WebKit Review Bot 2011-06-28 02:46:27 PDT
All reviewed patches have been landed.  Closing bug.