Bug 55987 – input type=email and multiple is not compliant

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Nathan Samson]

Example document (extract)

<form>
<input type="email" multiple name="emails" />
<input type="submit" label="Submit" />
</form>


When typing in the input field: "[e-mail], [e-mail]" 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.

[Kent Tamura]

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.

[Mounir Lamouri]

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.

[Kentaro Hara]

Created attachment 98463 [details]
Patch

[Kentaro Hara]

Created attachment 98464 [details]
Patch

[Kent Tamura]

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   ,   [e-mail]   ", true);
> +emailCheck("   [e-mail]   ,   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?

[Kent Tamura]

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

InputType::convertFromVisibleValue() too.

[Kentaro Hara]

Created attachment 98685 [details]
Patch

[Kentaro Hara]

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("   [e-mail]   ,   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.

[Kent Tamura]

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 "[e-mail]" is a correct valid email address. It was sanitized to "[e-mail]".

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("[e-mail]", "[e-mail]", false, false);

Boolean parameters are not good for readability.
I'd like to do:

var expectValid = false;
var expectInvalid = true;
var multiple = true;

emailCheck("[e-mail]", "[e-mail]", expectValid); // We can omit the trailing false parameter.
emailCheck(" [e-mail],[e-mail]", "[e-mail],[e-mail]", 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.

[Kent Tamura]

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

wtf/text/StringBuilder

[Kentaro Hara]

Created attachment 98855 [details]
Patch

[Kentaro Hara]

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 "[e-mail]" is a correct valid email address. It was sanitized to "[e-mail]".
> 
> 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("[e-mail]", "[e-mail]", false, false);
> 
> Boolean parameters are not good for readability.
> I'd like to do:
> 
> var expectValid = false;
> var expectInvalid = true;
> var multiple = true;
> 
> emailCheck("[e-mail]", "[e-mail]", expectValid); // We can omit the trailing false parameter.
> emailCheck(" [e-mail],[e-mail]", "[e-mail],[e-mail]", 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.

[Kent Tamura]

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).

[Kent Tamura]

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.

[Kentaro Hara]

Created attachment 98861 [details]
Patch

[Kentaro Hara]

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.

[Kent Tamura]

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().

[Kentaro Hara]

Created attachment 98882 [details]
Patch

[Kent Tamura]

Comment on attachment 98882 [details]
Patch

Great patch.  Thank you!

[WebKit Review Bot]

Comment on attachment 98882 [details]
Patch

Clearing flags on attachment: 98882

Committed r89915: <http://trac.webkit.org/changeset/89915>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.