r75627 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r75626‎ | r75627 | r75628 >
Date:21:01, 28 October 2010
Author:hashar
Status:resolved (Comments)
Tags:
Comment:
Lame user side email validator using JQuery.
See bug 22449 : MediaWiki should do validation of e-mail addresses
Modified paths:
  • /trunk/phase3/includes/specials/SpecialPreferences.php (modified) (history)
  • /trunk/phase3/resources/Resources.php (modified) (history)
  • /trunk/phase3/resources/mediawiki.specials/mediawiki.specials.preferences.css (added) (history)
  • /trunk/phase3/resources/mediawiki.specials/mediawiki.specials.preferences.js (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/specials/SpecialPreferences.php
——@@ -54,6 +54,7 @@
5555
5656 $wgOut->addModules( 'mediawiki.legacy.prefs' );
5757 $wgOut->addModuleScripts( 'mediawiki.specials.preferences' );
 58+ $wgOut->addModuleStyles( 'mediawiki.specials.preferences' );
5859
5960 if ( $wgRequest->getCheck( 'success' ) ) {
6061 $wgOut->wrapWikiMsg(
Index: trunk/phase3/resources/Resources.php
——@@ -321,6 +321,7 @@
322322 ) ),
323323 'mediawiki.specials.preferences' => new ResourceLoaderFileModule( array(
324324 'scripts' => 'resources/mediawiki.specials/mediawiki.specials.preferences.js',
 325+ 'styles' => 'resources/mediawiki.specials/mediawiki.specials.preferences.css',
325326 ) ),
326327 'mediawiki.specials.search' => new ResourceLoaderFileModule( array(
327328 'scripts' => 'resources/mediawiki.specials/mediawiki.specials.search.js',
——@@ -490,4 +491,4 @@
491492 'mediawiki.legacy.wikiprintable' => new ResourceLoaderFileModule( array(
492493 'styles' => array( 'skins/common/wikiprintable.css' => array( 'media' => 'print' ) ),
493494 ) ),
494 -);
\ No newline at end of file
 495+);
Index: trunk/phase3/resources/mediawiki.specials/mediawiki.specials.preferences.js
——@@ -38,3 +38,22 @@
3939 )
4040 );
4141 } );
 42+
 43+// Lame tip to let user know if its email is valid. See bug 22449
 44+$( '#mw-input-emailaddress' )
 45+ .keyup( function() {
 46+ var mailtxt = $(this).val();
 47+ if( mailtxt == '' ) {
 48+ // mail is optional !
 49+ $(this).removeClass( "invalid" );
 50+ $(this).removeClass( "valid" );
 51+ return;
 52+ }
 53+ if( mailtxt.match( /.+@.+\..+/ ) ) {
 54+ $(this).addClass( "valid" );
 55+ $(this).removeClass( "invalid" );
 56+ } else {
 57+ $(this).addClass( "invalid" );
 58+ $(this).removeClass( "valid" );
 59+ }
 60+ } );
Index: trunk/phase3/resources/mediawiki.specials/mediawiki.specials.preferences.css
——@@ -0,0 +1,8 @@
 2+input.valid {
 3+ background-color: #C0FFC0;
 4+ color: black;
 5+}
 6+input.invalid {
 7+ background-color: #FFC0C0;
 8+ color: black;
 9+}
Property changes on: trunk/phase3/resources/mediawiki.specials/mediawiki.specials.preferences.css
___________________________________________________________________
Added: svn:eol-style
110 + native

Follow-up revisions

RevisionCommit summaryAuthorDate
r75670Follow up r75627 (code review comments):...hashar20:44, 29 October 2010
r75682Follow up r75627. Implements r75670 in PHP to validate emails....hashar22:03, 29 October 2010
r75780Follow up r75627 : addModule() is enough.hashar11:21, 1 November 2010
r79924* Consistent single quotes...krinkle04:40, 10 January 2011
r80694Hack invalid w3 spec to validate @localhost email...hashar18:01, 21 January 2011

Comments

#Comment by Hashar (talk | contribs)   21:05, 28 October 2010

The regular expression is really lame since it will validate : 

à!@ç.,

One might want to set the class on load time.

#Comment by Brion VIBBER (talk | contribs)   21:09, 28 October 2010

Should probably make sure this can be hooked into other email fields, such as on the registration form. Probably should have a common framework for inline form field validation so we don't have to define the valid/invalid CSS separately for multiple modules & data types, as well.

#Comment by Hashar (talk | contribs)   21:15, 28 October 2010

Additional notes by Brion:

probably sensible to use a class or similar as trigger for the client-side validation code, rather than a one-off id and split it out of mediawiki.specials.preferences module into a validation module, and add that to specials.preferences' dependencies

(everything sounds easy when brion explain things)

#Comment by Gurch (talk | contribs)   21:12, 28 October 2010

Never really seen the point of this type of validation myself. I take it you've seen what it takes to validate an e-mail address with regular expressions? And even that says nothing about whether the address actually exists.

I can see the use for by-email account creation, to try to stop the account ending up in an unusable state. But it doesn't do that either, if the user enters a valid but non-existent e-mail address. That bug would be better addressed by only creating the account upon successful delivery of the e-mail.

#Comment by Brion VIBBER (talk | contribs)   21:14, 28 October 2010

It's still better to say "hey you just put COMPLETE GARBAGE IN THIS FIELD, you might want to double-check it" if you can, rather than penalizing 99% of normal people making normal mistakes. :)

#Comment by OverlordQ (talk | contribs)   22:41, 28 October 2010

If were pulling statistics out of orifices, I'd have to say the vast majority of people likely input syntactically correct but wrong email addresses then ones that are not valid.

#Comment by Simetrical (talk | contribs)   23:03, 28 October 2010

The point is just to catch silly mistakes like the user entering their username where it asks for the e-mail, or whatever. The HTML5 spec has a somewhat more rigorous definition: http://www.whatwg.org/specs/web-apps/current-work/multipage/states-of-the-type-attribute.html#e-mail-state I think we should use that definition on the server side, and use HTML5 for client-side validation where it works well, which currently means Firefox 4. Check out this URL in a recent Firefox 4 beta:

data:text/html,<!doctype html><input type=email>

They'll be improving their UI further before final release. If you added a submit button, it wouldn't let you submit if the value was invalid. There's no point in reinventing the wheel here and writing our own checks when HTML5 hands them to us right around the corner, IMO.

So I'm marking fixme, I think this isn't a good approach and should be reverted. The UI is worse than nothing, in that it will disturb the large majority of users who are actually entering a valid e-mail address by changing around the background on them while they type.

#Comment by Hashar (talk | contribs)   06:36, 29 October 2010

I am aware of HTML5 self validating input, but we are not going to have widespread support anytime soon. The less technical persons are likely to be the last to upgrade their webbrowser :-p

Instead of background color, maybe I could use a tab along the input box ? See https://twitter.com/signup

#Comment by Simetrical (talk | contribs)   00:15, 1 November 2010

I think we should ideally use the HTML5 markup together with (optionally) some JavaScript fallback for old browsers, using feature-detection. Unfortunately this isn't quite possible, because we really want to blacklist the bad implementations of Opera and some WebKit versions -- but we should be trying to emulate the HTML5 behavior where it's not supported, not make up different and incompatible behavior.

#Comment by Krinkle (talk | contribs)   12:03, 29 October 2010

A populair script I often use in a jQuery environment with forms is the "Validation" plugin

They have a demo here: http://jquery.bassistance.de/validate/demo/ An example of use on a live site: http://www.vaneckblues.nl/contact/

#Comment by Krinkle (talk | contribs)   12:05, 29 October 2010

The syntax is super simple (classes like "email" and attributes like "minlength") and on the fly. Errors are shown in with the label (instead of marking the input field red itself which, in my opinion, can sometimes confuse the user too much while typing in the field)

#Comment by Hashar (talk | contribs)   20:50, 29 October 2010

Follow up in r75670 adressing some issues reported above. Please comment on the new revision. We might want to mark this one (r75627) reverted.

#Comment by Catrope (talk | contribs)   19:37, 31 October 2010 
 		$wgOut->addModuleScripts( 'mediawiki.specials.preferences' );
+		$wgOut->addModuleStyles( 'mediawiki.specials.preferences' );

Just use one addModules() call here.

#Comment by Hashar (talk | contribs)   11:22, 1 November 2010

Thanks, fixed with r75780

#Comment by Reedy (talk | contribs)   19:32, 7 January 2011

Marking resolved, noted on bug 22449, added bug url and tagged upstream

Status & tagging log