Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(input): use Chromium's email validation regexp #5924

Closed
wants to merge 1 commit into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Jan 22, 2014

This change uses the regexp from Chromium/Blink to validate emails, and corrects an error in the validation engine, which previously considered an invalid email to be valid. Additionally, the regexp was invalidating emails with capital letters, however this is not the behaviour recomended in the spec, or implemented in Chromium.

(The regexp can be seen in Blink at https://github.com/mirrors/blink/blob/9d8fc44892f633b1448d0daa638969ffac497bc4/Source/core/html/forms/EmailInputType.cpp#L45-L48)
Closes #5899

This change uses the regexp from Chromium/Blink to validate emails, and corrects
an error in the validation engine, which previously considered an invalid email
to be valid. Additionally, the regexp was invalidating emails with capital
letters, however this is not the behaviour recomended in the spec, or implemented
in Chromium.

Closes angular#5899
@IgorMinar
Copy link
Contributor

lgtm

@IgorMinar
Copy link
Contributor

thanks for the reference to the blink code

@caitp
Copy link
Contributor Author

caitp commented Jan 22, 2014

Thanks for the quick review, I'll merge

@caitp caitp closed this in 79e519f Jan 22, 2014
@caitp caitp deleted the issue-5899 branch January 22, 2014 02:41
@KevinBrogan
Copy link
Contributor

I noticed a bug in chromium's regexp. It accepts a domain part that begins or ends with a dash.

@caitp
Copy link
Contributor Author

caitp commented Jan 26, 2014

Should file a bug on Blink for this, I think it would be best if we kept our validation in sync with their own, IMO. It looks like Firefox will consider a domain like this invalid, so it seems reasonable

@KevinBrogan
Copy link
Contributor

On the one hand, it is good to match the browser. On the other hand, it is good to be correct. I noticed that chromium also accepts domain parts that begin with numbers. :-(

This is the correct implementation of the domain part.

[a-z]([a-z0-9-]*[a-z0-9])?(\.[a-z]([a-z0-9-]*[a-z0-9])?)*

I'll see about filing a bug on Blink and see if we can get both hands to agree.

@KevinBrogan
Copy link
Contributor

Can't say that I like their bug submission form, but I submitted to chromium.
https://code.google.com/p/chromium/issues/detail?id=338140

@caitp
Copy link
Contributor Author

caitp commented Jan 26, 2014

I've filed a bug on Chromium/Blink https://code.google.com/p/chromium/issues/detail?id=338141, I think we can fix it pretty easily in both products. If you feel like getting some code into Chromium you should take a stab at fixing this, it's pretty trivial.

... Oh, you submitted one too! well then ;)

@caitp
Copy link
Contributor Author

caitp commented Jan 28, 2014

@KevinBrogan I've got a patch being reviewed to fix this (https://codereview.chromium.org/132483006/), want to submit a patch updating the code in Angular? It probably won't be more than a week or two before it gets merged, so we should try to keep them in sync.

@KevinBrogan
Copy link
Contributor

@caitp Thanks Caitlin, pull request is up.
#6026

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

email validator is wrong
3 participants