r112573 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r112572‎ | r112573 | r112574 >
Date:03:17, 28 February 2012
Author:krinkle
Status:ok (Comments)
Tags:tstarling 
Comment:
[mediawiki.action.edit] Clean up and bug fixes
* Bugfix: Locally alias the toolbar object and replace usage of 'this' with 'toolbar'. Calls to window.addButton were failing due to it referring to the dynamic 'this' context which changes when the function is a member of a different object.
* Bugfux: Move currentFocused = $( '#wpTextbox1' ) into the dom-ready hook. When executed before dom-ready the element doesn't exist yet. r111983 did this for $toolbar, but not for currentFocused (bug 34538)

* Move onReady and isReady to the local scope (introduced recently in r111983, not used or meant to be used publicly)
* Merge onReady with the $(document).ready hook function, same purpose
* JS conventions (closure arguments, var hoisting, whitespace)


* Touch r111983, r112451, r112567
Modified paths:
  • /trunk/phase3/resources/mediawiki.action/mediawiki.action.edit.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/mediawiki.action/mediawiki.action.edit.js
@@ -1,79 +1,96 @@
2 -(function( $ ) {
3 - // currentFocus is used to determine where to insert tags
4 - var currentFocused = $( '#wpTextbox1' );
 2+( function ( $, mw ) {
 3+ var isReady, toolbar, currentFocused;
54
6 - mw.toolbar = {
7 - $toolbar : false,
8 - buttons : [],
9 - isReady : false,
10 - // If you want to add buttons, use
11 - // mw.toolbar.addButton( imageFile, speedTip, tagOpen, tagClose, sampleText, imageId, selectText );
12 - addButton : function() {
13 - if ( this.isReady ) {
14 - this.insertButton.apply( this, arguments );
 5+ isReady = false;
 6+
 7+ toolbar = {
 8+ $toolbar: false,
 9+ buttons: [],
 10+ /**
 11+ * If you want to add buttons, use
 12+ * mw.toolbar.addButton( imageFile, speedTip, tagOpen, tagClose, sampleText, imageId, selectText );
 13+ */
 14+ addButton: function () {
 15+ if ( isReady ) {
 16+ toolbar.insertButton.apply( toolbar, arguments );
1517 } else {
16 - this.buttons.push( [].slice.call( arguments ) );
 18+ toolbar.buttons.push( [].slice.call( arguments ) );
1719 }
1820 },
19 - insertButton : function( imageFile, speedTip, tagOpen, tagClose, sampleText, imageId, selectText ) {
 21+ insertButton: function ( imageFile, speedTip, tagOpen, tagClose, sampleText, imageId, selectText ) {
2022 var image = $('<img>', {
21 - width : 23,
22 - height : 22,
23 - src : imageFile,
24 - alt : speedTip,
25 - title : speedTip,
26 - id : imageId || '',
 23+ width : 23,
 24+ height: 22,
 25+ src : imageFile,
 26+ alt : speedTip,
 27+ title : speedTip,
 28+ id : imageId || '',
2729 'class': 'mw-toolbar-editbutton'
28 - } ).click( function() {
 30+ } ).click( function () {
2931 mw.toolbar.insertTags( tagOpen, tagClose, sampleText, selectText );
3032 return false;
3133 } );
3234
33 - this.$toolbar.append( image );
 35+ toolbar.$toolbar.append( image );
3436 return true;
3537 },
3638
37 - // apply tagOpen/tagClose to selection in textarea,
38 - // use sampleText instead of selection if there is none
39 - insertTags : function( tagOpen, tagClose, sampleText, selectText) {
40 - if ( currentFocused.length ) {
 39+ /**
 40+ * apply tagOpen/tagClose to selection in textarea,
 41+ * use sampleText instead of selection if there is none.
 42+ */
 43+ insertTags: function ( tagOpen, tagClose, sampleText, selectText ) {
 44+ if ( currentFocused && currentFocused.length ) {
4145 currentFocused.textSelection(
42 - 'encapsulateSelection', { 'pre': tagOpen, 'peri': sampleText, 'post': tagClose }
 46+ 'encapsulateSelection', {
 47+ 'pre': tagOpen,
 48+ 'peri': sampleText,
 49+ 'post': tagClose
 50+ }
4351 );
4452 }
4553 },
4654
4755 // For backwards compatibility
48 - init : function() {},
 56+ init: function () {}
 57+ };
4958
50 - onReady : function() {
51 - this.$toolbar = $( '#toolbar' );
52 - this.isReady = true;
53 - // Legacy
54 - // Merge buttons from mwCustomEditButtons
55 - var buttons = [].concat( this.buttons, window.mwCustomEditButtons );
56 - for ( var i = 0; i < buttons.length; i++ ) {
57 - if ( $.isArray( buttons[i] ) ) {
58 - // Passes our button array as arguments
59 - this.insertButton.apply( this, buttons[i] );
60 - } else {
61 - // Legacy mwCustomEditButtons is an object
62 - var c = buttons[i];
63 - this.insertButton( c.imageFile, c.speedTip, c.tagOpen,
64 - c.tagClose, c.sampleText, c.imageId, c.selectText );
65 - }
 59+ // Legacy (for compatibility with the code previously in skins/common.edit.js)
 60+ window.addButton = toolbar.addButton;
 61+ window.insertTags = toolbar.insertTags;
 62+
 63+ // Explose publicly
 64+ mw.toolbar = toolbar;
 65+
 66+ $( document ).ready( function () {
 67+ var buttons, i, c, iframe;
 68+
 69+ // currentFocus is used to determine where to insert tags
 70+ currentFocused = $( '#wpTextbox1' );
 71+
 72+ // Populate the selector cache for $toolbar
 73+ toolbar.$toolbar = $( '#toolbar' );
 74+
 75+ // Legacy: Merge buttons from mwCustomEditButtons
 76+ buttons = [].concat( toolbar.buttons, window.mwCustomEditButtons );
 77+ for ( i = 0; i < buttons.length; i++ ) {
 78+ if ( $.isArray( buttons[i] ) ) {
 79+ // Passes our button array as arguments
 80+ toolbar.insertButton.apply( toolbar, buttons[i] );
 81+ } else {
 82+ // Legacy mwCustomEditButtons is an object
 83+ c = buttons[i];
 84+ toolbar.insertButton( c.imageFile, c.speedTip, c.tagOpen,
 85+ c.tagClose, c.sampleText, c.imageId, c.selectText );
6686 }
67 - return true;
6887 }
69 - };
7088
71 - //Legacy
72 - window.addButton = mw.toolbar.addButton;
73 - window.insertTags = mw.toolbar.insertTags;
 89+ // This causes further calls to addButton to go to insertion directly
 90+ // instead of to the toolbar.buttons queue.
 91+ // It is important that this is after the one and only loop through
 92+ // the the toolbar.buttons queue
 93+ isReady = true;
7494
75 - $( document ).ready( function() {
76 - mw.toolbar.onReady();
77 -
7895 // Make sure edit summary does not exceed byte limit
7996 $( '#wpSummary' ).byteLimit( 250 );
8097
@@ -81,32 +98,37 @@
8299 * Restore the edit box scroll state following a preview operation,
83100 * and set up a form submission handler to remember this state
84101 */
85 - var scrollEditBox = function() {
86 - var editBox = document.getElementById( 'wpTextbox1' );
87 - var scrollTop = document.getElementById( 'wpScrolltop' );
88 - var $editForm = $( '#editform' );
89 - if( $editForm.length && editBox && scrollTop ) {
90 - if( scrollTop.value ) {
 102+ ( function scrollEditBox() {
 103+ var editBox, scrollTop, $editForm;
 104+
 105+ editBox = document.getElementById( 'wpTextbox1' );
 106+ scrollTop = document.getElementById( 'wpScrolltop' );
 107+ $editForm = $( '#editform' );
 108+ if ( $editForm.length && editBox && scrollTop ) {
 109+ if ( scrollTop.value ) {
91110 editBox.scrollTop = scrollTop.value;
92111 }
93 - $editForm.submit( function() {
 112+ $editForm.submit( function () {
94113 scrollTop.value = editBox.scrollTop;
95114 });
96115 }
97 - };
98 - scrollEditBox();
 116+ }() );
99117
100 - $( 'textarea, input:text' ).focus( function() {
 118+ $( 'textarea, input:text' ).focus( function () {
101119 currentFocused = $(this);
102120 });
103121
104122 // HACK: make currentFocused work with the usability iframe
105123 // With proper focus detection support (HTML 5!) this'll be much cleaner
106 - var iframe = $( '.wikiEditor-ui-text iframe' );
 124+ iframe = $( '.wikiEditor-ui-text iframe' );
107125 if ( iframe.length > 0 ) {
108126 $( iframe.get( 0 ).contentWindow.document )
109 - .add( iframe.get( 0 ).contentWindow.document.body ) // for IE
110 - .focus( function() { currentFocused = iframe; } );
 127+ // for IE
 128+ .add( iframe.get( 0 ).contentWindow.document.body )
 129+ .focus( function () {
 130+ currentFocused = iframe;
 131+ } );
111132 }
112133 });
113 -})(jQuery);
 134+
 135+}( jQuery, mediaWiki ) );

Follow-up revisions

RevisionCommit summaryAuthorDate
r113174MFT r111795, r111881, r111920, r112573, r112995, r113169reedy20:12, 6 March 2012
r113175MFT r111795, r111881, r111920, r112573, r112995, r113169reedy20:13, 6 March 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r111983* Fixed failure of the edit toolbar to load when $wgResourceLoaderExperimenta...tstarling23:29, 20 February 2012
r111984Bug 34538 again. Bug reproduced and fix tested.tstarling23:47, 20 February 2012
r111985* Fixed load order like bug 34538...tstarling00:00, 21 February 2012
r111989* (bug 34538) Fixed compatibility with $wgResourceLoaderExperimentalAsyncLoad...tstarling04:03, 21 February 2012
r112451* Fix r111983 (bug 34662): make mw.toolbar.addButton() work even after DOM ready...tstarling22:46, 26 February 2012
r112462(bug 34538) Fixed compatibility with $wgResourceLoaderExperimentalAsyncLoadin...tstarling00:56, 27 February 2012
r112567Fix scoping in r112451: isReady was accidentally used as a global variablecatrope02:36, 28 February 2012

Comments

#Comment by Reedy (talk | contribs)   17:58, 6 March 2012

Presumably this should be tagged 1.19wmf1 also?

Status & tagging log