r113272 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r113271‎ | r113272 | r113273 >
Date:19:14, 7 March 2012
Author:krinkle
Status:ok
Tags:
Comment:
[Html::namespaceSelector] Remove default id/name attributes
* Remove default id/name attributes
* Remove now redundant tests introduced in r110274
* Add tests to make sure label has no 'for' attribute when label isn't null but name/id are unset
* Update tests to not include id="" and name="" when calling with no arguments
* Updating calls to add name/id if needed, and while at it remove useless 'null' params


* Context:
-- Introduced in r109990, r111376, r111315
-- No callers exist that assume these defaults. Forcing an ID that should be unique is annoying and redundant. The name used in the query when submitting a form should be mentioned in the same file where it is used from the submission, never assume what name="" is from unrelated code
-- Same for ID, this is often used in CSS or JavaScript, shouldn't be assumed. (It should be simple to but two simple namespace selectors on a page without getting DOM conflicts)
Modified paths:
  • /trunk/phase3/includes/Html.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialContributions.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialPrefixindex.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/HtmlTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/includes/HtmlTest.php
——@@ -215,7 +215,7 @@
216216
217217 function testNamespaceSelector() {
218218 $this->assertEquals(
219 - '<select id="namespace" name="namespace">' . "\n" .
 219+ '<select>' . "\n" .
220220 '<option value="0">(Main)</option>' . "\n" .
221221 '<option value="1">Talk</option>' . "\n" .
222222 '<option value="2">User</option>' . "\n" .
——@@ -236,6 +236,7 @@
237237 Html::namespaceSelector(),
238238 'Basic namespace selector without custom options'
239239 );
 240+
240241 $this->assertEquals(
241242 '<label for="mw-test-namespace">Select a namespace:</label>&#160;' .
242243 '<select id="mw-test-namespace" name="wpNamespace">' . "\n" .
——@@ -263,12 +264,14 @@
264265 ),
265266 'Basic namespace selector with custom values'
266267 );
267 - }
268268
269 - function testCanFilterOutNamespaces() {
270 - $this->assertEquals(
271 -'<select id="namespace" name="namespace">' . "\n" .
 269+ $this->assertEquals(
 270+ '<label>Select a namespace:</label>&#160;' .
 271+'<select>' . "\n" .
 272+'<option value="0">(Main)</option>' . "\n" .
 273+'<option value="1">Talk</option>' . "\n" .
272274 '<option value="2">User</option>' . "\n" .
 275+'<option value="3">User talk</option>' . "\n" .
273276 '<option value="4">MyWiki</option>' . "\n" .
274277 '<option value="5">MyWiki Talk</option>' . "\n" .
275278 '<option value="6">File</option>' . "\n" .
——@@ -279,17 +282,41 @@
280283 '<option value="11">Template talk</option>' . "\n" .
281284 '<option value="14">Category</option>' . "\n" .
282285 '<option value="15">Category talk</option>' . "\n" .
 286+'<option value="100">Custom</option>' . "\n" .
 287+'<option value="101">Custom talk</option>' . "\n" .
283288 '</select>',
284289 Html::namespaceSelector(
 290+ array( 'label' => 'Select a namespace:' )
 291+ ),
 292+ 'Basic namespace selector with a custom label but no id attribtue for the <select>'
 293+ );
 294+ }
 295+
 296+ function testCanFilterOutNamespaces() {
 297+ $this->assertEquals(
 298+'<select>' . "\n" .
 299+'<option value="2">User</option>' . "\n" .
 300+'<option value="4">MyWiki</option>' . "\n" .
 301+'<option value="5">MyWiki Talk</option>' . "\n" .
 302+'<option value="6">File</option>' . "\n" .
 303+'<option value="7">File talk</option>' . "\n" .
 304+'<option value="8">MediaWiki</option>' . "\n" .
 305+'<option value="9">MediaWiki talk</option>' . "\n" .
 306+'<option value="10">Template</option>' . "\n" .
 307+'<option value="11">Template talk</option>' . "\n" .
 308+'<option value="14">Category</option>' . "\n" .
 309+'<option value="15">Category talk</option>' . "\n" .
 310+'</select>',
 311+ Html::namespaceSelector(
285312 array( 'exclude' => array( 0, 1, 3, 100, 101 ) )
286313 ),
287314 'Namespace selector namespace filtering.'
288315 );
289 - }
 316+ }
290317
291 - function testCanDisableANamespaces() {
292 - $this->assertEquals(
293 -'<select id="namespace" name="namespace">' . "\n" .
 318+ function testCanDisableANamespaces() {
 319+ $this->assertEquals(
 320+'<select>' . "\n" .
294321 '<option disabled="" value="0">(Main)</option>' . "\n" .
295322 '<option disabled="" value="1">Talk</option>' . "\n" .
296323 '<option disabled="" value="2">User</option>' . "\n" .
——@@ -314,75 +341,4 @@
315342 );
316343 }
317344
318 - function testNamespaceSelectorIdAndNameDefaultsAttributes() {
319 -
320 - $this->assertNsSelectorIdAndName(
321 - 'namespace', 'namespace',
322 - Html::namespaceSelector( array(), array(
323 - # neither 'id' nor 'name' key given
324 - )),
325 - "Neither 'id' nor 'name' key given"
326 - );
327 -
328 - $this->assertNsSelectorIdAndName(
329 - 'namespace', 'select_name',
330 - Html::namespaceSelector( array(), array(
331 - 'name' => 'select_name',
332 - # no 'id' key given
333 - )),
334 - "No 'id' key given, 'name' given"
335 - );
336 -
337 - $this->assertNsSelectorIdAndName(
338 - 'select_id', 'namespace',
339 - Html::namespaceSelector( array(), array(
340 - 'id' => 'select_id',
341 - # no 'name' key given
342 - )),
343 - "'id' given, no 'name' key given"
344 - );
345 -
346 - $this->assertNsSelectorIdAndName(
347 - 'select_id', 'select_name',
348 - Html::namespaceSelector( array(), array(
349 - 'id' => 'select_id',
350 - 'name' => 'select_name',
351 - )),
352 - "Both 'id' and 'name' given"
353 - );
354 - }
355 -
356 - /**
357 - * Helper to verify <select> attributes generated by Html::namespaceSelector()
358 - * This helper expect the Html method to use 'namespace' as a default value for
359 - * both 'id' and 'name' attributes.
360 - *
361 - * @param String $expectedId <select> id attribute value
362 - * @param String $expectedName <select> name attribute value
363 - * @param String $html Output of a call to Html::namespaceSelector()
364 - * @param String $msg Optional message (default: '')
365 - */
366 - function assertNsSelectorIdAndName( $expectedId, $expectedName, $html, $msg = '' ) {
367 - $actualId = 'namespace';
368 - if( 1 === preg_match( '/id="(.+?)"/', $html, $m ) ) {
369 - $actualId = $m[1];
370 - }
371 -
372 - $actualName = 'namespace';
373 - if( 1 === preg_match( '/name="(.+?)"/', $html, $m ) ) {
374 - $actualName = $m[1];
375 - }
376 - $this->assertEquals(
377 - array( #expected
378 - 'id' => $expectedId,
379 - 'name' => $expectedName,
380 - ),
381 - array( #actual
382 - 'id' => $actualId,
383 - 'name' => $actualName,
384 - ),
385 - 'Html::namespaceSelector() got wrong id and/or name attribute(s). ' . $msg
386 - );
387 - }
388 -
389345 }
Index: trunk/phase3/includes/Html.php
——@@ -718,11 +718,6 @@
719719 public static function namespaceSelector( Array $params = array(), Array $selectAttribs = array() ) {
720720 global $wgContLang;
721721
722 - // Default 'id' & 'name' <select> attributes
723 - $selectAttribs = $selectAttribs + array(
724 - 'id' => 'namespace',
725 - 'name' => 'namespace',
726 - );
727722 ksort( $selectAttribs );
728723
729724 // Is a namespace selected?
——@@ -768,14 +763,22 @@
769764 // main we don't use "" but the user message descripting it (e.g. "(Main)" or "(Article)")
770765 $nsName = wfMsg( 'blanknamespace' );
771766 }
772 - $optionsHtml[] = Xml::option( $nsName, $nsId, $nsId === $params['selected'], array(
773 - 'disabled' => in_array( $nsId, $params['disable'] ),
774 - ) );
 767+ $optionsHtml[] = Html::element(
 768+ 'option', array(
 769+ 'disabled' => in_array( $nsId, $params['disable'] ),
 770+ 'value' => $nsId,
 771+ 'selected' => $nsId === $params['selected'],
 772+ ), $nsName
 773+ );
775774 }
776775
777776 $ret = '';
778777 if ( isset( $params['label'] ) ) {
779 - $ret .= Xml::label( $params['label'], $selectAttribs['id'] ) . '&#160;';
 778+ $ret .= Html::element(
 779+ 'label', array(
 780+ 'for' => isset( $selectAttribs['id'] ) ? $selectAttribs['id'] : null,
 781+ ), $params['label']
 782+ ) . '&#160;';
780783 }
781784
782785 // Wrap options in a <select>
Index: trunk/phase3/includes/specials/SpecialPrefixindex.php
——@@ -109,8 +109,6 @@
110110 <td class='mw-input'>" .
111111 Html::namespaceSelector( array(
112112 'selected' => $namespace,
113 - 'all' => null,
114 - 'label' => null,
115113 ), array(
116114 'name' => 'namespace',
117115 'id' => 'namespace',
Index: trunk/phase3/includes/specials/SpecialContributions.php
——@@ -447,7 +447,6 @@
448448 Html::namespaceSelector( array(
449449 'selected' => $this->opts['namespace'],
450450 'all' => '',
451 - 'label' => null,
452451 ), array(
453452 'name' => 'namespace',
454453 'id' => 'namespace',

Sign-offs

UserFlagDate
Hasharinspected15:21, 8 March 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r109990[Xml/Html] new method Html::namespaceSelector...krinkle03:01, 25 January 2012
r110274test Html::namespaceSelector() id & name attributes generation...hashar10:39, 30 January 2012
r111315[Html.php] Follow-up r110275, comment fixes....krinkle18:58, 12 February 2012
r111376[Html] Follow-up r109990: Add support for excluding and disabling optionskrinkle15:08, 13 February 2012
r113115get rid of deprecated method usage making people on education program test wi...jeroendedauw00:53, 6 March 2012

Status & tagging log