From 09374fc9dd65d4fa47a6d75e7de2c2823515c56b Mon Sep 17 00:00:00 2001 From: Gilles Dubuc Date: Fri, 11 Apr 2014 11:31:38 +0200 Subject: [PATCH] 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 | 1 + resources/mmv/mmv.bootstrap.js | 56 ++++++++--- resources/mmv/mmv.js | 19 ++-- resources/mmv/mmv.lightboxinterface.js | 30 +++--- .../features/basic_mmv_navigation.feature | 1 + .../basic_mmv_navigation_steps.rb | 11 +++ .../support/pages/lightbox_demo_page.rb | 2 +- tests/qunit/mmv/mmv.bootstrap.test.js | 98 +++++++++++++------ tests/qunit/mmv/mmv.lightboxinterface.test.js | 11 +-- 9 files changed, 149 insertions(+), 80 deletions(-) diff --git a/MultimediaViewer.php b/MultimediaViewer.php index 0af11da0d..490fd79b9 100644 --- a/MultimediaViewer.php +++ b/MultimediaViewer.php @@ -702,6 +702,7 @@ $wgResourceModules += array( 'mediawiki.Title', 'mmv.logger', 'mmv.HtmlUtils', + 'jquery.scrollTo', ), 'messages' => array( diff --git a/resources/mmv/mmv.bootstrap.js b/resources/mmv/mmv.bootstrap.js index 162843eb4..1cc2e20af 100755 --- a/resources/mmv/mmv.bootstrap.js +++ b/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 = $( '
' ) .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 () { diff --git a/resources/mmv/mmv.js b/resources/mmv/mmv.js index e003fcab6..99204742b 100755 --- a/resources/mmv/mmv.js +++ b/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 diff --git a/resources/mmv/mmv.lightboxinterface.js b/resources/mmv/mmv.lightboxinterface.js index 339bf502e..c36f97b18 100644 --- a/resources/mmv/mmv.lightboxinterface.js +++ b/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; }; /** diff --git a/tests/browser/features/basic_mmv_navigation.feature b/tests/browser/features/basic_mmv_navigation.feature index 84074f3e4..d25dac0a9 100644 --- a/tests/browser/features/basic_mmv_navigation.feature +++ b/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 diff --git a/tests/browser/features/step_definitions/basic_mmv_navigation_steps.rb b/tests/browser/features/step_definitions/basic_mmv_navigation_steps.rb index 664f7b0f1..46cd02e84 100644 --- a/tests/browser/features/step_definitions/basic_mmv_navigation_steps.rb +++ b/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) diff --git a/tests/browser/features/support/pages/lightbox_demo_page.rb b/tests/browser/features/support/pages/lightbox_demo_page.rb index c0266eaf8..320ddaadc 100644 --- a/tests/browser/features/support/pages/lightbox_demo_page.rb +++ b/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") diff --git a/tests/qunit/mmv/mmv.bootstrap.test.js b/tests/qunit/mmv/mmv.bootstrap.test.js index f13930eec..45dbb547d 100644 --- a/tests/qunit/mmv/mmv.bootstrap.test.js +++ b/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 ) ); diff --git a/tests/qunit/mmv/mmv.lightboxinterface.test.js b/tests/qunit/mmv/mmv.lightboxinterface.test.js index 713f2b620..5b6e05769 100644 --- a/tests/qunit/mmv/mmv.lightboxinterface.test.js +++ b/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;