From 0997b826501bcf60b25e7c28f46f26ea9e8d6a28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Tisza?= Date: Sat, 22 Nov 2014 00:16:36 +0000 Subject: [PATCH] Fix some metadata panel scrolling/text truncation bugs * isOpening was calculated too early, so whenever the panel was partially open and opened up fully via the forceDirection parameter of toggle(), it was logged as the wrong direction. (I think the only way for this to happen is via clicking on "view terms" while the panel is partially open.) * scrolling did not go all the way to the bottom when text was truncated as the target position was calculated before untruncating. * panel position was not preserved in some cases where it could have been because the attempt to restore it happened before untruncating the text, when the panel was not high enough. Change-Id: I47a96d42c80e0a00d95023526ede3b5bbf18a52c Mingle: https://wikimedia.mingle.thoughtworks.com/projects/multimedia/cards/983 --- resources/mmv/ui/mmv.ui.canvas.js | 2 +- resources/mmv/ui/mmv.ui.metadataPanel.js | 24 +++++++-------- .../mmv/ui/mmv.ui.metadataPanelScroller.js | 30 +++++++++---------- 3 files changed, 25 insertions(+), 31 deletions(-) diff --git a/resources/mmv/ui/mmv.ui.canvas.js b/resources/mmv/ui/mmv.ui.canvas.js index a1937c2d2..75e4f2d26 100644 --- a/resources/mmv/ui/mmv.ui.canvas.js +++ b/resources/mmv/ui/mmv.ui.canvas.js @@ -246,7 +246,7 @@ * @param {mw.mmv.model.ThumbnailWidth} imageWidths * @returns {boolean} Whether the image was blured or not */ - C.maybeDisplayPlaceholder = function ( size, $imagePlaceholder, imageWidths ) { + C.maybeDisplayPlaceholder = function ( size, $imagePlaceholder, imageWidths ) { var targetWidth, targetHeight, blowupFactor, diff --git a/resources/mmv/ui/mmv.ui.metadataPanel.js b/resources/mmv/ui/mmv.ui.metadataPanel.js index 320ef14e1..5c3bc34eb 100644 --- a/resources/mmv/ui/mmv.ui.metadataPanel.js +++ b/resources/mmv/ui/mmv.ui.metadataPanel.js @@ -61,6 +61,9 @@ .add( this.$authorAndSource ) .add( this.title.$ellipsis ) .add( this.creditField.$ellipsis ) + .each( function () { + $( this ).tipsy( 'enable' ); + } ) .on( 'click.mmv-mp', function ( e ) { var clickTargetIsLink = $( e.target ).is( 'a' ), clickTargetIsTruncated = !!$( e.target ).closest( '.mw-mmv-ttf-truncated' ).length, @@ -76,23 +79,15 @@ panel.scroller.toggle( 'up' ); } ); - $( this.$container ).on( 'mmv-metadata-open', function () { + $( this.$container ).on( 'mmv-metadata-open.mmv-mp mmv-metadata-reveal-truncated-text.mmv-mp', function () { panel.revealTruncatedText(); - } ).on( 'mmv-metadata-close', function () { + } ).on( 'mmv-metadata-close.mmv-mp', function () { panel.hideTruncatedText(); } ); this.handleEvent( 'jq-fullscreen-change.lip', function() { panel.hideTruncatedText(); } ); - - this.$title - .add( this.title.$ellipsis ) - .add( this.$authorAndSource ) - .add( this.creditField.$ellipsis ) - .each( function () { - $( this ).tipsy( 'enable' ); - } ); }; MPP.unattach = function() { @@ -104,12 +99,13 @@ .add( this.creditField.$ellipsis ) .each( function () { $( this ).tipsy( 'hide' ).tipsy( 'disable' ); - } ); + } ) + .off( 'click.mmv-mp' ); + + $( this.$container ).off( '.mmv-mp' ); this.scroller.unattach(); this.buttons.unattach(); - - this.$title.add( this.$authorAndSource ).off( 'click.mmv-mp' ); this.clearEvents(); }; @@ -671,8 +667,8 @@ this.setUserPageLink( repoData, imageData.lastUploader, user.gender ); } - this.scroller.unfreezeHeight(); this.resetTruncatedText(); + this.scroller.unfreezeHeight(); }; /** diff --git a/resources/mmv/ui/mmv.ui.metadataPanelScroller.js b/resources/mmv/ui/mmv.ui.metadataPanelScroller.js index 0992c1eb4..3e7416ca3 100644 --- a/resources/mmv/ui/mmv.ui.metadataPanelScroller.js +++ b/resources/mmv/ui/mmv.ui.metadataPanelScroller.js @@ -156,34 +156,32 @@ * @param {string} [forceDirection] 'up' or 'down' makes the panel move on that direction (and is a noop * if the panel is already at the upmost/bottommost position); without the parameter, the panel position * is toggled. (Partially open counts as open.) - * @param {number} [duration] duration of the scroll animation; defaults to #toggleScrollDuration. - * Use 0 to avoid animating altogether. * @return {jQuery.Deferred} a deferred which resolves after the animation has finished. */ - MPSP.toggle = function ( forceDirection, duration ) { + MPSP.toggle = function ( forceDirection ) { var deferred = $.Deferred(), scrollTopWhenOpen = this.getScrollTopWhenOpen(), scrollTopWhenClosed = 0, scrollTop = $.scrollTo().scrollTop(), panelIsOpen = scrollTop > scrollTopWhenClosed, - scrollTopTarget = panelIsOpen ? scrollTopWhenClosed : scrollTopWhenOpen, - isOpening = scrollTopTarget === scrollTopWhenOpen; - - - if ( duration === null || duration === undefined ) { - duration = this.toggleScrollDuration; - } - - if ( forceDirection ) { - scrollTopTarget = forceDirection === 'down' ? scrollTopWhenClosed : scrollTopWhenOpen; - } + direction = forceDirection || ( panelIsOpen ? 'down' : 'up' ), + scrollTopTarget = ( direction === 'up' ) ? scrollTopWhenOpen : scrollTopWhenClosed; // don't log / animate if the panel is already in the end position if ( scrollTopTarget === scrollTop ) { deferred.resolve(); } else { - mw.mmv.actionLogger.log( isOpening ? 'metadata-open' : 'metadata-close' ); - $.scrollTo( scrollTopTarget, duration, { + mw.mmv.actionLogger.log( direction === 'up' ? 'metadata-open' : 'metadata-close' ); + if ( direction === 'up' && !panelIsOpen ) { + // FIXME nasty. This is not really an event but a command sent to the metadata panel; + // child UI elements should not send commands to their parents. However, there is no way + // to calculate the target scrollTop accurately without revealing the text, and the event + // which does that (metadata-open) is only triggered later in the process, when the panel + // actually scrolled, so we cannot use it here without risking triggering it multiple times. + this.$container.trigger( 'mmv-metadata-reveal-truncated-text' ); + scrollTopTarget = this.getScrollTopWhenOpen(); + } + $.scrollTo( scrollTopTarget, this.toggleScrollDuration, { onAfter: function () { deferred.resolve(); }