Restore article scroll after closing Media Viewer

There used to be a CSS trick with the order we added things to the
page and removed them from it, but it doesn't seem possible anymore
with the new order of execution, with the overlay appearing
immediately and being taken care of inside bootstrap.

The main cause of the bug, however, was the hash reset happening
after the interface was closed.

Doing the scroll restore with jQuery.scrollTo is more future-proof
and testable in QUnit.

Additions were also made to the cucumber E2E test because QUnit
alone wouldn't have caught the hash issue.

This also cleans up custom events a little and reintroduces
pushState on browsers that support the history API.

Change-Id: I63187383b632a2e8793f05380c18db2713856865
Mingle: https://wikimedia.mingle.thoughtworks.com/projects/multimedia/cards/439
Bug: 63892

MultimediaViewer.php

@@ -702,6 +702,7 @@ $wgResourceModules += array(
 			'mediawiki.Title',
 			'mmv.logger',
 			'mmv.HtmlUtils',
+			'jquery.scrollTo',
 		),
 
 		'messages' => array(

resources/mmv/mmv.bootstrap.js

@@ -45,6 +45,8 @@
 		this.thumbs = [];
 		this.$thumbs = $( '.gallery .image img, a.image img, #file a img' );
 		this.processThumbs();
+
+		this.browserHistory = window.history;
 	}
 
 	MMVB = MultimediaViewerBootstrap.prototype;
@@ -264,13 +266,26 @@
 
 	/**
 	 * Handles hash change requests coming from mmv
-	 * @param {jQuery.Event} e Custom mmv.hash event
+	 * @param {jQuery.Event} e Custom mmv-hash event
 	 */
 	MMVB.internalHashChange = function ( e ) {
-		// Since we voluntarily changed the hash, we don't want MMVB.hash to treat it
-		this.skipNextHashHandling = true;
+		var hash = e.hash;
 
-		window.location.hash = e.hash;
+		// The advantage of using pushState when it's available is that it has to ability to truly
+		// clear the hash, not leaving "#" in the history
+		// An entry with "#" in the history has the side-effect of resetting the scroll position when navigating the history
+		if ( this.browserHistory ) {
+			// In order to truly clear the hash, we need to reconstruct the hash-free URL
+			if ( hash === '#' ) {
+				hash = window.location.href.replace( /#.*$/, '' );
+			}
+			this.browserHistory.pushState( null, null, hash );
+		} else {
+			// Since we voluntarily changed the hash, we don't want MMVB.hash (which will trigger on hashchange event) to treat it
+			this.skipNextHashHandling = true;
+
+			window.location.hash = hash;
+		}
 	};
 
 	/**
@@ -292,13 +307,16 @@
 	MMVB.setupEventHandlers = function () {
 		var self = this;
 
-		$( window ).hashchange( function () {
+		$( window ).on( this.browserHistory ? 'popstate.mmvb' : 'hashchange', function () {
 			self.hash();
-		} ).hashchange();
+		} );
 
-		$( document ).on( 'mmv.hash', function ( e ) {
+		// Interpret any hash that might already be in the url
+		self.hash();
+
+		$( document ).on( 'mmv-hash', function ( e ) {
 			self.internalHashChange( e );
-		} ).on( 'mmv.close', function () {
+		} ).on( 'mmv-cleanup-overlay', function () {
 			self.cleanupOverlay();
 		} );
 	};
@@ -307,20 +325,31 @@
 	 * Cleans up event handlers, used for tests
 	 */
 	MMVB.cleanupEventHandlers = function () {
-		$( window ).off( 'hashchange' );
-		$( document ).off( 'mmv.hash' );
+		$( window ).off( 'hashchange popstate.mmvb' );
+		$( document ).off( 'mmv-hash' );
 	};
 
 	/**
 	 * Sets up the overlay while the viewer loads
 	 */
 	MMVB.setupOverlay = function () {
+		var $scrollTo = $.scrollTo(),
+			$body = $( document.body );
+
+		// There are situations where we can call setupOverlay while the overlay is already there,
+		// such as inside this.hash(). In that case, do nothing
+		if ( $body.hasClass( 'mw-mmv-lightbox-open' ) ) {
+			return;
+		}
+
 		if ( !this.$overlay ) {
 			this.$overlay = $( '<div>' )
 				.addClass( 'mw-mmv-overlay' );
 		}
 
-		$( document.body ).addClass( 'mw-mmv-lightbox-open' )
+		this.savedScroll = { top : $scrollTo.scrollTop(), left : $scrollTo.scrollLeft() };
+
+		$body.addClass( 'mw-mmv-lightbox-open' )
 			.append( this.$overlay );
 	};
 
@@ -333,6 +362,11 @@
 		if ( this.$overlay ) {
 			this.$overlay.remove();
 		}
+
+		if ( this.savedScroll ) {
+			$.scrollTo( this.savedScroll, 0 );
+			this.savedScroll = undefined;
+		}
 	};
 
 	MMVB.whenThumbsReady = function () {

resources/mmv/mmv.js

@@ -613,14 +613,15 @@
 	 * Handles close event coming from the lightbox
 	 */
 	MMVP.close = function () {
-		$( document ).trigger( $.Event( 'mmv.close' ) );
-
 		if ( comingFromHashChange === false ) {
-			$( document ).trigger( $.Event( 'mmv.hash', { hash : '#' } ) );
+			$( document ).trigger( $.Event( 'mmv-hash', { hash : '#' } ) );
 		} else {
 			comingFromHashChange = false;
 		}
 
+		// This has to happen after the hash reset, because setting the hash to # will reset the page scroll
+		$( document ).trigger( $.Event( 'mmv-cleanup-overlay' ) );
+
 		this.isOpen = false;
 	};
 
@@ -634,7 +635,7 @@
 		if ( linkState[0] === '#mediaviewer' ) {
 			this.loadImageByTitle( linkState[ 1 ] );
 		} else if ( this.isOpen ) {
-			// This allows us to avoid the mmv.hash event that normally happens on close
+			// This allows us to avoid the mmv-hash event that normally happens on close
 			comingFromHashChange = true;
 
 			if ( this.ui ) {
@@ -649,19 +650,13 @@
 	MMVP.setHash = function() {
 		if ( !this.comingFromHashChange ) {
 			var hashFragment = '#mediaviewer/' + this.currentImageFilename;
-			$( document ).trigger( $.Event( 'mmv.hash', { hash : hashFragment } ) );
+			$( document ).trigger( $.Event( 'mmv-hash', { hash : hashFragment } ) );
 		}
 	};
 
-	/**
-	 * @event mmv.close
-	 * Fired when the viewer should be closed. This is used by components (e.g. the close button)
-	 * to notify the main app that it should close.
-	 */
 	/**
 	 * @event mmv-close
-	 * Fired when the viewer is closed. This is used by the main app to notify other components
-	 * (notably the bootstrap).
+	 * Fired when the viewer is closed. This is used by the ligthbox to notify the main app.
 	 */
 	/**
 	 * @event mmv-next

resources/mmv/mmv.lightboxinterface.js

@@ -121,13 +121,9 @@
 
 		// Advanced description needs to be below the fold when the lightbox opens
 		// regardless of what the scroll value was prior to opening the lightbox
-
-		// 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 = $.scrollTo().scrollTop();
+		// If the lightbox is already attached, it means we're doing prev/next, and
+		// we should avoid scrolling the panel
+		if ( !this.attached ) {
 			$.scrollTo( 0, 0 );
 		}
 
@@ -188,17 +184,14 @@
 
 		// Reset the cursor fading
 		this.fadeStopped();
+
+		this.attached = true;
 	};
 
 	/**
 	 * Detaches the interface from the DOM.
 	 */
 	LIP.unattach = function () {
-		// We trigger this event on the document because unattach() can run
-		// when the interface is unattached
-		$( document ).trigger( $.Event( 'mmv-close' ) )
-			.off( 'jq-fullscreen-change.lip' );
-
 		this.$wrapper.detach();
 
 		this.currentlyAttached = false;
@@ -207,15 +200,16 @@
 
 		this.canvas.unattach();
 
-		// Restore the scrollTop as it was before opening the lightbox
-		if ( this.scrollTopBeforeAttach !== undefined ) {
-			$.scrollTo( this.scrollTopBeforeAttach, 0 );
-			this.scrollTopBeforeAttach = undefined;
-		}
-
 		this.panel.fileReuse.closeDialog();
 
 		this.clearEvents();
+
+		// We trigger this event on the document because unattach() can run
+		// when the interface is unattached
+		$( document ).trigger( $.Event( 'mmv-close' ) )
+			.off( 'jq-fullscreen-change.lip' );
+
+		this.attached = false;
 	};
 
 	/**

tests/browser/features/basic_mmv_navigation.feature

@@ -23,3 +23,4 @@ Feature: Basic Multimedia Viewer navigation
     Then the image and metadata of the previous image should appear
     When I close MMV
     Then I should be navigated back to the original wiki article
+    Then the wiki article should be scrolled to the same position as before opening MMV

tests/browser/features/step_definitions/basic_mmv_navigation_steps.rb

@@ -5,6 +5,10 @@ end
 
 When(/^I click on the first image in the article$/) do
   on(LightboxDemoPage) do |page|
+    # Scroll the article on purpose
+    page.execute_script "window.scroll(10, 100)"
+    @articleScrollTop = page.execute_script("return $(window).scrollTop();")
+    @articleScrollLeft = page.execute_script("return $(window).scrollLeft();")
     page.image1_in_article
   end
 end
@@ -61,6 +65,13 @@ Then(/^the image and metadata of the previous image should appear$/) do
   end
 end
 
+Then(/^the wiki article should be scrolled to the same position as before opening MMV$/) do
+  on(LightboxDemoPage) do |page|
+    page.execute_script("return $(window).scrollTop();").should eq @articleScrollTop
+    page.execute_script("return $(window).scrollLeft();").should eq @articleScrollLeft
+  end
+end
+
 # Helper function that verifies the presence of various elements in viewer
 # while looking at image1 (Kerala)
 def check_elements_in_viewer_for_image1(page)

tests/browser/features/support/pages/lightbox_demo_page.rb

@@ -7,7 +7,7 @@ class LightboxDemoPage
   # Tag page elements that we will need.
 
   # First image in lightbox demo page
-  a(:image1_in_article, href: /Kerala\.jpg$/)
+  a(:image1_in_article, class: "image", href: /Kerala\.jpg$/)
 
   # Wrapper div for all mmv elements
   div(:mmv_wrapper, class: "mw-mmv-wrapper")

tests/qunit/mmv/mmv.bootstrap.test.js

@@ -27,6 +27,31 @@
 		return bootstrap;
 	}
 
+	function hashTest( bootstrap, assert ) {
+		var hash = 'mediaviewer/foo';
+
+		bootstrap.setupEventHandlers();
+
+		bootstrap.loadViewer = function () {
+			assert.ok( false, 'Viewer should not be loaded' );
+			return $.Deferred().reject();
+		};
+
+		window.location.hash = 'Foo';
+
+		bootstrap.loadViewer = function () {
+			QUnit.start();
+			assert.ok( true, 'Viewer should be loaded' );
+			bootstrap.cleanupEventHandlers();
+			window.location.hash = '';
+
+			return $.Deferred().reject();
+		};
+
+		QUnit.stop();
+		window.location.hash = hash;
+	}
+
 	QUnit.test( 'Promise does not hang on ResourceLoader errors', 3, function ( assert ) {
 		var oldUsing = mw.loader.using,
 			bootstrap,
@@ -229,58 +254,47 @@
 		$link.trigger( { type : 'click', which : 1 } );
 	} );
 
-	QUnit.asyncTest( 'Only load the viewer on a valid hash', 1, function ( assert ) {
+	QUnit.test( 'Only load the viewer on a valid hash (modern browsers)', 1, function ( assert ) {
 		var bootstrap;
 
 		window.location.hash = '';
 
 		bootstrap = createBootstrap();
-		bootstrap.setupEventHandlers();
 
-		bootstrap.loadViewer = function () {
-			assert.ok( false, 'Viewer should not be loaded' );
-			return $.Deferred().reject();
-		};
+		hashTest( bootstrap, assert );
+	} );
 
-		window.location.hash = 'Foo';
+	QUnit.test( 'Only load the viewer on a valid hash (old browsers)', 1, function ( assert ) {
+		var bootstrap;
 
-		bootstrap.loadViewer = function () {
-			QUnit.start();
-			assert.ok( true, 'Viewer should be loaded' );
-			bootstrap.cleanupEventHandlers();
-			window.location.hash = '';
-			return $.Deferred().reject();
-		};
+		window.location.hash = '';
 
-		window.location.hash = 'mediaviewer/foo';
+		bootstrap = createBootstrap();
+		bootstrap.browserHistory = undefined;
+
+		hashTest( bootstrap, assert );
 	} );
 
 	QUnit.test( 'internalHashChange', 1, function ( assert ) {
+		var bootstrap = createBootstrap(),
+			hash = '#mediaviewer/foo';
+
 		window.location.hash = '';
 
-		var bootstrap = createBootstrap(),
-			hash = '#mediaviewer/foo',
-			oldHash = bootstrap.hash;
-
 		bootstrap.setupEventHandlers();
 
-		bootstrap.hash = function () {
-			oldHash.call( this );
-
-			bootstrap.cleanupEventHandlers();
-			window.location.hash = '';
-			QUnit.start();
-		};
-
 		bootstrap.loadViewer = function () {
 			assert.ok( false, 'Viewer should not be loaded' );
 			return $.Deferred().reject();
 		};
 
-		QUnit.stop();
 		bootstrap.internalHashChange( { hash: hash } );
 
 		assert.strictEqual( window.location.hash, hash, 'Window\'s hash has been updated correctly' );
+
+		bootstrap.cleanupEventHandlers();
+
+		window.location.hash = '';
 	} );
 
 	QUnit.test( 'isCSSReady', 3, function ( assert ) {
@@ -310,4 +324,32 @@
 
 		$style.appendTo( 'head' );
 	} );
+
+	QUnit.test( 'Restoring article scroll position', 2, function ( assert ) {
+		var bootstrap = createBootstrap(),
+			scrollTop = 50,
+			scrollLeft = 60,
+			stubbedScrollTop = scrollTop,
+			stubbedScrollLeft = scrollLeft;
+
+		this.sandbox.stub( $, 'scrollTo', function ( target ) {
+			if ( target ) {
+				stubbedScrollTop = target.top;
+				stubbedScrollLeft = target.left;
+			} else {
+				return {
+					scrollTop : function () { return stubbedScrollTop; },
+					scrollLeft : function () { return stubbedScrollLeft; }
+				};
+			}
+		} );
+
+		bootstrap.setupOverlay();
+		// Calling this a second time because it can happen in history navigation context
+		bootstrap.setupOverlay();
+		bootstrap.cleanupOverlay();
+
+		assert.strictEqual( stubbedScrollTop, scrollTop, 'Scroll is correctly reset to original top position' );
+		assert.strictEqual( stubbedScrollLeft, scrollLeft, 'Scroll is correctly reset to original left position' );
+	} );
 }( mediaWiki, jQuery ) );

tests/qunit/mmv/mmv.lightboxinterface.test.js

@@ -240,11 +240,10 @@
 		restoreScrollTo();
 	} );
 
-	QUnit.test( 'Metadata scrolling', 15, function ( assert ) {
+	QUnit.test( 'Metadata scrolling', 14, function ( assert ) {
 		var ui = new mw.mmv.LightboxInterface(),
 			keydown = $.Event( 'keydown' ),
 			$document = $( document ),
-			scrollTopBeforeOpeningLightbox,
 			originalJQueryScrollTop = $.fn.scrollTop,
 			memorizedScrollToScroll = 0,
 			originalJQueryScrollTo = $.scrollTo;
@@ -339,11 +338,6 @@
 
 		// Second phase of the test: scroll memory
 
-		// Scroll down a little bit to check that the scroll memory works
-		$.scrollTo( 10, 0 );
-
-		scrollTopBeforeOpeningLightbox = $.scrollTo().scrollTop();
-
 		// Attach lightbox to testing fixture to avoid interference with other tests.
 		ui.attach( '#qunit-fixture' );
 
@@ -362,9 +356,6 @@
 		// Unattach lightbox from document
 		ui.unattach();
 
-		// Lightbox is supposed to restore the document scrollTop value that was set prior to opening it
-		assert.strictEqual( $.scrollTo().scrollTop(), scrollTopBeforeOpeningLightbox, 'document scrollTop value has been restored correctly' );
-
 		// Let's restore all originals, to make sure this test is free of side-effect
 		$.fn.scrollTop = originalJQueryScrollTop;
 		$.scrollTo = originalJQueryScrollTo;