From cc8dae6518bfccaa369a3ed47accf5e108d84625 Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinklemail@gmail.com>
Date: Wed, 1 Oct 2014 16:12:55 -0700
Subject: [PATCH] mediawiki.util: Clean up nextnode logic

Change-Id: I21469a765002897f03caca97628ad36264d49c83
---
 resources/src/mediawiki/mediawiki.util.js     | 30 +++++++++++--------
 .../mediawiki/mediawiki.util.test.js          | 10 +++----
 2 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/resources/src/mediawiki/mediawiki.util.js b/resources/src/mediawiki/mediawiki.util.js
index 2662913781..fe5f2b2376 100644
--- a/resources/src/mediawiki/mediawiki.util.js
+++ b/resources/src/mediawiki/mediawiki.util.js
@@ -290,26 +290,30 @@
 			}
 
 			if ( nextnode ) {
+				// Case: nextnode is a DOM element (was the only option before MW 1.17, in wikibits.js)
+				// Case: nextnode is a CSS selector for jQuery
 				if ( nextnode.nodeType || typeof nextnode === 'string' ) {
-					// nextnode is a DOM element (was the only option before MW 1.17, in wikibits.js)
-					// or nextnode is a CSS selector for jQuery
 					nextnode = $ul.find( nextnode );
-				} else if ( !nextnode.jquery || ( nextnode.length && nextnode[0].parentNode !== $ul[0] ) ) {
-					// Fallback
-					$ul.append( $item );
-					return $item[0];
+				} else if ( !nextnode.jquery ) {
+					// Error: Invalid nextnode
+					nextnode = undefined;
 				}
-				if ( nextnode.length === 1 ) {
-					// nextnode is a jQuery object that represents exactly one element
-					nextnode.before( $item );
-					return $item[0];
+				if ( nextnode && ( nextnode.length !== 1 || nextnode[0].parentNode !== $ul[0] ) ) {
+					// Error: nextnode must resolve to a single node
+					// Error: nextnode must have the associated <ul> as its parent
+					nextnode = undefined;
 				}
 			}
 
-			// Fallback (this is the default behavior)
-			$ul.append( $item );
-			return $item[0];
+			// Case: nextnode is a jQuery-wrapped DOM element
+			if ( nextnode ) {
+				nextnode.before( $item );
+			} else {
+				// Fallback (this is the default behavior)
+				$ul.append( $item );
+			}
 
+			return $item[0];
 		},
 
 		/**
diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.util.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.util.test.js
index 4401eadb57..9b620de406 100644
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.util.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.util.test.js
@@ -189,7 +189,7 @@
 		);
 
 		assert.equal( $tbMW.closest( '.portlet' ).attr( 'id' ), 'p-test-tb', 'Link was inserted within correct portlet' );
-		assert.strictEqual( $tbMW.next()[0], tbRL, 'Link is in the correct position (by passing nextnode)' );
+		assert.strictEqual( $tbMW.next()[0], tbRL, 'Link is in the correct position (nextnode as Node object)' );
 
 		cuQuux = mw.util.addPortletLink( 'p-test-custom', '#', 'Quux', null, 'Example [shift-x]', 'q' );
 		$cuQuux = $( cuQuux );
@@ -205,7 +205,7 @@
 		tbRLDM = mw.util.addPortletLink( 'p-test-tb', '//mediawiki.org/wiki/RL/DM',
 			'Default modules', 't-rldm', 'List of all default modules ', 'd', '#t-rl' );
 
-		assert.equal( $( tbRLDM ).next().attr( 'id' ), 't-rl', 'Link is in the correct position (by passing CSS selector)' );
+		assert.strictEqual( $( tbRLDM ).next()[0], tbRL, 'Link is in the correct position (CSS selector as nextnode)' );
 
 		caFoo = mw.util.addPortletLink( 'p-test-views', '#', 'Foo' );
 
@@ -213,19 +213,19 @@
 		assert.strictEqual( $( caFoo ).find( 'span' ).length, 1, 'A <span> element should be added for porlets with vectorTabs class.' );
 
 		addedAfter = mw.util.addPortletLink( 'p-test-tb', '#', 'After foo', 'post-foo', 'After foo', null, $( tbRL ) );
-		assert.strictEqual( $( addedAfter ).next()[0], tbRL, 'Link is in the correct position (by passing a jQuery object as nextnode)' );
+		assert.strictEqual( $( addedAfter ).next()[0], tbRL, 'Link is in the correct position (jQuery object as nextnode)' );
 
 		// test case - nonexistent id as next node
 		tbRLDMnonexistentid = mw.util.addPortletLink( 'p-test-tb', '//mediawiki.org/wiki/RL/DM',
 			'Default modules', 't-rldm-nonexistent', 'List of all default modules ', 'd', '#t-rl-nonexistent' );
 
-		assert.equal( tbRLDMnonexistentid, $( '#p-test-tb li:last' )[0], 'Nonexistent id as nextnode adds the portlet at end' );
+		assert.equal( tbRLDMnonexistentid, $( '#p-test-tb li:last' )[0], 'Fallback to adding at the end (nextnode non-matching CSS selector)' );
 
 		// test case - empty jquery object as next node
 		tbRLDMemptyjquery = mw.util.addPortletLink( 'p-test-tb', '//mediawiki.org/wiki/RL/DM',
 			'Default modules', 't-rldm-empty-jquery', 'List of all default modules ', 'd', $( '#t-rl-nonexistent' ) );
 
-		assert.equal( tbRLDMemptyjquery, $( '#p-test-tb li:last' )[0], 'Empty jquery as nextnode adds the portlet at end' );
+		assert.equal( tbRLDMemptyjquery, $( '#p-test-tb li:last' )[0], 'Fallback to adding at the end (nextnode as empty jQuery object)' );
 	} );
 
 	QUnit.test( 'jsMessage', 1, function ( assert ) {
-- 
2.20.1