jenkins-bot has submitted this change and it was merged. Change subject: Fixes hash handling issues ......................................................................
Fixes hash handling issues * Fixes the bug where the browser hash would get nuked on foreign anchor clicks * Moves the hash handling back into the mmv itself * Adds test coverage for the hash nuking bug Change-Id: Iea57d0f4b9090f96e622418223d3f774923e8038 Mingle: https://wikimedia.mingle.thoughtworks.com/projects/multimedia/cards/254 --- M resources/mmv/mmv.bootstrap.js M resources/mmv/mmv.js M tests/qunit/mmv.bootstrap.test.js M tests/qunit/mmv.test.js 4 files changed, 204 insertions(+), 160 deletions(-) Approvals: Gergő Tisza: Looks good to me, approved jenkins-bot: Verified diff --git a/resources/mmv/mmv.bootstrap.js b/resources/mmv/mmv.bootstrap.js index 1b69b69..cad5a87 100755 --- a/resources/mmv/mmv.bootstrap.js +++ b/resources/mmv/mmv.bootstrap.js @@ -19,12 +19,12 @@ var bootstrap, MMVB; /** - * Bootstrap code listening to thumb clicks and location.hash + * Bootstrap code listening to thumb clicks checking the initial location.hash * Loads the mmv and opens it if necessary * @class mw.mmv.MultimediaViewerBootstrap */ function MultimediaViewerBootstrap () { - var bs = this; + var self = this; this.validExtensions = { 'jpg' : true, @@ -41,19 +41,12 @@ this.processThumbs(); this.hash(); - $( window ).on( 'popstate.mmv', function() { - bs.hash(); + $( window ).on( 'popstate.mmvb', function () { + self.hash(); } ); } MMVB = MultimediaViewerBootstrap.prototype; - - /** - * Stops the boostrap - */ - MMVB.shutdown = function () { - $( window ).off( 'popstate.mmv' ); - }; /** * Loads the mmv module asynchronously and passes the thumb data to it @@ -166,22 +159,18 @@ }; /** - * Handles the browser's location.hash + * Handles the browser location hash on pageload or popstate */ MMVB.hash = function () { - var hash = decodeURIComponent( document.location.hash ), - linkState = hash.split( '/' ); - - if ( linkState[0] === '#mediaviewer' ) { - this.loadViewer().then( function () { - mw.mediaViewer.loadImageByTitle( linkState[ 1 ] ); - } ); - } else { - // If the hash is invalid (not a mmv hash) we close any open mmv - if ( mw.mediaViewer ) { - mw.mediaViewer.shutdown(); - } + // 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 && window.location.hash.indexOf( '#mediaviewer/') !== 0 ) { + return; } + + this.loadViewer().then( function () { + mw.mediaViewer.hash(); + } ); }; mw.mmv.MultimediaViewerBootstrap = MultimediaViewerBootstrap; diff --git a/resources/mmv/mmv.js b/resources/mmv/mmv.js index 7871a92..23e57e3 100755 --- a/resources/mmv/mmv.js +++ b/resources/mmv/mmv.js @@ -551,28 +551,39 @@ }; /** - * Shuts down the viewer + * Handles a hash change coming from the browser */ - MMVP.shutdown = function () { - if ( this.lightbox && this.lightbox.iface ) { - this.lightbox.iface.unattach(); + MMVP.hash = function () { + var hash = decodeURIComponent( document.location.hash ), + linkState = hash.split( '/' ); + + if ( linkState[0] === '#mediaviewer' ) { + this.loadImageByTitle( linkState[ 1 ] ); + } else if ( this.isOpen ) { + // This allows us to avoid the pushState that normally happens on close + comingFromPopstate = true; + + if ( this.lightbox && this.lightbox.iface ) { + this.lightbox.iface.unattach(); + } else { + this.close(); + } } }; /** - * Registers all event handlers, on the document, for various MMV-specific - * events. + * Registers all event handlers */ MMVP.setupEventHandlers = function () { var viewer = this; - $( document ).on( 'mmv-close', function () { + $( document ).on( 'mmv-close.mmvp', function () { viewer.close(); - } ).on( 'mmv-next', function () { + } ).on( 'mmv-next.mmvp', function () { viewer.nextImage(); - } ).on( 'mmv-prev', function () { + } ).on( 'mmv-prev.mmvp', function () { viewer.prevImage(); - } ).on( 'mmv-resize', function () { + } ).on( 'mmv-resize.mmvp', function () { viewer.resize( viewer.lightbox.iface ); }); }; diff --git a/tests/qunit/mmv.bootstrap.test.js b/tests/qunit/mmv.bootstrap.test.js index 95340d4..a2511f7 100644 --- a/tests/qunit/mmv.bootstrap.test.js +++ b/tests/qunit/mmv.bootstrap.test.js @@ -1,6 +1,4 @@ ( function ( mw, $ ) { - var oldUsing; - QUnit.module( 'mmv.bootstrap', QUnit.newMwEnvironment() ); function createGallery ( imageSrc ) { @@ -22,25 +20,10 @@ return div; } - function setupUsing ( assert ) { - oldUsing = mw.loader.using; - - mw.loader.using = function( module ) { - if ( module === 'mmv' ) { - assert.ok( true, 'mmv requested to be loaded' ); - } - }; - } - - function restoreUsing () { - mw.loader.using = oldUsing; - } - QUnit.test( 'Check viewer invoked when clicking on legit image links', 2, function ( assert ) { // TODO: Is <div class="gallery"><span class="image"><img/></span></div> valid ??? - var div, link, link2, link3, bootstrap; - - setupUsing( assert ); + var div, link, link2, link3, bootstrap, + oldLoadImageByTitle = mw.mediaViewer.loadImageByTitle; // Create gallery with legit link image div = createGallery(); @@ -57,23 +40,30 @@ // Create a new bootstrap object to trigger the DOM scan, etc. bootstrap = new mw.mmv.MultimediaViewerBootstrap(); + mw.mediaViewer.loadImageByTitle = function() { + assert.ok( true, 'Image loaded' ); + }; + // Click on legit link link.trigger( { type : 'click', which : 1 } ); // Click on legit link link2.trigger( { type : 'click', which : 1 } ); + mw.mediaViewer.loadImageByTitle = function() { + assert.ok( false, 'Image should not be loaded' ); + }; + // Click on non-legit link link3.trigger( { type : 'click', which : 1 } ); - bootstrap.shutdown(); - restoreUsing(); + mw.mediaViewer.loadImageByTitle = oldLoadImageByTitle; + bootstrap.hash = $.noop; } ); QUnit.test( 'Skip images with invalid extensions', 0, function ( assert ) { - var div, link, bootstrap; - - setupUsing( assert ); + var div, link, bootstrap, + oldLoadImageByTitle = mw.mediaViewer.loadImageByTitle; // Create gallery with image that has invalid name extension div = createGallery( 'thumb.badext' ); @@ -82,17 +72,20 @@ // Create a new bootstrap object to trigger the DOM scan, etc. bootstrap = new mw.mmv.MultimediaViewerBootstrap(); + mw.mediaViewer.loadImageByTitle = function() { + assert.ok( false, 'Image should not be loaded' ); + }; + // Click on legit link with wrong image extension link.trigger( { type : 'click', which : 1 } ); - bootstrap.shutdown(); - restoreUsing(); + mw.mediaViewer.loadImageByTitle = oldLoadImageByTitle; + bootstrap.hash = $.noop; } ); QUnit.test( 'Accept only left clicks without modifier keys, skip the rest', 1, function ( assert ) { - var $div, $link, bootstrap; - - setupUsing( assert ); + var $div, $link, bootstrap, + oldLoadImageByTitle = mw.mediaViewer.loadImageByTitle; // Create gallery with image that has invalid name extension $div = createGallery(); @@ -102,8 +95,16 @@ $link = $div.find( 'a.image' ); + mw.mediaViewer.loadImageByTitle = function() { + assert.ok( true, 'Image loaded' ); + }; + // Handle valid left click, it should try to load the image $link.trigger( { type : 'click', which : 1 } ); + + mw.mediaViewer.loadImageByTitle = function() { + assert.ok( false, 'Image should not be loaded' ); + }; // Skip Ctrl-left-click, no image is loaded $link.trigger( { type : 'click', which : 1, ctrlKey : true } ); @@ -111,124 +112,88 @@ // Skip invalid right click, no image is loaded $link.trigger( { type : 'click', which : 2 } ); - bootstrap.shutdown(); - restoreUsing(); + mw.mediaViewer.loadImageByTitle = oldLoadImageByTitle; + bootstrap.hash = $.noop; } ); - QUnit.asyncTest( 'Ensure that the correct title is loaded when clicking', 1, function ( assert ) { - // Preload mmv - mw.loader.using( 'mmv', function () { - QUnit.start(); - - var bootstrap, - oldByTitle, - $div = createGallery( 'foo.jpg' ), - $link = $div.find( 'a.image' ); - - oldByTitle = mw.mediaViewer.loadImageByTitle; - - mw.mediaViewer.loadImageByTitle = function ( loadedTitle ) { - assert.strictEqual( loadedTitle, 'File:Foo.jpg', 'Titles are identical' ); - }; - - // Create a new bootstrap object to trigger the DOM scan, etc. - bootstrap = new mw.mmv.MultimediaViewerBootstrap(); - - $link.trigger( { type : 'click', which : 1 } ); - - bootstrap.shutdown(); - mw.mediaViewer.loadImageByTitle = oldByTitle; - } ); - } ); - - QUnit.asyncTest( 'Validate new LightboxImage object has sane constructor parameters', 6, function ( assert ) { - // Preload mmv - mw.loader.using( 'mmv', function () { - QUnit.start(); - - var bootstrap, - $div, - $link, - oldNewImage = mw.mediaViewer.createNewImage, - oldLoadImage = mw.mediaViewer.loadImage, - fname = 'valid', - imgSrc = '/' + fname + '.jpg/300px-' + fname + '.jpg', - imgRegex = new RegExp( imgSrc + '$' ); - - $div = createThumb( imgSrc, 'Blah blah' ); + QUnit.test( 'Ensure that the correct title is loaded when clicking', 1, function ( assert ) { + var bootstrap, + oldByTitle, + $div = createGallery( 'foo.jpg' ), $link = $div.find( 'a.image' ); - // Create a new bootstrap object to trigger the DOM scan, etc. - bootstrap = new mw.mmv.MultimediaViewerBootstrap(); + oldByTitle = mw.mediaViewer.loadImageByTitle; - mw.mediaViewer.loadImage = $.noop; + mw.mediaViewer.loadImageByTitle = function ( loadedTitle ) { + assert.strictEqual( loadedTitle, 'File:Foo.jpg', 'Titles are identical' ); + }; - mw.mediaViewer.createNewImage = function ( fileLink, filePageLink, fileTitle, index, thumb, caption ) { - assert.ok( fileLink.match( imgRegex ), 'Thumbnail URL used in creating new image object' ); - assert.strictEqual( filePageLink, '', 'File page link is sane when creating new image object' ); - assert.strictEqual( fileTitle.title, fname, 'Filename is correct when passed into new image constructor' ); - assert.strictEqual( index, 0, 'The only image we created in the gallery is set at index 0 in the images array' ); - assert.strictEqual( thumb.outerHTML, '<img src="' + imgSrc + '">', 'The image element passed in is the thumbnail we want.' ); - assert.strictEqual( caption, 'Blah blah', 'The caption passed in is correct' ); - }; + // Create a new bootstrap object to trigger the DOM scan, etc. + bootstrap = new mw.mmv.MultimediaViewerBootstrap(); - $link.trigger( { type : 'click', which : 1 } ); + $link.trigger( { type : 'click', which : 1 } ); - mw.mediaViewer.createNewImage = oldNewImage; - mw.mediaViewer.loadImage = oldLoadImage; - - bootstrap.shutdown(); - } ); + mw.mediaViewer.loadImageByTitle = oldByTitle; + bootstrap.hash = $.noop; } ); - QUnit.asyncTest( 'Hash handling', 3, function ( assert ) { - // Preload mmv - mw.loader.using( 'mmv', function () { - QUnit.start(); + QUnit.test( 'Validate new LightboxImage object has sane constructor parameters', 6, function ( assert ) { + var bootstrap, + $div, + $link, + oldNewImage = mw.mediaViewer.createNewImage, + oldLoadImage = mw.mediaViewer.loadImage, + fname = 'valid', + imgSrc = '/' + fname + '.jpg/300px-' + fname + '.jpg', + imgRegex = new RegExp( imgSrc + '$' ); - var bootstrap, - oldLoadImage = mw.mediaViewer.loadImageByTitle, - oldLightbox = mw.mediaViewer.lightbox, - imageSrc = 'Foo bar.jpg', - image = { filePageTitle: new mw.Title( 'File:' + imageSrc ) }; + $div = createThumb( imgSrc, 'Blah blah' ); + $link = $div.find( 'a.image' ); - // Create a new bootstrap object to trigger the DOM scan, etc. - bootstrap = new mw.mmv.MultimediaViewerBootstrap(); + mw.mediaViewer.loadImage = $.noop; - document.location.hash = ''; + mw.mediaViewer.createNewImage = function ( fileLink, filePageLink, fileTitle, index, thumb, caption ) { + assert.ok( fileLink.match( imgRegex ), 'Thumbnail URL used in creating new image object' ); + assert.strictEqual( filePageLink, '', 'File page link is sane when creating new image object' ); + assert.strictEqual( fileTitle.title, fname, 'Filename is correct when passed into new image constructor' ); + assert.strictEqual( index, 0, 'The only image we created in the gallery is set at index 0 in the images array' ); + assert.strictEqual( thumb.outerHTML, '<img src="' + imgSrc + '">', 'The image element passed in is the thumbnail we want.' ); + assert.strictEqual( caption, 'Blah blah', 'The caption passed in is correct' ); + }; - mw.mediaViewer.lightbox = { iface: { unattach: function() { - assert.ok( true, 'Interface unattached' ); - } } }; + // Create a new bootstrap object to trigger the DOM scan, etc. + bootstrap = new mw.mmv.MultimediaViewerBootstrap(); - // Verify that passing an invalid mmv hash triggers unattach() - document.location.hash = 'Foo'; + $link.trigger( { type : 'click', which : 1 } ); - mw.mediaViewer.lightbox = { images: [ image ] }; + mw.mediaViewer.createNewImage = oldNewImage; + mw.mediaViewer.loadImage = oldLoadImage; + bootstrap.hash = $.noop; + } ); - $( '#qunit-fixture' ).append( '<a class="image"><img src="' + imageSrc + '"></a>' ); + QUnit.test( 'Only load the viewer on a valid hash', 1, function ( assert ) { + var bootstrap; - mw.mediaViewer.loadImageByTitle = function( title ) { - assert.strictEqual( title, 'File:' + imageSrc, 'The title matches' ); - }; + window.location.hash = ''; - // Open a valid mmv hash link and check that the right image is requested. - // imageSrc contains a space without any encoding on purpose - document.location.hash = 'mediaviewer/File:' + imageSrc; + bootstrap = new mw.mmv.MultimediaViewerBootstrap(); - // Reset the hash, because for some browsers switching from the non-URI-encoded to - // the non-URI-encoded version of the same text with a space will not trigger a hash change - document.location.hash = ''; + bootstrap.loadViewer = function () { + assert.ok( false, 'Viewer should not be loaded' ); + return $.Deferred().reject(); + }; - // Try again with an URI-encoded imageSrc containing a space - document.location.hash = 'mediaviewer/File:' + encodeURIComponent( imageSrc ); + window.location.hash = 'Foo'; - mw.mediaViewer.lightbox = oldLightbox; - mw.mediaViewer.loadImageByTitle = oldLoadImage; + bootstrap.loadViewer = function () { + assert.ok( true, 'Viewer should be loaded' ); + return $.Deferred().reject(); + }; - document.location.hash = ''; + window.location.hash = 'mediaviewer/foo'; - bootstrap.shutdown(); - } ); + bootstrap.hash = $.noop; + + window.location.hash = ''; } ); }( mediaWiki, jQuery ) ); diff --git a/tests/qunit/mmv.test.js b/tests/qunit/mmv.test.js index 59a632a..8b7aafb 100644 --- a/tests/qunit/mmv.test.js +++ b/tests/qunit/mmv.test.js @@ -4,14 +4,19 @@ QUnit.test( 'Metadata div is only animated once', 4, function ( assert ) { localStorage.removeItem( 'mmv.hasOpenedMetadata' ); - var viewer = new mw.MultimediaViewer(), + var viewer, backupAnimation = $.fn.animate, - animationRan = false; + animationRan = false, + oldSetupEventHandlers = mw.MultimediaViewer.prototype.setupEventHandlers; $.fn.animate = function () { animationRan = true; return this; }; + + // Because we don't want that throwaway instance to listen to events, could interfere with other tests + mw.MultimediaViewer.prototype.setupEventHandlers = $.noop; + viewer = new mw.MultimediaViewer(); viewer.animateMetadataDivOnce(); assert.strictEqual( viewer.hasAnimatedMetadata, true, 'The first call to animateMetadataDivOnce set hasAnimatedMetadata to true' ); @@ -23,6 +28,8 @@ assert.strictEqual( animationRan, false, 'The second call to animateMetadataDivOnce did not lead to an animation' ); $.fn.animate = backupAnimation; + + mw.MultimediaViewer.prototype.setupEventHandlers = oldSetupEventHandlers; } ); QUnit.test( 'eachPrealoadableLightboxIndex()', 11, function ( assert ) { @@ -70,4 +77,76 @@ viewer.lightbox.currentIndex = oldPosition; } } ); + + QUnit.test( 'Hash handling', 7, function ( assert ) { + var oldUnattach, + multiLightbox = new window.MultiLightbox(), + lightbox = new mw.LightboxInterface( mw.mediaViewer ), + oldLoadImage = mw.mediaViewer.loadImageByTitle, + oldLightbox = mw.mediaViewer.lightbox, + imageSrc = 'Foo bar.jpg', + image = { filePageTitle: new mw.Title( 'File:' + imageSrc ) }; + + document.location.hash = ''; + + oldUnattach = lightbox.unattach; + + lightbox.unattach = function () { + assert.ok( true, 'Lightbox was unattached' ); + oldUnattach.call( this ); + }; + + mw.mediaViewer.lightbox = multiLightbox; + mw.mediaViewer.lightbox.iface = lightbox; + mw.mediaViewer.close(); + + assert.ok( !mw.mediaViewer.isOpen, 'Viewer is closed' ); + + // The mediaViewer won't be initialized through bootstrap by any other way than a valid action + document.location.hash = 'mediaviewer/Foo'; + mw.mediaViewer.isOpen = true; + // From now on we're certain that the viewer receives hash changes through bootstrap + + // Verify that passing an invalid mmv hash when the mmv is open triggers unattach() + document.location.hash = 'Foo'; + + // Verify that mmv doesn't reset a foreign hash + assert.strictEqual( document.location.hash, '#Foo', 'Foreign hash remains intact' ); + assert.ok( !mw.mediaViewer.isOpen, 'Viewer is closed' ); + + lightbox.unattach = function () { + assert.ok( false, 'Lightbox was not unattached' ); + oldUnattach.call( this ); + }; + + // Verify that passing an invalid mmv hash when the mmv is closed doesn't trigger unattach() + document.location.hash = 'Bar'; + + // Verify that mmv doesn't reset a foreign hash + assert.strictEqual( document.location.hash, '#Bar', 'Foreign hash remains intact' ); + + mw.mediaViewer.lightbox = { images: [ image ] }; + + $( '#qunit-fixture' ).append( '<a class="image"><img src="' + imageSrc + '"></a>' ); + + mw.mediaViewer.loadImageByTitle = function( title ) { + assert.strictEqual( title, 'File:' + imageSrc, 'The title matches' ); + }; + + // Open a valid mmv hash link and check that the right image is requested. + // imageSrc contains a space without any encoding on purpose + document.location.hash = 'mediaviewer/File:' + imageSrc; + + // Reset the hash, because for some browsers switching from the non-URI-encoded to + // the non-URI-encoded version of the same text with a space will not trigger a hash change + document.location.hash = ''; + + // Try again with an URI-encoded imageSrc containing a space + document.location.hash = 'mediaviewer/File:' + encodeURIComponent( imageSrc ); + + mw.mediaViewer.lightbox = oldLightbox; + mw.mediaViewer.loadImageByTitle = oldLoadImage; + + document.location.hash = ''; + } ); }( mediaWiki, jQuery ) ); -- To view, visit https://gerrit.wikimedia.org/r/114728 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Iea57d0f4b9090f96e622418223d3f774923e8038 Gerrit-PatchSet: 6 Gerrit-Project: mediawiki/extensions/MultimediaViewer Gerrit-Branch: master Gerrit-Owner: Gilles <gdu...@wikimedia.org> Gerrit-Reviewer: Aarcos <aarcos.w...@gmail.com> Gerrit-Reviewer: Gergő Tisza <gti...@wikimedia.org> Gerrit-Reviewer: Gilles <gdu...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
