From 6178781c43b3ccd908ab9017f1ffb9d607e0c59a Mon Sep 17 00:00:00 2001 From: jdlrobson Date: Fri, 13 May 2016 15:45:19 -0700 Subject: [PATCH] Do not unnecessarily expose private variables article.surveyLink, article.SIZES and currentRequest do not need to be globally available on mw.popups.render object. They can be local variables. Similarly openTimer and closeTimer should not be public APIs. Changes: * Add an abort method for purpose of aborting existing requests. Change-Id: Ic2add9c611990bf80e8b80ab154563f6551a77ea --- resources/ext.popups.renderer.article.js | 118 +++++++++--------- .../ext.popups.renderer/desktopRenderer.js | 58 +++------ 2 files changed, 76 insertions(+), 100 deletions(-) diff --git a/resources/ext.popups.renderer.article.js b/resources/ext.popups.renderer.article.js index ac8363a27..7fa13e4f4 100644 --- a/resources/ext.popups.renderer.article.js +++ b/resources/ext.popups.renderer.article.js @@ -5,38 +5,24 @@ * @class mw.popups.render.article * @singleton */ - var article = {}, - $window = $( window ); - - /** - * Number of chars to request for the article extract - * @property CHARS - */ - article.CHARS = 525; - - /** - * Size constants for popup images - * @property SIZES - */ - article.SIZES = { - portraitImage: { - h: 250, // Exact height - w: 203 // Max width - }, - landscapeImage: { - h: 200, // Max height - w: 300 // Exact Width - }, - landscapePopupWidth: 450, // Exact width of a landscape popup - portraitPopupWidth: 300, // Exact width of a portrait popup - pokeySize: 8 // Height of the triangle used to point at the link - }; - - /** - * Survey link, if any, for this renderer - * @property surveyLink - */ - article.surveyLink = mw.config.get( 'wgPopupsSurveyLink' ); + var currentRequest, + article = {}, + surveyLink = mw.config.get( 'wgPopupsSurveyLink' ), + $window = $( window ), + CHARS = 525, + SIZES = { + portraitImage: { + h: 250, // Exact height + w: 203 // Max width + }, + landscapeImage: { + h: 200, // Max height + w: 300 // Exact Width + }, + landscapePopupWidth: 450, // Exact width of a landscape popup + portraitPopupWidth: 300, // Exact width of a portrait popup + pokeySize: 8 // Height of the triangle used to point at the link + }; /** * Send an API request and cache the jQuery element @@ -55,13 +41,13 @@ return deferred.reject().promise(); } - mw.popups.render.currentRequest = mw.popups.api.get( { + currentRequest = mw.popups.api.get( { action: 'query', prop: 'info|extracts|pageimages|revisions', formatversion: 2, redirects: true, exintro: true, - exchars: article.CHARS, + exchars: CHARS, // there is an added geometric limit on .mwe-popups-extract // so that text does not overflow from the card explaintext: true, @@ -78,16 +64,16 @@ } } ); - mw.popups.render.currentRequest.fail( function ( textStatus ) { + currentRequest.fail( function ( textStatus ) { mw.track( 'ext.popups.schemaPopups', $.extend( logData, { action: 'error', errorState: textStatus, totalInteractionTime: Math.round( mw.now() - logData.dwellStartTime ) } ) ); deferred.reject(); - } ); - mw.popups.render.currentRequest.done( function ( re ) { - mw.popups.render.currentRequest = undefined; + } ) + .done( function ( re ) { + currentRequest = undefined; if ( !re.query || @@ -159,8 +145,8 @@ } $div.find( '.mwe-popups-extract' ) .append( article.getProcessedElements( page.extract, page.title ) ); - if ( article.surveyLink ) { - $div.find( 'footer' ).append( article.createSurveyLink( article.surveyLink ) ); + if ( surveyLink ) { + $div.find( 'footer' ).append( article.createSurveyLink( surveyLink ) ); } mw.popups.render.cache[ href ].settings = { @@ -320,9 +306,9 @@ if ( // Image too small for landscape display - ( !tall && thumbWidth < article.SIZES.landscapeImage.w ) || + ( !tall && thumbWidth < SIZES.landscapeImage.w ) || // Image too small for portrait display - ( tall && thumbHeight < article.SIZES.portraitImage.h ) || + ( tall && thumbHeight < SIZES.portraitImage.h ) || // These characters in URL that could inject CSS and thus JS ( thumbnail.source.indexOf( '\\' ) > -1 || @@ -337,16 +323,16 @@ return article.createSvgImageThumbnail( 'mwe-popups-is-not-tall', thumbnail.source, - ( thumbWidth > article.SIZES.portraitImage.w ) ? - ( ( thumbWidth - article.SIZES.portraitImage.w ) / -2 ) : - ( article.SIZES.portraitImage.w - thumbWidth ), - ( thumbHeight > article.SIZES.portraitImage.h ) ? - ( ( thumbHeight - article.SIZES.portraitImage.h ) / -2 ) : + ( thumbWidth > SIZES.portraitImage.w ) ? + ( ( thumbWidth - SIZES.portraitImage.w ) / -2 ) : + ( SIZES.portraitImage.w - thumbWidth ), + ( thumbHeight > SIZES.portraitImage.h ) ? + ( ( thumbHeight - SIZES.portraitImage.h ) / -2 ) : 0, thumbWidth, thumbHeight, - article.SIZES.portraitImage.w, - article.SIZES.portraitImage.h + SIZES.portraitImage.w, + SIZES.portraitImage.h ); } @@ -359,14 +345,14 @@ 'mwe-popups-is-not-tall', thumbnail.source, 0, - ( thumbHeight > article.SIZES.landscapeImage.h ) ? - ( ( thumbHeight - article.SIZES.landscapeImage.h ) / -2 ) : + ( thumbHeight > SIZES.landscapeImage.h ) ? + ( ( thumbHeight - SIZES.landscapeImage.h ) / -2 ) : 0, thumbWidth, thumbHeight, - article.SIZES.landscapeImage.w + 3, - ( thumbHeight > article.SIZES.landscapeImage.h ) ? - article.SIZES.landscapeImage.h : + SIZES.landscapeImage.w + 3, + ( thumbHeight > SIZES.landscapeImage.h ) ? + SIZES.landscapeImage.h : thumbHeight, 'mwe-popups-mask' ); @@ -461,9 +447,9 @@ event.pageY - $window.scrollTop(), link.get( 0 ).getClientRects(), false - ) + $window.scrollTop() + article.SIZES.pokeySize : + ) + $window.scrollTop() + SIZES.pokeySize : // Position according to link position or size - link.offset().top + link.height() + article.SIZES.pokeySize, + link.offset().top + link.height() + SIZES.pokeySize, clientTop = ( event.clientY ) ? event.clientY : offsetTop, @@ -475,8 +461,8 @@ if ( offsetLeft > ( $( window ).width() / 2 ) ) { offsetLeft += ( !event.pageX ) ? link.width() : 0; offsetLeft -= ( !settings.tall ) ? - article.SIZES.portraitPopupWidth : - article.SIZES.landscapePopupWidth; + SIZES.portraitPopupWidth : + SIZES.landscapePopupWidth; flippedX = true; } @@ -498,7 +484,7 @@ event.pageY - $window.scrollTop(), link.get( 0 ).getClientRects(), true - ) + $window.scrollTop() + 2 * article.SIZES.pokeySize; + ) + $window.scrollTop() + 2 * SIZES.pokeySize; } } @@ -591,10 +577,10 @@ } ) ); } ); - if ( !flippedY && !tall && cache.settings.thumbnail.height < article.SIZES.landscapeImage.h ) { + if ( !flippedY && !tall && cache.settings.thumbnail.height < SIZES.landscapeImage.h ) { $( '.mwe-popups-extract' ).css( 'margin-top', - cache.settings.thumbnail.height - article.SIZES.pokeySize + cache.settings.thumbnail.height - SIZES.pokeySize ); } @@ -670,6 +656,16 @@ return result; } + /** + * Aborts any pending ajax requests + */ + mw.popups.render.abortCurrentRequest = function () { + if ( currentRequest ) { + currentRequest.abort(); + currentRequest = undefined; + } + }; + /** * Expose for tests */ diff --git a/resources/ext.popups.renderer/desktopRenderer.js b/resources/ext.popups.renderer/desktopRenderer.js index 28c943116..6fd21edb1 100644 --- a/resources/ext.popups.renderer/desktopRenderer.js +++ b/resources/ext.popups.renderer/desktopRenderer.js @@ -1,7 +1,8 @@ /*global popupDelay: true, popupHideDelay: true*/ ( function ( $, mw ) { - var logData = {}; + var closeTimer, openTimer, + logData = {}; /** * Logs the click on link or popup @@ -55,30 +56,12 @@ */ mw.popups.render.cache = {}; - /** - * The timer used to delay `closePopups` - * @property {jQuery.Deferred} closeTimer - */ - mw.popups.render.closeTimer = undefined; - - /** - * The timer used to delay sending the API request/opening the popup form cache - * @property {jQuery.Deferred} openTimer - */ - mw.popups.render.openTimer = undefined; - /** * The link the currently has a popup * @property {jQuery} currentLink */ mw.popups.render.currentLink = undefined; - /** - * Current API request - * @property {jQuery.Deferred} currentRequest - */ - mw.popups.render.currentRequest = undefined; - /** * Object to store all renderers * @property {Object} renderers @@ -104,8 +87,8 @@ mw.popups.render.currentLink && mw.popups.render.currentLink[ 0 ] === link[ 0 ] ) { - if ( mw.popups.render.closeTimer ) { - mw.popups.render.closeTimer.abort(); + if ( closeTimer ) { + closeTimer.abort(); } return; } @@ -134,13 +117,13 @@ link.off( 'click', logClickAction ).on( 'click', logClickAction ); if ( mw.popups.render.cache[ link.attr( 'href' ) ] ) { - mw.popups.render.openTimer = mw.popups.render.wait( mw.popups.render.POPUP_DELAY ) + openTimer = mw.popups.render.wait( mw.popups.render.POPUP_DELAY ) .done( function () { mw.popups.render.openPopup( link, event ); } ); } else { // Wait for timer before making API queries and showing hovercard - mw.popups.render.openTimer = mw.popups.render.wait( mw.popups.render.API_DELAY ) + openTimer = mw.popups.render.wait( mw.popups.render.API_DELAY ) .done( function () { var cachePopup, key, renderers = mw.popups.render.renderers; @@ -159,9 +142,9 @@ cachePopup = mw.popups.render.renderers.article.init( link, $.extend( {}, logData ) ); } - mw.popups.render.openTimer = mw.popups.render.wait( mw.popups.render.POPUP_DELAY - mw.popups.render.API_DELAY ); + openTimer = mw.popups.render.wait( mw.popups.render.POPUP_DELAY - mw.popups.render.API_DELAY ); - $.when( mw.popups.render.openTimer, cachePopup ).done( function () { + $.when( openTimer, cachePopup ).done( function () { mw.popups.render.openPopup( link, event ); } ); } ); @@ -195,8 +178,8 @@ .attr( 'aria-hidden', 'false' ) .on( 'mouseleave', mw.popups.render.leaveActive ) .on( 'mouseenter', function () { - if ( mw.popups.render.closeTimer ) { - mw.popups.render.closeTimer.abort(); + if ( closeTimer ) { + closeTimer.abort(); } } ); @@ -278,8 +261,8 @@ } } ); - if ( mw.popups.render.closeTimer ) { - mw.popups.render.closeTimer.abort(); + if ( closeTimer ) { + closeTimer.abort(); } $( document ).off( 'keydown', mw.popups.render.closeOnEsc ); @@ -330,12 +313,11 @@ * @method leaveActive */ mw.popups.render.leaveActive = function () { - mw.popups.render.closeTimer = mw.popups.render.wait( mw.popups.render.POPUP_CLOSE_DELAY ).done( function () { + closeTimer = mw.popups.render.wait( mw.popups.render.POPUP_CLOSE_DELAY ).done( function () { mw.track( 'ext.popups.schemaPopups', $.extend( {}, logData, { action: 'dismissed', totalInteractionTime: Math.round( mw.now() - logData.dwellStartTime ) } ) ); - mw.popups.render.closePopup(); } ); }; @@ -357,12 +339,10 @@ } // TODO: should `blur` also be here? $( mw.popups.render.currentLink ).off( 'mouseleave', mw.popups.render.leaveInactive ); - if ( mw.popups.render.openTimer ) { - mw.popups.render.openTimer.abort(); - } - if ( mw.popups.render.currentRequest ) { - mw.popups.render.currentRequest.abort(); + if ( openTimer ) { + openTimer.abort(); } + mw.popups.render.abortCurrentRequest(); mw.popups.render.reset(); }; @@ -375,9 +355,9 @@ mw.popups.render.reset = function () { logData = {}; mw.popups.render.currentLink = undefined; - mw.popups.render.currentRequest = undefined; - mw.popups.render.openTimer = undefined; - mw.popups.render.closeTimer = undefined; + mw.popups.render.abortCurrentRequest(); + openTimer = undefined; + closeTimer = undefined; }; } )( jQuery, mediaWiki );