From 380612c4bfe9185cf44c4325f22b07f34f5380b4 Mon Sep 17 00:00:00 2001
From: Gilles Dubuc <gdubuc@wikimedia.org>
Date: Thu, 9 Jan 2014 17:44:00 +0100
Subject: [PATCH] Only scroll to the top when opening the lightbox

Not when going prev/next.
The saved position feature was probably broken as well if you
happened to press prev/next.
This was my mistake for not noticing that attach() runs on
prev/next.

Bug: 59861
Change-Id: Ic6ff4b15a54178fb5d38640317650f5676293083
---
 .../ext.multimediaViewer.lightboxinterface.js   | 13 +++++++++----
 ...t.multimediaViewer.lightboxinterface.test.js | 17 +++++++++++++++--
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/resources/ext.multimediaViewer/ext.multimediaViewer.lightboxinterface.js b/resources/ext.multimediaViewer/ext.multimediaViewer.lightboxinterface.js
index 673212d90..d0d3bcd69 100644
--- a/resources/ext.multimediaViewer/ext.multimediaViewer.lightboxinterface.js
+++ b/resources/ext.multimediaViewer/ext.multimediaViewer.lightboxinterface.js
@@ -94,10 +94,14 @@
 		// regardless of what the scroll value was prior to opening the lightbox
 		var $document = $( document );
 
-		// Save the scrollTop value because we want below to be back to where they were
-		// before opening the lightbox
-		this.scrollTopBeforeAttach = $document.scrollTop();
-		$document.scrollTop( 0 );
+		// Only scroll and save the position if it's the first attach
+		// Otherwise it could be an attach event happening because of prev/next
+		if ( this.scrollTopBeforeAttach === undefined ) {
+			// Save the scrollTop value because we want below to be back to where they were
+			// before opening the lightbox
+			this.scrollTopBeforeAttach = $document.scrollTop();
+			$document.scrollTop( 0 );
+		}
 
 		MLBInterface.prototype.attach.call( this, parentId );
 	};
@@ -108,6 +112,7 @@
 		// Restore the scrollTop as it was before opening the lightbox
 		if ( this.scrollTopBeforeAttach !== undefined ) {
 			$( document ).scrollTop( this.scrollTopBeforeAttach );
+			this.scrollTopBeforeAttach = undefined;
 		}
 	};
 
diff --git a/tests/qunit/ext.multimediaViewer.lightboxinterface.test.js b/tests/qunit/ext.multimediaViewer.lightboxinterface.test.js
index d28698fa4..f6efda52f 100644
--- a/tests/qunit/ext.multimediaViewer.lightboxinterface.test.js
+++ b/tests/qunit/ext.multimediaViewer.lightboxinterface.test.js
@@ -22,8 +22,10 @@
 
 	QUnit.module( 'ext.multimediaViewer.lightboxInterface', QUnit.newMwEnvironment() );
 
-	QUnit.test( 'Sanity test, object creation and ui construction', 12, function ( assert ) {
-		var lightbox = new mw.LightboxInterface();
+	QUnit.test( 'Sanity test, object creation and ui construction', 14, function ( assert ) {
+		var lightbox = new mw.LightboxInterface(),
+			$document = $( document ),
+			scrollTopBeforeOpeningLightbox;
 
 		function checkIfUIAreasAttachedToDocument( inDocument ) {
 			var msg = inDocument === 1 ? ' ' : ' not ';
@@ -33,18 +35,29 @@
 			assert.strictEqual( $( '.mw-mlb-image-links' ).length, inDocument, 'Links area' + msg + 'attached.' );
 		}
 
+		// Scroll down a little bit to check that the scroll memory works
+		$document.scrollTop( 10 );
+
+		scrollTopBeforeOpeningLightbox = $document.scrollTop();
+
 		// UI areas not attached to the document yet.
 		checkIfUIAreasAttachedToDocument(0);
 
 		// Attach lightbox to testing fixture to avoid interference with other tests.
 		lightbox.attach( '#qunit-fixture' );
 
+		// To make sure that the details are out of view, the lightbox is supposed to scroll to the top when open
+		assert.strictEqual( $document.scrollTop(), 0, 'Document scrollTop should be set to 0' );
+
 		// UI areas should now be attached to the document.
 		checkIfUIAreasAttachedToDocument(1);
 
 		// Unattach lightbox from document
 		lightbox.unattach();
 
+		// Lightbox is supposed to restore the document scrollTop value that was set prior to opening it
+		assert.strictEqual( $document.scrollTop(), scrollTopBeforeOpeningLightbox, 'document scrollTop value has been restored correctly' );
+
 		// UI areas not attached to the document anymore.
 		checkIfUIAreasAttachedToDocument(0);
 	} );