From 11431dd2f64cbfe3d4e7368a96ca03595eff413a Mon Sep 17 00:00:00 2001 From: Jdlrobson Date: Thu, 3 Nov 2016 17:46:41 +0000 Subject: [PATCH] Revert "Fix tooltip interactions" There is one reference to removeTooltips in ext.popups.targets.desktopTarget. I suspect this got broken in a rebase. It seems Jenkins only runs browser tests post merge. We should fix that. Please resubmit the patch with this amended. This reverts commit 0ff40a6532b57001b753d29c47b7477d905a9a39. Change-Id: Idd8dffb853db760ebc5866190d008f173e3025ba --- resources/ext.popups.core/ext.popups.core.js | 48 ++++++++----------- .../desktopRenderer.js | 3 +- .../desktopTarget.js | 26 ++++------ tests/qunit/ext.popups.core.test.js | 24 +++++----- 4 files changed, 43 insertions(+), 58 deletions(-) diff --git a/resources/ext.popups.core/ext.popups.core.js b/resources/ext.popups.core/ext.popups.core.js index 1baa5a7e7..cbca4c95d 100644 --- a/resources/ext.popups.core/ext.popups.core.js +++ b/resources/ext.popups.core/ext.popups.core.js @@ -36,35 +36,29 @@ ]; /** - * Temporarily remove the title attribute of a link so that - * the tooltip doesn't show up alongside the Hovercard. + * Temporarily remove the title attribute of the links so that + * the yellow tooltips don't show up alongside the Hovercard. * - * @method removeTooltip - * @param {jQuery.Object} $link link from which to strip title + * @method removeTooltips */ - mw.popups.removeTooltip = function ( $link ) { - // We shouldn't empty the title attribute of links that - // can't have Hovercards, ie. TextExtracts didn't return - // anything. It's set in the article.init after attempting - // to make an API request. - if ( - $link.data( 'dont-empty-title' ) !== true && - $link.filter( '[title]:not([title=""])' ).length - ) { - $link - .data( 'title', $link.attr( 'title' ) ) - .attr( 'title', '' ); - } - }; - - /** - * Restore previously-removed title attribute. - * - * @method restoreTooltip - * @param {jQuery.Object} $link link to which to restore title - */ - mw.popups.restoreTooltip = function ( $link ) { - $link.attr( 'title', $link.data( 'title' ) ); + mw.popups.removeTooltips = function ( $elements ) { + $elements + .filter( '[title]:not([title=""])' ) + .on( 'mouseenter focus', function () { + // We shouldn't empty the title attribute of links that + // can't have Hovercards, ie. TextExtracts didn't return + // anything. Its set in the article.init after attempting + // to make an API request. + if ( $( this ).data( 'dont-empty-title' ) !== true ) { + $( this ) + .data( 'title', $( this ).attr( 'title' ) ) + .attr( 'title', '' ); + } + } ) + .on( 'mouseleave blur', function () { + $( this ) + .attr( 'title', $( this ).data( 'title' ) ); + } ); }; /** diff --git a/resources/ext.popups.renderer.desktopRenderer/desktopRenderer.js b/resources/ext.popups.renderer.desktopRenderer/desktopRenderer.js index b94edd9db..368f0885a 100644 --- a/resources/ext.popups.renderer.desktopRenderer/desktopRenderer.js +++ b/resources/ext.popups.renderer.desktopRenderer/desktopRenderer.js @@ -373,7 +373,8 @@ action: 'dwelledButAbandoned' } ) ); - $activeLink.off( 'mouseleave blur', leaveInactive ); + // TODO: should `blur` also be here? + $activeLink.off( 'mouseleave', leaveInactive ); if ( openTimer ) { openTimer.abort(); } diff --git a/resources/ext.popups.targets.desktopTarget/desktopTarget.js b/resources/ext.popups.targets.desktopTarget/desktopTarget.js index fec773a1a..f171bfcc4 100644 --- a/resources/ext.popups.targets.desktopTarget/desktopTarget.js +++ b/resources/ext.popups.targets.desktopTarget/desktopTarget.js @@ -18,7 +18,7 @@ var $this = $( this ), data = event.data || {}; - $this.off( 'mouseleave.popups blur.popups', onLinkAbandon ); + $this.off( 'mouseleave blur', onLinkAbandon ); mw.track( 'ext.popups.event', { pageTitleHover: $this.attr( 'title' ), @@ -55,25 +55,16 @@ mw.popups.removeTooltips( $link ); mw.popups.render.render( $link, event, eventData.linkInteractionToken ); } else { - // Remove existing handlers and replace with logging only $link - .off( 'mouseleave.popups blur.popups mouseenter.popups focus.popups click.popups' ) - // We are passing the same data, rather than a shared object, into two different - // functions so that one function doesn't change the data and + .off( 'mouseleave.popups blur.popups click.popups' ) + // We are passing the same data, rather than a shared object, into two different functions. + // The reason is that we don't want one function to change the data and // have a side-effect on the other function's data. .on( 'mouseleave.popups blur.popups', eventData, onLinkAbandon ) .on( 'click.popups', eventData, onLinkClick ); } } - /** - * `mouseleave` and `blur` events handler for links that are eligible for - * popups. Handles the restoration of title attributes - */ - function onLinkBlur() { - mw.popups.restoreTooltip( $( this ) ); - } - /** * `click` event handler for links that are eligible for popups, but when * Popups are disabled. @@ -86,7 +77,7 @@ href = $this.attr( 'href' ), data = event.data || {}; - $this.off( 'click.popups', onLinkClick ); + $this.off( 'click', onLinkClick ); mw.track( 'ext.popups.event', { pageTitleHover: $this.attr( 'title' ), @@ -155,11 +146,11 @@ * @method checkScroll */ mw.popups.checkScroll = function () { - $( window ).on( 'scroll.popups', function () { + $( window ).on( 'scroll', function () { mw.popups.scrolled = true; } ); - $( window ).on( 'mousemove.popups', function () { + $( window ).on( 'mousemove', function () { mw.popups.scrolled = false; } ); }; @@ -175,8 +166,7 @@ function setupMouseEvents( $content ) { mw.popups.$content = $content; mw.popups.selectPopupElements() - .on( 'mouseenter.popups focus.popups', onLinkHover ) - .on( 'mouseleave.popups blur.popups', onLinkBlur ); + .on( 'mouseenter focus', onLinkHover ); } mw.hook( 'wikipage.content' ).add( setupMouseEvents ); diff --git a/tests/qunit/ext.popups.core.test.js b/tests/qunit/ext.popups.core.test.js index 9069a8bc6..360c2eec7 100644 --- a/tests/qunit/ext.popups.core.test.js +++ b/tests/qunit/ext.popups.core.test.js @@ -33,9 +33,7 @@ } } ); - // FIXME: This test should be split to cover each function separately and a browser test should - // be created to test user interactions with focus and mouseenter - planned for the Hovercards rewrite - QUnit.test( 'removeTooltip and restoreTooltip', function ( assert ) { + QUnit.test( 'removeTooltips', function ( assert ) { var $link = $( '', { text: 'link with tooltip', title: 'link title', @@ -44,20 +42,22 @@ QUnit.expect( 5 ); - mw.popups.removeTooltip( $link ); - assert.equal( $link.attr( 'title' ), '', 'The title should be removed by `removeTooltip`.' ); + mw.popups.removeTooltips( $link ); - mw.popups.restoreTooltip( $link ); - assert.equal( $link.attr( 'title' ), 'link title', 'The title should be restored by `restoreTooltip`.' ); + $link.trigger( 'mouseenter' ); + assert.equal( $link.attr( 'title' ), '', 'The link does not have a title on mouseenter.' ); - mw.popups.removeTooltip( $link ); - assert.equal( $link.attr( 'title' ), '', 'Multiple calls should still remove title attribute' ); + $link.trigger( 'mouseleave' ); + assert.equal( $link.attr( 'title' ), 'link title', 'The link has a title on mouseleave.' ); - mw.popups.restoreTooltip( $link ); - assert.equal( $link.attr( 'title' ), 'link title', 'Multiple calls should still restore title attribute.' ); + $link.trigger( 'focus' ); + assert.equal( $link.attr( 'title' ), '', 'The link does not have a title on focus.' ); + + $link.trigger( 'blur' ); + assert.equal( $link.attr( 'title' ), 'link title', 'The link has a title on blur.' ); $link.data( 'dont-empty-title', true ); - mw.popups.removeTooltip( $link ); + $link.trigger( 'mouseenter' ); assert.equal( $link.attr( 'title' ), 'link title', 'The link title is not removed when `dont-empty-title` data attribute is `true`.' );