jenkins-bot has submitted this change and it was merged.
Change subject: Do not set up the overlay on irrelevant hash changes
......................................................................
Do not set up the overlay on irrelevant hash changes
On not-MMV -> not-MMV hash changes the bootstrap class has set
up and immediately torn down the overlay. Besides slowing things
down, this broke TOC navigation on Chrome in an ugly way.
Add a special case to skip the overlay loading when the new hash
does not contain #/media. This is a quick-and-dirty fix; the whole
loading and hash handling could use a rewrite.
Bug: T119854
Change-Id: I5494903dfe778e533773ff142fdf756475c2df32
---
M resources/mmv/mmv.bootstrap.js
M tests/qunit/mmv/mmv.bootstrap.test.js
2 files changed, 51 insertions(+), 10 deletions(-)
Approvals:
Jdlrobson: Looks good to me, approved
jenkins-bot: Verified
diff --git a/resources/mmv/mmv.bootstrap.js b/resources/mmv/mmv.bootstrap.js
index 2a2ef4d..d1e3ef0 100644
--- a/resources/mmv/mmv.bootstrap.js
+++ b/resources/mmv/mmv.bootstrap.js
@@ -79,9 +79,10 @@
/**
* Loads the mmv module asynchronously and passes the thumb data to it
+ * @param {boolean} [setupOverlay]
* @returns {jQuery.Promise}
*/
- MMVB.loadViewer = function () {
+ MMVB.loadViewer = function ( setupOverlay ) {
var deferred = $.Deferred(),
bs = this,
viewer,
@@ -92,7 +93,14 @@
return deferred.reject();
}
- bs.setupOverlay();
+ // FIXME setupOverlay is a quick hack to avoid setting up and
immediately
+ // removing the overlay on a not-MMV -> not-MMV hash change.
+ // loadViewer is called on every click and hash change and
setting up
+ // the overlay is not needed on all of those; this logic really
should
+ // not be here.
+ if ( setupOverlay ) {
+ bs.setupOverlay();
+ }
mw.loader.using( 'mmv', function () {
try {
@@ -373,7 +381,7 @@
this.ensureEventHandlersAreSetUp();
- return this.loadViewer().then( function ( viewer ) {
+ return this.loadViewer( true ).then( function ( viewer ) {
viewer.loadImageByTitle( title, true );
} );
};
@@ -410,7 +418,15 @@
return false;
};
-
+ /**
+ * Returns true if the hash part of the current URL is one that's owned
by MMV.
+ * @returns {boolean}
+ * @private
+ */
+ MMVB.isViewerHash = function () {
+ return window.location.hash.indexOf( '#mediaviewer/' ) === 0 ||
+ window.location.hash.indexOf( '#/media/' ) === 0;
+ };
/**
* Handles the browser location hash on pageload or hash change
@@ -421,9 +437,7 @@
// 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
- && window.location.hash.indexOf( '#/media/' ) !== 0 ) {
+ if ( !this.viewerInitialized && !this.isViewerHash() ) {
return;
}
@@ -432,7 +446,7 @@
return;
}
- this.loadViewer().then( function ( viewer ) {
+ this.loadViewer( this.isViewerHash() ).then( function ( viewer
) {
viewer.hash();
// this is an ugly temporary fix to avoid a black
screen of death when
// the page is loaded with an invalid MMV url
diff --git a/tests/qunit/mmv/mmv.bootstrap.test.js
b/tests/qunit/mmv/mmv.bootstrap.test.js
index 61a2ccf..56c1fc9 100644
--- a/tests/qunit/mmv/mmv.bootstrap.test.js
+++ b/tests/qunit/mmv/mmv.bootstrap.test.js
@@ -56,7 +56,7 @@
// MediaViewer should work without it, and so should the tests.
bootstrap.ensureEventHandlersAreSetUp = $.noop;
- bootstrap.getViewer = function () { return viewer ? viewer : {
initWithThumbs: $.noop }; };
+ bootstrap.getViewer = function () { return viewer ? viewer : {
initWithThumbs: $.noop, hash: $.noop }; };
return bootstrap;
}
@@ -106,7 +106,7 @@
QUnit.stop();
- bootstrap.loadViewer().fail( function ( message ) {
+ bootstrap.loadViewer( true ).fail( function ( message ) {
assert.strictEqual( message, errorMessage, 'promise is
rejected with the error message when loading fails' );
QUnit.start();
} );
@@ -363,6 +363,33 @@
hashTest( 'mediaviewer', bootstrap, assert );
} );
+ QUnit.test( 'Overlay is set up on hash change', 1, function( assert ) {
+ var bootstrap;
+
+ window.location.hash = '#/media/foo';
+
+ bootstrap = createBootstrap();
+ this.sandbox.stub( bootstrap, 'setupOverlay' );
+
+ bootstrap.hash();
+
+ assert.ok( bootstrap.setupOverlay.called, 'Overlay is set up' );
+ } );
+
+ QUnit.test( 'Overlay is not set up on an irrelevant hash change', 1,
function( assert ) {
+ var bootstrap;
+
+ window.location.hash = '#foo';
+
+ bootstrap = createBootstrap();
+ this.sandbox.stub( bootstrap, 'setupOverlay' );
+ bootstrap.loadViewer();
+ bootstrap.setupOverlay.reset();
+
+ bootstrap.hash();
+
+ assert.ok( !bootstrap.setupOverlay.called, 'Overlay is not set
up' );
+ } );
QUnit.test( 'internalHashChange', 1, function ( assert ) {
var bootstrap = createBootstrap(),
--
To view, visit https://gerrit.wikimedia.org/r/256841
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I5494903dfe778e533773ff142fdf756475c2df32
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/extensions/MultimediaViewer
Gerrit-Branch: master
Gerrit-Owner: Gergő Tisza <[email protected]>
Gerrit-Reviewer: Gergő Tisza <[email protected]>
Gerrit-Reviewer: Gilles <[email protected]>
Gerrit-Reviewer: Jdlrobson <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
