r54567 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r54566‎ | r54567 | r54568 >
Date:03:32, 7 August 2009
Author:simetrical
Status:ok (Comments)
Tags:
Comment:
Start using some HTML 5 form features

autofocus attribute added in some places; this looks like it's respected
by both recent Opera and recent WebKit. Its function is
self-explanatory. :) I used this in a few obvious places like
Special:UserLogin and Special:ResetPass to focus the first field in the
form. Could be used in other places too: Special:Search, etc.

required attribute added in some places. This is only supported in
recent Opera at the moment. Also self-explanatory: it won't allow form
submission if the field is empty.

For stuff using HTMLForm (i.e., Special:Preferences), validation will be
done for integers and floats. Browsers that support this (recent Opera)
will not allow non-integers to be submitted for integer fields, will not
allow non-floating-point values to be submitted for float fields, and
will enforce any min/max values specified. Opera also gives little up
and down arrows to allow the user to increment/decrement the value in
addition to letting them edit the field as text.

For HTMLForm and account creation, the email input type is used for
e-mails. This enforces a sane set of values for e-mails (alphanumerics
plus some ASCII punctuation, with an @ in it). Again, this is supported
only by recent Opera (yay Opera!). Note that this is actually more
restrictive than what we currently check for on the server side; it
might be sane to tighten up our server-side checks to forbid e-mail
addresses that HTML 5 forbids.

In all cases, the extra features aren't added if $wgHtml5 is false, and
will be ignored by non-supporting browsers.

The major room for further improvement here is use of the pattern
attribute. We can have the client refuse to submit the form unless it
matches a regex! The HTML 5 spec says that if a title attribute is
provided, it should be a message that explains what the valid values
are and browsers should provide it to the user if the regex doesn't
match, so it's not a usability problem. I didn't bother adding that
anywhere at this point because it would require adding new messages, but
it should be easy to do. Note of course that HTMLForm should be updated
to verify that pattern matches on the server side as well -- this way we
have a clean, unified way of ensuring that our client and server checks
are the same.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/HTMLForm.php (modified) (history)
  • /trunk/phase3/includes/Preferences.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialResetpass.php (modified) (history)
  • /trunk/phase3/includes/templates/Userlogin.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/HTMLForm.php
@@ -23,6 +23,10 @@
2424 'float' => 'HTMLFloatField',
2525 'info' => 'HTMLInfoField',
2626 'selectorother' => 'HTMLSelectOrOtherField',
 27+ # HTMLTextField will output the correct type="" attribute automagically.
 28+ # There are about four zillion other HTML 5 input types, like url, but
 29+ # we don't use those at the moment, so no point in adding all of them.
 30+ 'email' => 'HTMLTextField',
2731 );
2832
2933 function __construct( $descriptor, $messagePrefix ) {
@@ -512,22 +516,53 @@
513517 }
514518
515519 class HTMLTextField extends HTMLFormField {
516 -
517520 function getSize() {
518521 return isset( $this->mParams['size'] ) ? $this->mParams['size'] : 45;
519522 }
520523
521524 function getInputHTML( $value ) {
 525+ global $wgHtml5;
522526 $attribs = array( 'id' => $this->mID );
523527
524528 if ( isset( $this->mParams['maxlength'] ) ) {
525529 $attribs['maxlength'] = $this->mParams['maxlength'];
526530 }
527531
528 - if( !empty( $this->mParams['disabled'] ) ) {
 532+ if ( !empty( $this->mParams['disabled'] ) ) {
529533 $attribs['disabled'] = 'disabled';
530534 }
531535
 536+ if ( $wgHtml5 ) {
 537+ # TODO: Enforce pattern, step, required, readonly on the server
 538+ # side as well
 539+ foreach ( array( 'min', 'max', 'pattern', 'title', 'step',
 540+ 'placeholder' ) as $param ) {
 541+ if ( isset( $this->mParams[$param] ) ) {
 542+ $attribs[$param] = $this->mParams[$param];
 543+ }
 544+ }
 545+ foreach ( array( 'required', 'autofocus', 'multiple', 'readonly' )
 546+ as $param ) {
 547+ if ( isset( $this->mParams[$param] ) ) {
 548+ $attribs[$param] = '';
 549+ }
 550+ }
 551+ if ( isset( $this->mParams['type'] ) ) {
 552+ switch ( $this->mParams['type'] ) {
 553+ case 'email':
 554+ $attribs['type'] = 'email';
 555+ break;
 556+ case 'int':
 557+ $attribs['type'] = 'number';
 558+ break;
 559+ case 'float':
 560+ $attribs['type'] = 'number';
 561+ $attribs['step'] = 'any';
 562+ break;
 563+ }
 564+ }
 565+ }
 566+
532567 return Xml::input(
533568 $this->mName,
534569 $this->getSize(),
@@ -535,7 +570,6 @@
536571 $attribs
537572 );
538573 }
539 -
540574 }
541575
542576 class HTMLFloatField extends HTMLTextField {
Index: trunk/phase3/includes/specials/SpecialResetpass.php
@@ -128,14 +128,24 @@
129129 }
130130
131131 function pretty( $fields ) {
 132+ global $wgHtml5;
132133 $out = '';
133134 foreach( $fields as $list ) {
134135 list( $name, $label, $type, $value ) = $list;
135136 if( $type == 'text' ) {
136137 $field = htmlspecialchars( $value );
137138 } else {
 139+ $attribs = array( 'id' => $name, 'type' => $type );
 140+ if ( $wgHtml5 ) {
 141+ # All three fields are required, and we should focus the
 142+ # first (wpPassword)
 143+ $attribs['required'] = '';
 144+ if ( $name == 'wpPassword' ) {
 145+ $attribs['autofocus'] = '';
 146+ }
 147+ }
138148 $field = Xml::input( $name, 20, $value,
139 - array( 'id' => $name, 'type' => $type ) );
 149+ $attribs );
140150 }
141151 $out .= '<tr>';
142152 $out .= "<td class='mw-label'>";
Index: trunk/phase3/includes/Preferences.php
@@ -334,7 +334,7 @@
335335
336336 $defaultPreferences['emailaddress'] =
337337 array(
338 - 'type' => $wgAuth->allowPropChange( 'emailaddress' ) ? 'text' : 'info',
 338+ 'type' => $wgAuth->allowPropChange( 'emailaddress' ) ? 'email' : 'info',
339339 'default' => $user->getEmail(),
340340 'section' => 'personal/email',
341341 'label-message' => 'youremail',
Index: trunk/phase3/includes/templates/Userlogin.php
@@ -12,6 +12,8 @@
1313 */
1414 class UserloginTemplate extends QuickTemplate {
1515 function execute() {
 16+ global $wgHtml5;
 17+
1618 if( $this->data['message'] ) {
1719 ?>
1820 <div class="<?php $this->text('messagetype') ?>box">
@@ -37,7 +39,11 @@
3840 <td class="mw-input">
3941 <input type='text' class='loginText' name="wpName" id="wpName1"
4042 tabindex="1"
41 - value="<?php $this->text('name') ?>" size='20' />
 43+ value="<?php $this->text('name'); ?>" size='20'<?php
 44+if ( $wgHtml5 ) {
 45+ echo ' required="" autofocus=""';
 46+}
 47+?>" />
4248 </td>
4349 </tr>
4450 <tr>
@@ -109,6 +115,8 @@
110116 }
111117
112118 function execute() {
 119+ global $wgHtml5, $wgMinimalPasswordLength;
 120+
113121 if( $this->data['message'] ) {
114122 ?>
115123 <div class="<?php $this->text('messagetype') ?>box">
@@ -132,7 +140,11 @@
133141 <td class="mw-input">
134142 <input type='text' class='loginText' name="wpName" id="wpName2"
135143 tabindex="1"
136 - value="<?php $this->text('name') ?>" size='20' />
 144+ value="<?php $this->text('name') ?>" size='20'<?php
 145+if ( $wgHtml5 ) {
 146+ echo ' required=""';
 147+}
 148+?> />
137149 </td>
138150 </tr>
139151 <tr>
@@ -140,7 +152,11 @@
141153 <td class="mw-input">
142154 <input type='password' class='loginPassword' name="wpPassword" id="wpPassword2"
143155 tabindex="2"
144 - value="" size='20' />
 156+ value="" size='20'<?php
 157+if ( $wgHtml5 && $wgMinimalPasswordLength > 0 ) {
 158+ echo ' required=""';
 159+}
 160+?> />
145161 </td>
146162 </tr>
147163 <?php if( $this->data['usedomain'] ) {
@@ -165,14 +181,18 @@
166182 <input type='password' class='loginPassword' name="wpRetype" id="wpRetype"
167183 tabindex="4"
168184 value=""
169 - size='20' />
 185+ size='20'<?php
 186+if ( $wgHtml5 && $wgMinimalPasswordLength > 0 ) {
 187+ echo ' required=""';
 188+}
 189+?> />
170190 </td>
171191 </tr>
172192 <tr>
173193 <?php if( $this->data['useemail'] ) { ?>
174194 <td class="mw-label"><label for='wpEmail'><?php $this->msg('youremail') ?></label></td>
175195 <td class="mw-input">
176 - <input type='text' class='loginText' name="wpEmail" id="wpEmail"
 196+ <input type='<?php echo $wgHtml5 ? 'email' : 'text' ?>' class='loginText' name="wpEmail" id="wpEmail"
177197 tabindex="5"
178198 value="<?php $this->text('email') ?>" size='20' />
179199 <div class="prefsectiontip">
Index: trunk/phase3/RELEASE-NOTES
@@ -71,7 +71,6 @@
7272 interface will not appear in Special:AllMessages.
7373 * $wgRegisterInternalExternals can be used to record external links pointing
7474 to same server
75 -* $wgHtml5 outputs an HTML 5 doctype instead of XHTML 1.0 Transitional.
7675 * $wgSpecialVersionExtended shows the extended version information besides
7776 PHP and database version.
7877 * $wgSecondaryGoNamespaces allows an arry of namespaces to be checked when the
@@ -188,6 +187,13 @@
189188 * wgMainPageTitle variable now available to JavaScript code to identify the main
190189 page link, so it doesn't have to be extracted from the link URLs.
191190 * (bug 16836) Display preview of signature in user preferences and describe its use
 191+* The default output format is now HTML 5 instead of XHTML 1.0 Transitional.
 192+ This can be disabled by setting $wgHtml5 = false;. Specific features enabled
 193+ if HTML 5 is used:
 194+** New HTML 5 input attributes allow JavaScript-free input validation in some
 195+ cutting-edge browsers. E.g., some inputs will be autofocused, users will
 196+ not be allowed to submit forms with certain types of invalid values (like
 197+ numbers outside the permitted ranges), etc.
192198
193199 === Bug fixes in 1.16 ===
194200

Follow-up revisions

RevisionCommit summaryAuthorDate
r55435Only require necessary fields in Special:ResetPass...simetrical21:06, 21 August 2009
r55444Use Html::input() for login form...simetrical21:35, 21 August 2009

Comments

#Comment by Simetrical (talk | contribs)   03:44, 7 August 2009

Actually, WebKit also supports required="" in the very latest builds (since July 17). I've posted a notice on the Mozilla and WebKit Bugzillas:

https://bugzilla.mozilla.org/show_bug.cgi?id=344614 https://bugs.webkit.org/show_bug.cgi?id=19264

#Comment by Simetrical (talk | contribs)   03:45, 7 August 2009

. . . wow, bare URL input is pretty broken on CodeReview, huh?

#Comment by Brion VIBBER (talk | contribs)   05:11, 19 August 2009

I'm not sure how much benefit there is to using 'autofocus' given lack of support; we need to do such focusing in JavaScript to make it actually work on current browsers. (Of course we could change our implementation from the occasional one-off <script> to a standard wikibits.js component which finds the appropriate element with 'autofocus' attribute...?)

#Comment by Simetrical (talk | contribs)   11:02, 19 August 2009

Well, we could also write a JavaScript library to enforce all the other HTML 5 attributes in all browsers. Or we could not, and just let other browsers gracefully degrade. If someone wants to spend the time writing up something in wikibits.js to make autofocus automatically focus, required be required, etc., etc., then great. (I assume there's some easy way to tell from JS whether browsers support these attributes already, like checking for an "autofocus" property.)

This would be more convenient if we were able to drop $wgHtml5 and have it always on, so that we could use autofocus in all cases instead of having to try to figure out how to embed "autofocus me" metadata in XHTML 1.0 while still validating. Then the script could get getElementsByName( "input" ) and check each one for the autofocus attribute, if the browser doesn't already support it. Otherwise we'd have to dump an array of id's in the generated JS variables or something? Messy.

#Comment by Simetrical (talk | contribs)   02:30, 23 August 2009

I should point out that it wouldn't be ideal to only focus the autofocus stuff onload. That seems to be what we do now for Special:Search, but on a slow server, maybe in a hypothetical future where most of our scripts were at the bottom of the page, there could be a significant delay between the input becoming usable and focusing. So it occurs to me that one way to do this would be to just have something like this output immediately after the <input>:

<script>
var focusInput = document.getElementsByName( "$name" );
// If there are multiples, the last one is probably what we want, since this is
// executed synchronously and is immediately after the element in the page
// source
focusInput = focusInput[focusInput.length - 1];
// Don't focus() if the browser already supports autofocus
if ( autofocus not in focusInput ) { focusInput.focus(); }
</script>

. . . probably somewhat more compact and with the comments in the PHP source, of course.

(Is it just me, or does GeSHi not really work here for some reason?)

#Comment by Nikerabbit (talk | contribs)   07:07, 23 August 2009

That would be bug 19474.

#Comment by Simetrical (talk | contribs)   20:00, 23 August 2009

With respect to <input type=email>, see my post to whatwg about it. I think it's validated a little too strictly at present, so I haven't changed our server-side checks, but it should work fine for ~99.9% of our users, and ~99.999% if the user manually removes extra whitespace and <> and (comments) and such. So I think it's fine to keep the client-side email type even if it's slightly too strict right now in Opera (the only browser that supports it).

Status & tagging log