Bug 27457

Summary: Add <input type=email> validation support
Product: WebKit Reporter: Peter Kasting <pkasting>
Component: FormsAssignee: Michelangelo De Simone <michelangelo>
Status: RESOLVED FIXED    
Severity: Enhancement CC: commit-queue, ddkilzer, laszlo.gombos, mike, pkasting, tkent
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.whatwg.org/specs/web-apps/current-work/#e-mail-state
Bug Depends on:    
Bug Blocks: 19264    
Attachments:
Description Flags
Patch v1
none
Patch v1a
none
Patch v1, Changelogs were missing
eric: review-
Patch rev.2
none
Patch rev.2a
none
Patch rev. 2b none

Peter Kasting
Reported Monday, July 20, 2009 8:29:07 PM UTC
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.
Attachments
Patch v1 (7.83 KB, patch)
2009-08-24 10:23 PDT, Michelangelo De Simone
no flags
Patch v1a (7.80 KB, patch)
2009-08-24 10:25 PDT, Michelangelo De Simone
no flags
Patch v1, Changelogs were missing (9.29 KB, patch)
2009-08-24 10:32 PDT, Michelangelo De Simone
eric: review-
Patch rev.2 (9.96 KB, patch)
2009-08-26 12:32 PDT, Michelangelo De Simone
no flags
Patch rev.2a (9.91 KB, patch)
2009-08-28 16:03 PDT, Michelangelo De Simone
no flags
Patch rev. 2b (10.51 KB, patch)
2009-09-15 08:37 PDT, Michelangelo De Simone
no flags
Peter Kasting
Comment 1 Tuesday, July 21, 2009 6:24:47 PM UTC
Note: The underlying "URL" input type and associated parser changes were made on bug 25554, so this is only about validation.
Michelangelo De Simone
Comment 2 Friday, August 21, 2009 2:53:51 PM UTC
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
Comment 3 Monday, August 24, 2009 6:23:33 PM UTC
Created attachment 38484 [details] Patch v1
Michelangelo De Simone
Comment 4 Monday, August 24, 2009 6:25:41 PM UTC
Created attachment 38485 [details] Patch v1a
Michelangelo De Simone
Comment 5 Monday, August 24, 2009 6:32:10 PM UTC
Created attachment 38487 [details] Patch v1, Changelogs were missing
Eric Seidel (no email)
Comment 6 Monday, August 24, 2009 9:36:51 PM UTC
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); }
Eric Seidel (no email)
Comment 7 Tuesday, August 25, 2009 6:43:17 PM UTC
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
Comment 8 Wednesday, August 26, 2009 8:32:00 PM UTC
Created attachment 38631 [details] Patch rev.2 Both Eric's comments have been addressed.
Peter Kasting
Comment 9 Friday, August 28, 2009 11:23:44 PM UTC
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
Comment 10 Saturday, August 29, 2009 12:03:11 AM UTC
Created attachment 38759 [details] Patch rev.2a Corrected, thanks to Peter.
Peter Kasting
Comment 11 Monday, August 31, 2009 8:02:25 AM UTC
Note that Ian has just changed the spec as to what the acceptance criteria are.
Michelangelo De Simone
Comment 12 Monday, August 31, 2009 6:52:29 PM UTC
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
Comment 13 Thursday, September 3, 2009 12:12:28 AM UTC
(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
Comment 14 Friday, September 11, 2009 2:34:46 AM UTC
(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
Comment 15 Monday, September 14, 2009 3:09:12 PM UTC
(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
Comment 16 Monday, September 14, 2009 4:35:33 PM UTC
(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
Comment 17 Tuesday, September 15, 2009 4:37:28 PM UTC
Created attachment 39602 [details] Patch rev. 2b
Adam Barth
Comment 18 Tuesday, October 13, 2009 9:34:25 PM UTC
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 19 Tuesday, October 13, 2009 9:53:20 PM UTC
Comment on attachment 39602 [details] Patch rev. 2b Clearing flags on attachment: 39602 Committed r49508: <http://trac.webkit.org/changeset/49508>
WebKit Commit Bot
Comment 20 Tuesday, October 13, 2009 9:53:26 PM UTC
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.