Bug 27457 – Add <input type=email> validation support

• Component: Forms
• Status: RESOLVED FIXED

[Peter Kasting]

See spec section 4.10.4.1.5.

Basically, the only difference between an e-mail input and a normal text input is that at validation time the e-mail input can return true from validityState's typeMismatch() method if it doesn't contain a valid email address or list of addresses.  The spec covers in detail what those consist of.

[Peter Kasting]

Note: The underlying "URL" input type and associated parser changes were made on bug 25554, so this is only about validation.

[Michelangelo De Simone]

AFAIK the fastest and most synthetic way to check for email validity is to make use of regexp.
My intention is to divide this task into two separate patches:

Part 1: regular validation check on type=email input value
Part 2: same check when the "multiple" attribute is specified.

[Michelangelo De Simone]

Created attachment 38484 [details]
Patch v1

[Michelangelo De Simone]

Created attachment 38485 [details]
Patch v1a

[Michelangelo De Simone]

Created attachment 38487 [details]
Patch v1, Changelogs were missing

[Eric Seidel (no email)]

Comment on attachment 38487 [details]
Patch v1, Changelogs were missing

I think the test results could be made much clearer by using a little pass/fail wrapper function:

function testEmail(value, expectedValid)
{
    i.value = "[e-mail]";
    shouldBe("v.typeMismatch", "false");
    var didPass = v.typeMismatch == expectedValid;
    var validText = (expectedValue ? !didPass : didPass) ? "an invalid" : "a valid";
    var resultText = "" + value + " is " + validText + " email address"
    if (didPass)
        testPassed(resultText);
    else
        testFailed(resultText);
}

[Eric Seidel (no email)]

Comment on attachment 38487 [details]
Patch v1, Changelogs were missing

Maybe we could also break:
 31 #define RFC5322_DOTATOMTEXT "[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*"

up into parts.  It's kinda confusing to read as-is.

r- for the tests being confusing.

[Michelangelo De Simone]

Created attachment 38631 [details]
Patch rev.2

Both Eric's comments have been addressed.

[Peter Kasting]

Comment on attachment 38631 [details]
Patch rev.2

> +bool ValidityState::isValidEmailAddress(String& email)
> +{
> +    if (email.isEmpty())
> +        return false;
> +
> +    DEFINE_STATIC_LOCAL(String, dotAtomText, (RFC5322_ATEXT));
> +    dotAtomText.append("+(?:\\.");
> +    dotAtomText.append(RFC5322_ATEXT);
> +    dotAtomText.append("+)*");

Wait a minute, won't this append these on every call, making the regex longer and longer?
What about just

#define RFC5322_ATEXT "[a-z0-9!#$%&'*+/=?^_`{|}~-]"
#define RFC5322_DOTATOMTEXT RFC5322_ATEXT "+(?:\\." RFC5322_ATEXT "+)*"
...
DEFINE_STATIC_LOCAL(AtomicString, emailPattern, RFC5322_DOTATOMTEXT "@" RFC5322_DOTATOMTEXT);

[Michelangelo De Simone]

Created attachment 38759 [details]
Patch rev.2a

Corrected, thanks to Peter.

[Peter Kasting]

Note that Ian has just changed the spec as to what the acceptance criteria are.

[Michelangelo De Simone]

Comment on attachment 38759 [details]
Patch rev.2a

Canceling r? due to specs change (4.10.4.1.5); a valid email address is in format 1*( atext / "." ) "@" ldh-str 1*( "." ldh-str )

[Michelangelo De Simone]

(In reply to comment #12)

> Canceling r? due to specs change (4.10.4.1.5); a valid email address is in
> format 1*( atext / "." ) "@" ldh-str 1*( "." ldh-str )

I'm gonna publish a patch that makes use of the following regexp to match Ian's modifications to specs:

[a-z0-9!#$%&'*+/=?^_`{|}~-](?.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)* @ [a-z0-9-]+(?.[a-z0-9-])*

It's obviously quite "verbose" but, for the sake of clearness...

[Kent Tamura]

(In reply to comment #13)
> (In reply to comment #12)
> 
> > Canceling r? due to specs change (4.10.4.1.5); a valid email address is in
> > format 1*( atext / "." ) "@" ldh-str 1*( "." ldh-str )
> 
> I'm gonna publish a patch that makes use of the following regexp to match Ian's
> modifications to specs:
> 
> [a-z0-9!#$%&'*+/=?^_`{|}~-](?.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)* @
> [a-z0-9-]+(?.[a-z0-9-])*
> 
> It's obviously quite "verbose" but, for the sake of clearness...

It should be 
  [a-z0-9!#$%&'*+/=?^_`{|}~-.]+@[a-z0-9-]+(?\.[a-z0-9-])+

[Michelangelo De Simone]

(In reply to comment #14)

> It should be 
>   [a-z0-9!#$%&'*+/=?^_`{|}~-.]+@[a-z0-9-]+(?\.[a-z0-9-])+

This seems slightly incorrect.:)

1*( atext / "." ) = [a-z0-9!#$%&'*+/=?^_`{|}~.-]+
ldh-str 1*( "." ldh-str ) = [a-z0-9-]+(.[a-z0-9-]+)*

[a-z0-9!#$%&'*+/=?^_`{|}~.-]+@[a-z0-9-]+(.[a-z0-9-]+)*

[Kent Tamura]

(In reply to comment #15)
> 1*( atext / "." ) = [a-z0-9!#$%&'*+/=?^_`{|}~.-]+
> ldh-str 1*( "." ldh-str ) = [a-z0-9-]+(.[a-z0-9-]+)*
> 
> [a-z0-9!#$%&'*+/=?^_`{|}~.-]+@[a-z0-9-]+(.[a-z0-9-]+)*

The last '*' should be '+'.  We need at least 1 dot in a domain part.

[Michelangelo De Simone]

Created attachment 39602 [details]
Patch rev. 2b

[Adam Barth]

Comment on attachment 39602 [details]
Patch rev. 2b

This looks good.  I'm not 100% sure the regexp is right, but we can tweak that later if we're not matching HTML5.

[WebKit Commit Bot]

Comment on attachment 39602 [details]
Patch rev. 2b

Clearing flags on attachment: 39602

Committed r49508: <http://trac.webkit.org/changeset/49508>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.