Merge "Restore article scroll after closing Media Viewer"

MultimediaViewer.php

@@ -698,6 +698,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

@@ -614,14 +614,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;
 	};
 
@@ -635,7 +636,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 ) {
@@ -650,19 +651,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;