r81585 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r81584‎ | r81585 | r81586 >
Date:03:31, 6 February 2011
Author:krinkle
Status:ok (Comments)
Tags:
Comment:
Three small fixes to mediawiki.specials.preferences.js
* Moving class-adder from mousedown() to click().

Mousedown also fires when opening the contextmenu (right-click). If I right-clicked, say, preftab-4 to open in a new tab, it would work (thanks to r81573), but it would also trigger the 'selected'-class on that tab (which is wrong and out-of-sync since click() did not and should not fire). This behaviour was the case in Safari 5/Firefox 3 (Mac) and Chrome (Windows). Moving it to click() should not have any affect on the behaviour in other browsers.
Test it at: http://api.jquery.com/mousedown/

* Wrapping the hash-jump in a document.ready function.

This module is enclosed in a self-executing anonymous function to enforce private scope passing mediaWiki and jQuery. Although that, (<code>(function($,mw){ /* ... */ })(jQuery,mediaWiki);</code>) may look like a shortcut for jQuery(document).ready(), it is not. (Because both <code>;</code> and <code>jQuery(function{ /* ... */ });</code> look alike and are shortcuts). Anyway, wrapping it now to avoid it jumping too early.

* Using second argument of .each(), which is a reference to 'this'

By using that (named) variable instead there's no confusion with the other 'this' inside the click event binder – which refers to the then-clicked element (the anchor tag) rather than the legend-element.
Modified paths:
  • /trunk/phase3/resources/mediawiki.special/mediawiki.special.preferences.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/mediawiki.special/mediawiki.special.preferences.js
@@ -12,24 +12,22 @@
1313 .addClass( 'prefsection' )
1414 .children( 'legend' )
1515 .addClass( 'mainLegend' )
16 - .each( function( i ) {
17 - $(this).parent().attr( 'id', 'prefsection-' + i );
 16+ .each( function( i, legend ) {
 17+ $(legend).parent().attr( 'id', 'prefsection-' + i );
1818 if ( i === 0 ) {
19 - $(this).parent().show();
 19+ $(legend).parent().show();
2020 }
2121 $( '#preftoc' ).append(
2222 $( '<li></li>' )
2323 .addClass( i === 0 ? 'selected' : null )
2424 .append(
2525 $( '<a></a>')
26 - .text( $(this).text() )
 26+ .text( $(legend).text() )
2727 .attr( 'id', 'preftab-' + i + '-tab' )
2828 .attr( 'href', '#preftab-' + i ) // Use #preftab-N instead of #prefsection-N to avoid jumping on click
29 - .mousedown( function( e ) {
 29+ .click( function() {
3030 $(this).parent().parent().find( 'li' ).removeClass( 'selected' );
31 - $(this).parent().addClass( 'selected' );
32 - } )
33 - .click( function( e ) {
 31+ $(this).parent().addClass( 'selected' )
3432 $( '#preferences > fieldset' ).hide();
3533 $( '#prefsection-' + i ).show();
3634 } )
@@ -40,11 +38,14 @@
4139
4240 // If we've reloaded the page or followed an open-in-new-window,
4341 // make the selected tab visible.
44 -var hash = window.location.hash;
45 -if( hash.match( /^#preftab-[\d]+$/ ) ) {
46 - var tab = $( hash + '-tab' );
47 - tab.mousedown().click();
48 -}
 42+// On document ready:
 43+$( function() {
 44+ var hash = window.location.hash;
 45+ if( hash.match( /^#preftab-[\d]+$/ ) ) {
 46+ var $tab = $( hash + '-tab' );
 47+ $tab.click();
 48+ }
 49+} );
4950
5051 /**
5152 * Given an email validity status (true, false, null) update the label CSS class

Follow-up revisions

RevisionCommit summaryAuthorDate
r819531.17wmf1: MFT r81573, r81585catrope10:11, 11 February 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r81573Preserve Special:Preferences tab state across reload, open in new tab/window....brion21:54, 5 February 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   22:14, 6 February 2011

On the 'jumping early' issue -- we've just done a whole bunch of giant DOM manipulation immediately above, which this is an addition to. All the DOM elements we're dealing with have already been manipulated or added to the page... so if document-ready is an issue there, shouldn't it be an issue for all the code above it too?


On click issue: I believe the classes got added in mousedown rather than click to provide more immediate visual feedback, but it indeed is problematic. (Also didn't fire right when selected by keyboard, which this fixes.)

#Comment by Krinkle (talk | contribs)   23:09, 6 February 2011

Wrapping it all in document ready would probably not brake anything, however I don't think it's needed now and could introduce a FOUC-ish event (preferences appearing underneath eachother and then rebuilding to tabs).

Manipulating the preferences form is save I think given the load order of the elements and the scripts. But hash-jumping really does require the document to be fully rendered since browsers sometimes do funky stuff when refreshing and ignoring it or jumping to the wrong spot (as far as I know this especially goes for (older) gecko browsers, see also the whole redirectToFragment() fancyness in wikibits.js [1])

-- Krinkle

[1] Which reminds me, I still need to port that and then I'll probably use it here.

#Comment by Krinkle (talk | contribs)   23:11, 6 February 2011

Eh nevermind that last part, obviously we're not using it here since this doesn't jump to it, rather it get's the hash value.

Status & tagging log