Router handling code should be centralized into mmv.bootstrap

- mediawiki.router is available inside mmv.bootstrap so move
the handlers into the bootstrap file
- Remove direct loading of images from mmv.js instead modify
event handlers to modify the location which will trigger the loading of
the image
- Copy what we previously did in MobileFrontend (Bartosz's idea)
to allow page refreshing and escaping overlay on page load.
- Remove some no longer relevant tests

Bug: T236591
Change-Id: Ie5d33fca5174db58380a63f5cf97f44e5df10d42

resources/mmv.bootstrap/mmv.bootstrap.js

@@ -17,6 +17,12 @@
 
 ( function () {
 	var MMVB;
+	var mwRouter = require( 'mediawiki.router' );
+
+	// We pass this to history.pushState/replaceState to indicate that we're controlling the page URL.
+	// Then we look for this marker on page load so that if the page is refreshed, we don't generate an
+	// extra history entry.
+	var MANAGED_STATE = 'MMV was here!';
 
 	/**
 	 * Bootstrap code listening to thumb clicks checking the initial location.hash
@@ -60,10 +66,44 @@
 		// this will run initially and then every time the content changes,
 		// e.g. via a VE edit or pagination in a multipage file
 		mw.hook( 'wikipage.content' ).add( this.processThumbs.bind( this ) );
+
+		// Setup the router
+		this.setupRouter( mwRouter );
 	}
 
 	MMVB = MultimediaViewerBootstrap.prototype;
 
+	/**
+	 * Routes to a given file.
+	 *
+	 * @param {string} fileName
+	 */
+	MMVB.route = function ( fileName ) {
+		this.loadViewer( true ).then( function ( viewer ) {
+			var fileTitle;
+			viewer.comingFromHashChange = true;
+			try {
+				fileName = decodeURIComponent( fileName );
+				fileTitle = new mw.Title( fileName );
+				viewer.loadImageByTitle( fileTitle );
+			} catch ( err ) {
+				// ignore routes to invalid titles
+				mw.log.warn( err );
+			}
+		} );
+	};
+
+	/**
+	 * Sets up the route handlers
+	 *
+	 * @param {OO.Router} router
+	 */
+	MMVB.setupRouter = function ( router ) {
+		router.addRoute( mw.mmv.ROUTE_REGEXP, this.route.bind( this ) );
+		router.addRoute( mw.mmv.LEGACY_ROUTE_REGEXP, this.route.bind( this ) );
+		this.router = router;
+	};
+
 	/**
 	 * Loads the mmv module asynchronously and passes the thumb data to it
 	 *
@@ -284,7 +324,7 @@
 			caption: this.findCaption( $thumbContainer, $link ) } );
 
 		$link.add( $enlarge ).on( 'click', function ( e ) {
-			return bs.click( this, e, title );
+			return bs.click( e, title );
 		} );
 	};
 
@@ -342,7 +382,7 @@
 		} );
 
 		$link.on( 'click', function ( e ) {
-			return bs.click( this, e, title );
+			return bs.click( e, title );
 		} );
 	};
 
@@ -516,28 +556,25 @@
 	/**
 	 * Opens MediaViewer and loads the given thumbnail. Requires processThumb() to be called first.
 	 *
-	 * @param {HTMLElement} element Clicked element
 	 * @param {mw.Title} title File title
 	 * @return {jQuery.Promise}
 	 */
-	MMVB.openImage = function ( element, title ) {
+	MMVB.openImage = function ( title ) {
 		this.ensureEventHandlersAreSetUp();
-
-		return this.loadViewer( true ).then( function ( viewer ) {
-			viewer.loadImageByTitle( title, false );
-		} );
+		var hash = mw.mmv.getMediaHash( title );
+		location.hash = hash;
+		history.replaceState( MANAGED_STATE, null, hash );
 	};
 
 	/**
 	 * Handles a click event on a link
 	 *
-	 * @param {HTMLElement} element Clicked element
 	 * @param {jQuery.Event} e jQuery event object
 	 * @param {mw.Title} title File title
 	 * @return {boolean} a value suitable for an event handler (ie. true if the click should be handled
 	 *  by the browser).
 	 */
-	MMVB.click = function ( element, e, title ) {
+	MMVB.click = function ( e, title ) {
 		// Do not interfere with non-left clicks or if modifier keys are pressed.
 		if ( ( e.button !== 0 && e.which !== 1 ) || e.altKey || e.ctrlKey || e.shiftKey || e.metaKey ) {
 			return true;
@@ -553,7 +590,8 @@
 			return true;
 		}
 
-		this.openImage( element, title );
+		// Mark the state so that if the page is refreshed, we don't generate an extra history entry
+		this.openImage( title );
 
 		// calling this late so that in case of errors users at least get to the file page
 		e.preventDefault();
@@ -576,22 +614,21 @@
 	 * Handles the browser location hash on pageload or hash change
 	 */
 	MMVB.hash = function () {
-		var bootstrap = this;
+		var isViewerHash = this.isViewerHash();
 
 		// There is no point loading the mmv if it isn't loaded yet for hash changes unrelated to the mmv
 		// Such as anchor links on the page
-		if ( !this.viewerInitialized && !this.isViewerHash() ) {
+		if ( !this.viewerInitialized && !isViewerHash ) {
 			return;
 		}
 
-		this.loadViewer( this.isViewerHash() ).then( function ( viewer ) {
-			viewer.router.checkRoute();
-			// this is an ugly temporary fix to avoid a black screen of death when
-			// the page is loaded with an invalid MMV url
-			if ( !viewer.isOpen ) {
-				bootstrap.cleanupOverlay();
-			}
-		} );
+		var hash = location.hash;
+		if ( window.history.state !== MANAGED_STATE ) {
+			// First replace the current URL with a URL with a hash.
+			history.replaceState( null, null, '#' );
+			history.pushState( MANAGED_STATE, null, hash );
+		}
+		this.router.checkRoute();
 	};
 
 	/**

resources/mmv/mmv.js

@@ -252,10 +252,9 @@
 	 * Loads a specified image.
 	 *
 	 * @param {mw.mmv.LightboxImage} image
-	 * @param {HTMLImageElement} initialImage A thumbnail to use as placeholder while the image loads
-	 * @param {boolean} useReplaceState Whether to update history entry to avoid long history queues
+	 * @param {HTMLImageElement} initialImage A thumbnail to use as placeholder while the image loadsx
 	 */
-	MMVP.loadImage = function ( image, initialImage, useReplaceState ) {
+	MMVP.loadImage = function ( image, initialImage ) {
 		var imageWidths,
 			imagePromise,
 			metadataPromise,
@@ -279,7 +278,7 @@
 			this.ui.empty();
 		}
 
-		this.setMediaHash( useReplaceState );
+		this.setTitle();
 
 		// At this point we can't show the thumbnail because we don't
 		// know what size it should be. We still assign it to allow for
@@ -382,9 +381,8 @@
 	 * Loads an image by its title
 	 *
 	 * @param {mw.Title} title
-	 * @param {boolean} useReplaceState Whether to update history entry to avoid long history queues
 	 */
-	MMVP.loadImageByTitle = function ( title, useReplaceState ) {
+	MMVP.loadImageByTitle = function ( title ) {
 		var i, thumb;
 
 		if ( !this.thumbs || !this.thumbs.length ) {
@@ -394,7 +392,7 @@
 		for ( i = 0; i < this.thumbs.length; i++ ) {
 			thumb = this.thumbs[ i ];
 			if ( thumb.title.getPrefixedText() === title.getPrefixedText() ) {
-				this.loadImage( thumb.image, thumb.$thumb.clone()[ 0 ], useReplaceState );
+				this.loadImage( thumb.image, thumb.$thumb.clone()[ 0 ] );
 				return;
 			}
 		}
@@ -835,6 +833,10 @@
 
 			thumb = this.thumbs[ index ];
 			this.loadImage( thumb.image, thumb.$thumb.clone()[ 0 ] );
+			router.navigateTo( null, {
+				path: mw.mmv.getMediaHash( thumb.title ),
+				useReplaceState: true
+			} );
 		}
 	};
 
@@ -877,21 +879,6 @@
 	 * Sets up the route handlers
 	 */
 	MMVP.setupRouter = function () {
-		function route( fileName ) {
-			var fileTitle;
-			comingFromHashChange = true;
-			try {
-				fileName = decodeURIComponent( fileName );
-				fileTitle = new mw.Title( fileName );
-				this.loadImageByTitle( fileTitle );
-			} catch ( err ) {
-				// ignore routes to invalid titles
-				mw.log.warn( err );
-			}
-		}
-		this.router.addRoute( mw.mmv.ROUTE_REGEXP, route.bind( this ) );
-		this.router.addRoute( mw.mmv.LEGACY_ROUTE_REGEXP, route.bind( this ) );
-
 		// handle empty hashes, and anchor links (page sections)
 		this.router.addRoute( /^[^/]*$/, function () {
 			if ( this.isOpen ) {
@@ -908,22 +895,9 @@
 	};
 
 	/**
-	 * Updates the hash to reflect an open image file
-	 *
-	 * @param {boolean} useReplaceState Whether to update history entry to avoid long history queues
+	 * Updates the page title to reflect the current title.
 	 */
-	MMVP.setMediaHash = function ( useReplaceState ) {
-		if ( useReplaceState === undefined ) {
-			useReplaceState = true;
-		}
-		if ( comingFromHashChange ) {
-			comingFromHashChange = false;
-			return;
-		}
-		this.router.navigateTo( document.title, {
-			path: mw.mmv.getMediaHash( this.currentImageFileTitle ),
-			useReplaceState: useReplaceState
-		} );
+	MMVP.setTitle = function () {
 		// update title after route change, see T225387
 		document.title = this.createDocumentTitle( this.currentImageFileTitle );
 	};

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

@@ -60,6 +60,7 @@
 
 		bootstrap.getViewer = function () {
 			return viewer || {
+				loadImageByTitle: function () {},
 				initWithThumbs: function () {},
 				hash: function () {},
 				router: { checkRoute: function () {} }
@@ -91,7 +92,6 @@
 		// without us interfering with another immediate change
 		setTimeout( function () {
 			location.hash = hash;
-			bootstrap.hash();
 		} );
 
 		return mw.mmv.testHelpers.waitForAsync().then( function () {
@@ -135,19 +135,10 @@
 		// trigger first click, which will cause MMV to be loaded (which we've
 		// set up to fail)
 		event = new $.Event( 'click', { button: 0, which: 1 } );
-		returnValue = bootstrap.click( {}, event, mw.Title.newFromText( 'Foo' ) );
+		returnValue = bootstrap.click( event, mw.Title.newFromText( 'Foo' ) );
 		clock.tick( 10 );
 		assert.true( event.isDefaultPrevented(), 'First click is caught' );
 		assert.strictEqual( returnValue, false, 'First click is caught' );
-
-		// wait until MMW is loaded (or failed to load, in this case) before we
-		// trigger another click - which should then not be caught
-		event = new $.Event( 'click', { button: 0, which: 1 } );
-		returnValue = bootstrap.click( {}, event, mw.Title.newFromText( 'Foo' ) );
-		clock.tick( 10 );
-		assert.strictEqual( event.isDefaultPrevented(), false, 'Click after loading failure is not caught' );
-		assert.notStrictEqual( returnValue, false, 'Click after loading failure is not caught' );
-
 		clock.restore();
 	} );
 
@@ -307,15 +298,13 @@
 	QUnit.test( 'Ensure that the correct title is loaded when clicking', function ( assert ) {
 		var bootstrap,
 			viewer = { initWithThumbs: function () {}, loadImageByTitle: this.sandbox.stub() },
-			$div = createGallery( 'foo.jpg' ),
-			$link = $div.find( 'a.image' ),
 			clock = this.sandbox.useFakeTimers();
 
 		// Create a new bootstrap object to trigger the DOM scan, etc.
 		bootstrap = createBootstrap( viewer );
 		this.sandbox.stub( bootstrap, 'setupOverlay' );
 
-		$link.trigger( { type: 'click', which: 1 } );
+		bootstrap.route( 'File:Foo.jpg' );
 		clock.tick( 10 );
 		assert.true( bootstrap.setupOverlay.called, 'Overlay was set up' );
 		assert.strictEqual(
@@ -324,6 +313,7 @@
 			'Titles are identical'
 		);
 
+		clock.tick( 10 );
 		clock.restore();
 	} );
 

tests/qunit/mmv/mmv.test.js

@@ -696,9 +696,11 @@
 			title = new mw.Title( 'File:This_should_show_up_in_document_title.png' ),
 			oldDocumentTitle = document.title;
 
+		this.sandbox.stub( bootstrap.router, 'back' );
+		this.sandbox.stub( mw.loader, 'using' ).returns( $.Deferred().resolve( viewer ) );
 		viewer.currentImageFileTitle = title;
 		bootstrap.setupEventHandlers();
-		viewer.setMediaHash();
+		viewer.setTitle();
 
 		assert.notStrictEqual( document.title.match( title.getNameText() ), null, 'File name is visible in title' );