Bug 27457

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

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.

Created attachment 38484 [details] Patch v1

Created attachment 38485 [details] Patch v1a

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

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 = "someone@somewhere.com"; 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); }

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.

Created attachment 38631 [details] Patch rev.2 Both Eric's comments have been addressed.

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

Created attachment 38759 [details] Patch rev.2a Corrected, thanks to Peter.

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

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 )

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

(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-])+

(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-]+)*

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

Created attachment 39602 [details] Patch rev. 2b

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.

Comment on attachment 39602 [details] Patch rev. 2b Clearing flags on attachment: 39602 Committed r49508: <http://trac.webkit.org/changeset/49508>

All reviewed patches have been landed. Closing bug.