r59832 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r59831‎ | r59832 | r59833 >
Date:17:17, 8 December 2009
Author:dantman
Status:ok (Comments)
Tags:
Comment:
Fixes for comments on r59655
- Look for any textbox since just creating one doesn't work
- editFormHeadInit -> showHeader
- code style
- moved checkbox initialization into relevant locations
- dropped hardcoded use of currentFocused in favor of catching event delegated focus events within the edit form

Also:
- Removed more direct uses of $wgRequest in favor of properties
- add $wgPreviewOnOpenNamespaces for extensions like SMW that have category like namespaces.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/EditPage.php (modified) (history)
  • /trunk/phase3/skins/common/edit.js (modified) (history)

Diff [purge]

Index: trunk/phase3/RELEASE-NOTES
——@@ -100,6 +100,8 @@
101101 file repository
102102 * $wgWikiId added to override default output of wfWikiId()
103103 * $wgDBAhandler added to choose a DBA handler when using CACHE_DBA
 104+* $wgPreviewOnOpenNamespaces for extensions that create namespaces that behave
 105+ similarly to the category namespace.
104106
105107 === New features in 1.16 ===
106108
Index: trunk/phase3/includes/EditPage.php
——@@ -72,9 +72,9 @@
7373 # Form values
7474 var $save = false, $preview = false, $diff = false;
7575 var $minoredit = false, $watchthis = false, $recreate = false;
76 - var $textbox1 = '', $textbox2 = '', $summary = '';
 76+ var $textbox1 = '', $textbox2 = '', $summary = '', $nosummary = false;
7777 var $edittime = '', $section = '', $starttime = '';
78 - var $oldid = 0, $editintro = '', $scrolltop = null;
 78+ var $oldid = 0, $editintro = '', $scrolltop = null, $bot = true;
7979
8080 # Placeholders for text injection by hooks (must be HTML)
8181 # extensions should take care to _append_ to the present value
——@@ -534,7 +534,7 @@
535535 * @return bool
536536 */
537537 protected function previewOnOpen() {
538 - global $wgRequest, $wgUser;
 538+ global $wgRequest, $wgUser, $wgPreviewOnOpenNamespaces;
539539 if ( $wgRequest->getVal( 'preview' ) == 'yes' ) {
540540 // Explicit override from request
541541 return true;
——@@ -547,7 +547,10 @@
548548 } elseif ( ( $wgRequest->getVal( 'preload' ) !== null || $this->mTitle->exists() ) && $wgUser->getOption( 'previewonfirst' ) ) {
549549 // Standard preference behaviour
550550 return true;
551 - } elseif ( !$this->mTitle->exists() && $this->mTitle->getNamespace() == NS_CATEGORY ) {
 551+ } elseif ( !$this->mTitle->exists() &&
 552+ isset($wgPreviewOnOpenNamespaces[$this->mTitle->getNamespace()]) &&
 553+ $wgPreviewOnOpenNamespaces[$this->mTitle->getNamespace()] )
 554+ {
552555 // Categories are special
553556 return true;
554557 } else {
——@@ -684,7 +687,7 @@
685688 $this->save = false;
686689 $this->diff = false;
687690 $this->minoredit = false;
688 - $this->watchthis = false;
 691+ $this->watchthis = $request->getBool( 'watchthis', false ); // Watch may be overriden by request parameters
689692 $this->recreate = false;
690693
691694 if ( $this->section == 'new' && $request->getVal( 'preloadtitle' ) ) {
——@@ -699,6 +702,9 @@
700703 }
701704 }
702705
 706+ $this->bot = $request->getBool( 'bot', true );
 707+ $this->nosummary = $request->getBool( 'nosummary' );
 708+
703709 // FIXME: unused variable?
704710 $this->oldid = $request->getInt( 'oldid' );
705711
——@@ -1174,8 +1180,22 @@
11751181 * Called on the first invocation, e.g. when a user clicks an edit link
11761182 */
11771183 function initialiseForm() {
 1184+ global $wgUser;
11781185 $this->edittime = $this->mArticle->getTimestamp();
11791186 $this->textbox1 = $this->getContent( false );
 1187+ // activate checkboxes if user wants them to be always active
 1188+ # Sort out the "watch" checkbox
 1189+ if ( $wgUser->getOption( 'watchdefault' ) ) {
 1190+ # Watch all edits
 1191+ $this->watchthis = true;
 1192+ } elseif ( $wgUser->getOption( 'watchcreations' ) && !$this->mTitle->exists() ) {
 1193+ # Watch creations
 1194+ $this->watchthis = true;
 1195+ } elseif ( $this->mTitle->userIsWatching() ) {
 1196+ # Already watched
 1197+ $this->watchthis = true;
 1198+ }
 1199+ if ( $wgUser->getOption( 'minordefault' ) ) $this->minoredit = true;
11801200 if ( $this->textbox1 === false ) return false;
11811201 wfProxyCheck();
11821202 return true;
——@@ -1211,7 +1231,7 @@
12121232 * near the top, for captchas and the like.
12131233 */
12141234 function showEditForm( $formCallback=null ) {
1215 - global $wgOut, $wgUser, $wgTitle, $wgRequest;
 1235+ global $wgOut, $wgUser, $wgTitle;
12161236
12171237 # If $wgTitle is null, that means we're in API mode.
12181238 # Some hook probably called this function without checking
——@@ -1240,7 +1260,7 @@
12411261 # Enabled article-related sidebar, toplinks, etc.
12421262 $wgOut->setArticleRelated( true );
12431263
1244 - if ( $this->editFormHeadInit() === false )
 1264+ if ( $this->showHeader() === false )
12451265 return;
12461266
12471267 $action = htmlspecialchars($this->getActionURL($wgTitle));
——@@ -1253,28 +1273,6 @@
12541274 }
12551275
12561276
1257 - // activate checkboxes if user wants them to be always active
1258 - if ( !$this->preview && !$this->diff ) {
1259 - # Sort out the "watch" checkbox
1260 - if ( $wgUser->getOption( 'watchdefault' ) ) {
1261 - # Watch all edits
1262 - $this->watchthis = true;
1263 - } elseif ( $wgUser->getOption( 'watchcreations' ) && !$this->mTitle->exists() ) {
1264 - # Watch creations
1265 - $this->watchthis = true;
1266 - } elseif ( $this->mTitle->userIsWatching() ) {
1267 - # Already watched
1268 - $this->watchthis = true;
1269 - }
1270 -
1271 - # May be overriden by request parameters
1272 - if( $wgRequest->getBool( 'watchthis' ) ) {
1273 - $this->watchthis = true;
1274 - }
1275 -
1276 - if ( $wgUser->getOption( 'minordefault' ) ) $this->minoredit = true;
1277 - }
1278 -
12791277 $wgOut->addHTML( $this->editFormPageTop );
12801278
12811279 if ( $wgUser->getOption( 'previewontop' ) ) {
——@@ -1333,7 +1331,7 @@
13341332 # For a bit more sophisticated detection of blank summaries, hash the
13351333 # automatic one and pass that in the hidden field wpAutoSummary.
13361334 if ( $this->missingSummary ||
1337 - ( $this->section == 'new' && $wgRequest->getBool( 'nosummary' ) ) )
 1335+ ( $this->section == 'new' && $this->nosummary ) )
13381336 $wgOut->addHTML( Xml::hidden( 'wpIgnoreBlankSummary', true ) );
13391337 $autosumm = $this->autoSumm ? $this->autoSumm : md5( $this->summary );
13401338 $wgOut->addHTML( Xml::hidden( 'wpAutoSummary', $autosumm ) );
——@@ -1395,7 +1393,7 @@
13961394 wfProfileOut( __METHOD__ );
13971395 }
13981396
1399 - protected function editFormHeadInit() {
 1397+ protected function showHeader() {
14001398 global $wgOut, $wgParser, $wgUser, $wgMaxArticleSize, $wgLang;
14011399 if ( $this->isConflict ) {
14021400 $wgOut->wrapWikiMsg( "<div class='mw-explainconflict'>\n$1</div>", 'explainconflict' );
——@@ -1422,20 +1420,25 @@
14231421 }
14241422 }
14251423
1426 - if ( $this->missingComment )
 1424+ if ( $this->missingComment ) {
14271425 $wgOut->wrapWikiMsg( '<div id="mw-missingcommenttext">$1</div>', 'missingcommenttext' );
 1426+ }
14281427
1429 - if ( $this->missingSummary && $this->section != 'new' )
 1428+ if ( $this->missingSummary && $this->section != 'new' ) {
14301429 $wgOut->wrapWikiMsg( '<div id="mw-missingsummary">$1</div>', 'missingsummary' );
 1430+ }
14311431
1432 - if ( $this->missingSummary && $this->section == 'new' )
 1432+ if ( $this->missingSummary && $this->section == 'new' ) {
14331433 $wgOut->wrapWikiMsg( '<div id="mw-missingcommentheader">$1</div>', 'missingcommentheader' );
 1434+ }
14341435
1435 - if ( $this->hookError !== '' )
 1436+ if ( $this->hookError !== '' ) {
14361437 $wgOut->addWikiText( $this->hookError );
 1438+ }
14371439
1438 - if ( !$this->checkUnicodeCompliantBrowser() )
 1440+ if ( !$this->checkUnicodeCompliantBrowser() ) {
14391441 $wgOut->addWikiMsg( 'nonunicodebrowser' );
 1442+ }
14401443
14411444 if ( isset( $this->mArticle ) && isset( $this->mArticle->mRevision ) ) {
14421445 // Let sysop know that this will make private content public if saved
——@@ -1501,8 +1504,9 @@
15021505 $wgOut->wrapWikiMsg( '<div class="mw-titleprotectedwarning">$1</div>', 'titleprotectedwarning' );
15031506 }
15041507
1505 - if ( $this->kblength === false )
 1508+ if ( $this->kblength === false ) {
15061509 $this->kblength = (int)( strlen( $this->textbox1 ) / 1024 );
 1510+ }
15071511
15081512 if ( $this->tooBig || $this->kblength > $wgMaxArticleSize ) {
15091513 $wgOut->addHTML( "<div class='error' id='mw-edit-longpageerror'>\n" );
——@@ -1529,23 +1533,19 @@
15301534 *
15311535 * @return array An array in the format array( $label, $input )
15321536 */
1533 - function getSummaryInput($summary = "", $labelText = null, $userInputAttrs = null, $userSpanLabelAttrs = null) {
1534 - $inputAttrs = array(
 1537+ function getSummaryInput($summary = "", $labelText = null, $inputAttrs = null, $spanLabelAttrs = null) {
 1538+ $inputAttrs = ( is_array($inputAttrs) ? $inputAttrs : array() ) + array(
15351539 'id' => 'wpSummary',
15361540 'maxlength' => '200',
15371541 'tabindex' => '1',
15381542 'size' => 60,
15391543 'spellcheck' => 'true',
1540 - 'onfocus' => "currentFocused = this;", // Make wpSummary insertable for editbuttons
15411544 );
1542 - if ( $userInputAttrs )
1543 - $inputAttrs += $userInputAttrs;
1544 - $spanLabelAttrs = array(
 1545+
 1546+ $spanLabelAttrs = ( is_array($spanLabelAttrs) ? $spanLabelAttrs : array() ) + array(
15451547 'class' => $this->missingSummary ? 'mw-summarymissed' : 'mw-summary',
15461548 'id' => "wpSummaryLabel"
15471549 );
1548 - if ( is_array($userSpanLabelAttrs) )
1549 - $spanLabelAttrs += $userSpanLabelAttrs;
15501550
15511551 $label = null;
15521552 if ( $labelText ) {
——@@ -1566,11 +1566,11 @@
15671567 * @return string
15681568 */
15691569 protected function showSummaryInput( $isSubjectPreview, $summary = "" ) {
1570 - global $wgOut, $wgContLang, $wgRequest;
 1570+ global $wgOut, $wgContLang;
15711571 # Add a class if 'missingsummary' is triggered to allow styling of the summary line
15721572 $summaryClass = $this->missingSummary ? 'mw-summarymissed' : 'mw-summary';
15731573 if ( $isSubjectPreview ) {
1574 - if ( $wgRequest->getBool( 'nosummary' ) )
 1574+ if ( $this->nosummary )
15751575 return;
15761576 } else {
15771577 if ( !$this->mShowSummaryField )
——@@ -1710,7 +1710,6 @@
17111711 'id' => $name,
17121712 'cols' => $wgUser->getIntOption( 'cols' ),
17131713 'rows' => $wgUser->getIntOption( 'rows' ),
1714 - 'onfocus' => "currentFocused = this;", // Make textareas insertable for editbuttons
17151714 'style' => '' // avoid php notices when appending for editwidth preference (appending allows customAttribs['style'] to still work
17161715 );
17171716
——@@ -1999,7 +1998,7 @@
20001999 * Call the stock "user is blocked" page
20012000 */
20022001 function blockedPage() {
2003 - global $wgOut, $wgUser;
 2002+ global $wgOut;
20042003 $wgOut->blockedPage( false ); # Standard block notice on the top, don't 'return'
20052004
20062005 # If the user made changes, preserve them when showing the markup
——@@ -2013,14 +2012,9 @@
20142013
20152014 # Spit out the source or the user's modified version
20162015 if ( $source !== false ) {
2017 - $rows = $wgUser->getIntOption( 'rows' );
2018 - $cols = $wgUser->getIntOption( 'cols' );
2019 - $attribs = array( 'id' => 'wpTextbox1', 'name' => 'wpTextbox1', 'cols' => $cols, 'rows' => $rows, 'readonly' => 'readonly' );
20202016 $wgOut->addHTML( '<hr />' );
20212017 $wgOut->addWikiMsg( $first ? 'blockedoriginalsource' : 'blockededitsource', $this->mTitle->getPrefixedText() );
2022 - # Why we don't use Xml::element here?
2023 - # Is it because if $source is '', it returns <textarea />?
2024 - $wgOut->addHTML( Xml::openElement( 'textarea', $attribs ) . htmlspecialchars( $source ) . Xml::closeElement( 'textarea' ) );
 2018+ $this->showTextbox1( array( 'readonly' ), $source );
20252019 }
20262020 }
20272021
——@@ -2616,11 +2610,11 @@
26172611 * @return bool false if output is done, true if the rest of the form should be displayed
26182612 */
26192613 function attemptSave() {
2620 - global $wgUser, $wgOut, $wgTitle, $wgRequest;
 2614+ global $wgUser, $wgOut, $wgTitle;
26212615
26222616 $resultDetails = false;
26232617 # Allow bots to exempt some edits from bot flagging
2624 - $bot = $wgUser->isAllowed( 'bot' ) && $wgRequest->getBool( 'bot', true );
 2618+ $bot = $wgUser->isAllowed( 'bot' ) && $this->bot;
26252619 $value = $this->internalAttemptSave( $resultDetails, $bot );
26262620
26272621 if ( $value == self::AS_SUCCESS_UPDATE || $value == self::AS_SUCCESS_NEW_ARTICLE ) {
Index: trunk/phase3/includes/DefaultSettings.php
——@@ -2162,6 +2162,15 @@
21632163 NS_CATEGORY_TALK => true
21642164 );
21652165
 2166+/**
 2167+ * Which namespaces have special treatment where they should be preview-on-open
 2168+ * Internaly only Category: pages apply, but using this extensions (e.g. Semantic MediaWiki)
 2169+ * can specify namespaces of pages they have special treatment for
 2170+ */
 2171+$wgPreviewOnOpenNamespaces = array(
 2172+ NS_CATEGORY => true
 2173+);
 2174+
21662175 $wgNamespacesToBeSearchedDefault = array(
21672176 NS_MAIN => true,
21682177 );
Index: trunk/phase3/skins/common/edit.js
——@@ -46,9 +46,14 @@
4747
4848 // Don't generate buttons for browsers which don't fully
4949 // support it.
50 - var textbox = document.createElement('textarea'); // abstract, don't assume wpTextbox1 is always there
 50+ // but don't assume wpTextbox1 is always here
 51+ var textboxes = document.getElementsByTagName('textarea');
 52+ if ( !textboxes.length ) {
 53+ // No toolbar if we can't find any textarea
 54+ return false;
 55+ }
5156 if (!(document.selection && document.selection.createRange)
52 - && textbox.selectionStart === null) {
 57+ && textboxes[0].selectionStart === null) {
5358 return false;
5459 }
5560
——@@ -160,5 +165,35 @@
161166 hookEvent( 'load', mwSetupToolbar );
162167 hookEvent( 'load', function() {
163168 currentFocused = document.getElementById( 'wpTextbox1' );
 169+ // http://www.quirksmode.org/blog/archives/2008/04/delegating_the.html
 170+ // focus does not bubble normally, but using a trick we can do event delegation
 171+ // on the focus event on all text inputs to make the toolbox usable on all of them
 172+ var editForm = document.getElementById( 'editform' );
 173+ if ( !editForm )
 174+ return;
 175+
 176+ function onfocus(e) {
 177+ var elm = e.target;
 178+ if ( !elm )
 179+ return;
 180+ var tagName = elm.tagName.toLowerCase();
 181+ var type = elm.type.toLowerCase();
 182+ if ( tagName !== "textarea" && tagName !== "input" )
 183+ return;
 184+ if ( tagName === "input" && type && type !== "text" )
 185+ return;
 186+
 187+ currentFocused = elm;
 188+ }
 189+
 190+ if ( editForm.addEventListener ) {
 191+ // Gecko, WebKit, Opera, etc... (all standards compliant browsers)
 192+ editForm.addEventListener('focus', onfocus, true); // This MUST be true to work
 193+ } else if ( editForm.attachEvent ) {
 194+ // IE needs a specific trick here since it doesn't support the standard
 195+ editForm.attachEvent( 'onfocusin', function() { onfocus(event); }, handler );
 196+ }
 197+
 198+ editForm
164199 } );
165200

Follow-up revisions

RevisionCommit summaryAuthorDate
r59907Removed the extra parameter from the attachEvent call, which only takes 2 par...tparscal21:57, 9 December 2009
r83123Code conventions, cross-browser fixes and JSHint validation...krinkle22:54, 2 March 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r59655EditPage refactor and improvements....dantman07:22, 2 December 2009

Comments

#Comment by Trevor Parscal (WMF) (talk | contribs)   20:50, 9 December 2009

In edit.js on line 194, where did the handler variable come from? I'm getting errors in IE (since it's IE specific code).

#Comment by Krinkle (talk | contribs)   22:45, 2 March 2011 
+++ trunk/phase3/skins/common/edit.js	(revision 59832)
...
+	editForm

JSHint warning: "editForm" Expected an assignment or function call and instead saw an expression.

What's that variable doing there ?

#Comment by Dantman (talk | contribs)   22:49, 2 March 2011

Oh, I think you can get rid of that. My editor+os+trackpad combination has a random habit of pasting the clipboard into random places of code, even in locations different from where the cursor is placed... that was probably inserted there without my knowing.

#Comment by Krinkle (talk | contribs)   22:55, 2 March 2011

Fixed in r83123.

Status & tagging log