r74740 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r74739‎ | r74740 | r74741 >
Date:21:58, 13 October 2010
Author:tparscal
Status:ok (Comments)
Tags:
Comment:
Fixes bug in wikibits that causes addPortletLink to fail when adding tabs to the vector skin.
Modified paths:
  • /trunk/phase3/skins/common/wikibits.js (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/wikibits.js
——@@ -352,15 +352,16 @@
353353 // unhide portlet if it was hidden before
354354 root.className = root.className.replace( /(^| )emptyPortlet( |$)/, "$2" );
355355
356 - var span = document.createElement( 'span' );
357 - span.appendChild( document.createTextNode( text ) );
358 -
359356 var link = document.createElement( 'a' );
360 - link.appendChild( span );
 357+ link.appendChild( document.createTextNode( text ) );
361358 link.href = href;
362359
 360+ // Wrap in a span - make it work with vector tabs and has no effect on any other portlets
 361+ var span = document.createElement( 'span' );
 362+ span.appendChild( link );
 363+
363364 var item = document.createElement( 'li' );
364 - item.appendChild( link );
 365+ item.appendChild( span );
365366 if ( id ) {
366367 item.id = id;
367368 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r74741Integrated r74740.tparscal22:02, 13 October 2010

Comments

#Comment by MaxSem (talk | contribs)   18:24, 12 November 2010

The extra <span> breaks existing site/user/gadget JS that relies on specific content of portlets, for example MediaWiki:Gadget-UTCLiveClock.js.

#Comment by Catrope (talk | contribs)   13:24, 6 January 2011

Wouldn't those things break for all Vector tabs anyway? Or should this be an if ( skin == 'vector' ) thing?

#Comment by Brion VIBBER (talk | contribs)   07:48, 19 January 2011

The versions of UTCLiveClock.js currently on www.mediawiki.org and en.wikipedia.org both seem to work fine for me on trunk, with both monobook and vector...

aha, ok it looks like it's meant to be clickable as an action=purge, which is failing.

I'm inclined to agree with note below that any gadgets broken by this simply need to be fixed. The problem here is that it assumes the first child node of the <li> is a <a>; it could just as easily be a comment or whitespace node preceding the link even if there's no difference in the structure of elements!

The gadget should instead use destNode.getElementsByTagName('a')[0] to get the link, which I can verify works in both monobook and vector with trunk code.

Note that moving the spans outside of the links is also good because that lets gadgets safely alter the contents of a portlet link without breaking the structure. innerHTML or better yet, jQuery's .text() for instance will handily change the text once you've got that link -- eg $('#pt-liveclock a').text(newdate). So it's a good thing. :D

#Comment by Krinkle (talk | contribs)   15:02, 1 February 2011

The script has been updated accordingly. The bug wasn't in addPortletLink, becuase that just returns the DOM element for the added list item. Later in the code it then uses firstChild to replace the text, which removed the anchor tag inside the <span>.

Fixed now at MediaWiki:Gadget-UTCLiveClock.js and tested in 1.16/1.17/trunk with Monobook and Vector.

Marking this revision as ok.

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:31, 6 January 2011

We have to maintain special casing for both "a span text /span /a" and "span a text /a /span" to support both skins.

#Comment by Ilmari Karonen (talk | contribs)   18:49, 6 January 2011

Hmm, looks like the Vector tab structure must've changed somewhere between r56045 and this revision. Annoying.

Anyway, I don't really see any feasible way to deal with scripts that assume too much about how the tabs are structured except to fix them to be less picky. In this particular case, I wrote an improved replacement for UTCLiveClock a long time ago which should work on all tabbed skins (and I just tested that it still does work on Vector).

#Comment by Trevor Parscal (WMF) (talk | contribs)   19:11, 6 January 2011

Probably to do with making right-clicking work on IE - using a span inside of an a caused issues.

#Comment by Krinkle (talk | contribs)   12:46, 23 January 2011

Whatever is decided, don't forget to update the new addPortletLink() in /resources/mediawiki.util/mediawiki.util.js accordingly.

#Comment by Catrope (talk | contribs)   14:44, 2 February 2011

Putting this back to OK: per Brion's comment, scripts breaking due to assuming a portlet link has a very specific structure have themselves to blame, and should find the a element in a more flexible way.

Status & tagging log