r42514 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r42513‎ | r42514 | r42515 >
Date:23:10, 24 October 2008
Author:mrzman
Status:old (Comments)
Tags:
Comment:
(bug 16073) * Use onclick handler for expand/collapse in enhanced recentchanges.
* Hide the expandable content with JavaScript for better fallback if JS is disabled for whatever reason.
* Move enhancedchanges JS to separate file.
* Less cryptic id names.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/ChangesList.php (modified) (history)
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/skins/common/enhancedchanges.js (added) (history)
  • /trunk/phase3/skins/common/wikibits.js (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/wikibits.js
——@@ -104,22 +104,6 @@
105105 }
106106 }
107107
108 -// for enhanced RecentChanges
109 -function toggleVisibility(_levelId, _otherId, _linkId) {
110 - var thisLevel = document.getElementById(_levelId);
111 - var otherLevel = document.getElementById(_otherId);
112 - var linkLevel = document.getElementById(_linkId);
113 - if (thisLevel.style.display == 'none') {
114 - thisLevel.style.display = 'block';
115 - otherLevel.style.display = 'none';
116 - linkLevel.style.display = 'inline';
117 - } else {
118 - thisLevel.style.display = 'none';
119 - otherLevel.style.display = 'inline';
120 - linkLevel.style.display = 'none';
121 - }
122 -}
123 -
124108 function showTocToggle() {
125109 if (document.createTextNode) {
126110 // Uses DOM calls to avoid document.write + XHTML issues
Index: trunk/phase3/skins/common/enhancedchanges.js
——@@ -0,0 +1,33 @@
 2+/*
 3+ JavaScript file for enhanced recentchanges
 4+ */
 5+
 6+/*
 7+ * onload hook to add the CSS to hide parts that should be collapsed
 8+ *
 9+ * We do this with JS so everything will be expanded by default
 10+ * if JS is disabled
 11+ */
 12+addOnloadHook(function () {
 13+ var css = ".mw-rc-jshidden { display:none; }";
 14+ appendCSS(css);
 15+});
 16+
 17+/*
 18+ * Switch an RC line between hidden/shown
 19+ * @param int idNumber : the id number of the RC group
 20+*/
 21+function toggleVisibility(idNumber) {
 22+ var openarrow = document.getElementById("mw-rc-openarrow-"+idNumber);
 23+ var closearrow = document.getElementById("mw-rc-closearrow-"+idNumber);
 24+ var subentries = document.getElementById("mw-rc-subentries-"+idNumber);
 25+ if (openarrow.style.display == 'inline') {
 26+ openarrow.style.display = 'none';
 27+ closearrow.style.display = 'inline';
 28+ subentries.style.display = 'block';
 29+ } else {
 30+ openarrow.style.display = 'inline';
 31+ closearrow.style.display = 'none';
 32+ subentries.style.display = 'none';
 33+ }
 34+}
Property changes on: trunk/phase3/skins/common/enhancedchanges.js
___________________________________________________________________
Added: svn:eol-style
135 + native
Index: trunk/phase3/includes/ChangesList.php
——@@ -388,7 +388,24 @@
389389 * Generate a list of changes using an Enhanced system (use javascript).
390390 */
391391 class EnhancedChangesList extends ChangesList {
 392+
392393 /**
 394+ * Add the JavaScript file for enhanced changeslist
 395+ * @ return string
 396+ */
 397+ public function beginRecentChangesList() {
 398+ global $wgStylePath, $wgStyleVersion;
 399+ $this->rc_cache = array();
 400+ $this->rcMoveIndex = 0;
 401+ $this->rcCacheIndex = 0;
 402+ $this->lastdate = '';
 403+ $this->rclistOpen = false;
 404+ $script = Xml::tags( 'script', array(
 405+ 'type' => 'text/javascript',
 406+ 'src' => $wgStylePath . "/common/enhancedchanges.js?$wgStyleVersion" ), '' );
 407+ return $script;
 408+ }
 409+ /**
393410 * Format a line for enhanced recentchange (aka with javascript and block of lines).
394411 */
395412 public function recentChangesLine( &$baseRC, $watched = false ) {
——@@ -596,13 +613,12 @@
597614
598615 $users = ' <span class="changedby">[' . implode( $this->message['semicolon-separator'], $users ) . ']</span>';
599616
600 - # Arrow
601 - $rci = 'RCI'.$this->rcCacheIndex;
602 - $rcl = 'RCL'.$this->rcCacheIndex;
603 - $rcm = 'RCM'.$this->rcCacheIndex;
604 - $toggleLink = "javascript:toggleVisibility('$rci','$rcm','$rcl')";
605 - $tl = '<span id="'.$rcm.'"><a href="'.$toggleLink.'">' . $this->sideArrow() . '</a></span>';
606 - $tl .= '<span id="'.$rcl.'" style="display:none"><a href="'.$toggleLink.'">' . $this->downArrow() . '</a></span>';
 617+ # ID for JS visibility toggle
 618+ $jsid = $this->rcCacheIndex;
 619+
 620+ $toggleLink = "onclick='toggleVisibility($jsid)'";
 621+ $tl = "<span id='mw-rc-openarrow-$jsid' style='display:inline;' $toggleLink>" . $this->sideArrow() . "</span>";
 622+ $tl .= "<span id='mw-rc-closearrow-$jsid' style='display:none;' class='mw-rc-jshidden' $toggleLink>" . $this->downArrow() . "</span>";
607623 $r .= '<td valign="top" style="white-space: nowrap"><tt>'.$tl.'&nbsp;';
608624
609625 # Main line
——@@ -680,7 +696,7 @@
681697 $r .= "</td></tr></table>\n";
682698
683699 # Sub-entries
684 - $r .= '<div id="'.$rci.'" style="display:none;"><table cellpadding="0" cellspacing="0" border="0" style="background: none">';
 700+ $r .= '<div id="mw-rc-subentries-'.$jsid.'" class="mw-rc-jshidden"><table cellpadding="0" cellspacing="0" border="0" style="background: none">';
685701 foreach( $block as $rcObj ) {
686702 # Get rc_xxxx variables
687703 // FIXME: Would be good to replace this extract() call with something that explicitly initializes local variables.
Index: trunk/phase3/includes/DefaultSettings.php
——@@ -1385,7 +1385,7 @@
13861386 * to ensure that client-side caches don't keep obsolete copies of global
13871387 * styles.
13881388 */
1389 -$wgStyleVersion = '182';
 1389+$wgStyleVersion = '183';
13901390
13911391
13921392 # Server-side caching:
Index: trunk/phase3/RELEASE-NOTES
——@@ -283,6 +283,8 @@
284284 * (bug 14609) User's namespaces to be searched default not updated after adding new namespace
285285 * Purge form uses valid XHTML and (bug 8992) uses $wgRequest instead of $_SERVER
286286 * (bug 12764) Special:LonelyPages shows transcluded pages
 287+* (bug 16073) Enhanced RecentChanges uses onclick handler with better fallback if
 288+ JavaScript is disabled.
287289
288290 === API changes in 1.14 ===
289291

Follow-up revisions

RevisionCommit summaryAuthorDate
r42521Revert r42514 for now "(bug 16073) * Use onclick handler for expand/collapse ...brion00:10, 25 October 2008
r42528Re-committing r42514:...mrzman02:27, 25 October 2008

Comments

#Comment by Brion VIBBER (talk | contribs)   00:10, 25 October 2008

I've reverted this for now as it has some issues:

  • The expand/contract icons no longer have regular link behavior (eg no hand icon, can't be reached by keyboard tabbing)
  • It looks like the stuff-to-be-hidden doesn't get hidden until after the </body>, which feels a little sketchy to me. On long lists and slow connections you may see odd behavior with the items being shown expanded, then suddenly hiding when things reach the end. Adding the style immediately in the JS instead of waiting for the body load completion should avoid that
  • mw-rc-jshidden class seems to be applied to things that shouldn't have it sincec they are explicitly given display:none?
  • Instead of style='display:none' etc, consider using clear classes for expanded and hidden modes, then switch the classes instead of the styles in the JS

Reverted in r42521

#Comment by Danny B. (talk | contribs)   00:16, 25 October 2008

Switching from <a> to <span> is very bad solution, since it decreases the accessibility of the page. All actions should be placed on <a> tags, so user immediately knows, which items on page are active. Use the correct code for javascript-only links: <a href="#" onclick="toggleVisibility('RCI0','RCM0','RCL0')">. Also don't forget to use the title attribute to display the description such as "toggle the details (needs JavaScript)" or so.

Try to turn off the stylesheets and see.

See also bug 13984.

(with edit conflict with Brion, regardless on what he wrote)

#Comment by Mr.Z-man (talk | contribs)   02:31, 25 October 2008

Okay re-done in r42528:

  • switched back to <a> tags, now with title attributes.
  • load the CSS immediately when the JS loads rather than 'onload'
  • better use of classes to control visibility.

Status & tagging log