Revert "Revert "Fix tooltip interactions""

This reverts commit 11431dd2f6.
It also applies the fix for the original error.

Change-Id: Ib86534ffbcd20f64c8ba06c23f2e8af509437cfe

resources/ext.popups.core/ext.popups.core.js

@@ -36,29 +36,35 @@
 	];
 
 	/**
-	 * Temporarily remove the title attribute of the links so that
-	 * the yellow tooltips don't show up alongside the Hovercard.
+	 * Temporarily remove the title attribute of a link so that
+	 * the tooltip doesn't show up alongside the Hovercard.
 	 *
-	 * @method removeTooltips
+	 * @method removeTooltip
+	 * @param {jQuery.Object} $link link from which to strip 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' ) );
-			} );
+	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' ) );
 	};
 
 	/**

resources/ext.popups.renderer.desktopRenderer/desktopRenderer.js

@@ -373,8 +373,7 @@
 			action: 'dwelledButAbandoned'
 		} ) );
 
-		// TODO: should `blur` also be here?
-		$activeLink.off( 'mouseleave', leaveInactive );
+		$activeLink.off( 'mouseleave blur', leaveInactive );
 		if ( openTimer ) {
 			openTimer.abort();
 		}

resources/ext.popups.targets.desktopTarget/desktopTarget.js

@@ -18,7 +18,7 @@
 		var $this = $( this ),
 			data = event.data || {};
 
-		$this.off( 'mouseleave blur', onLinkAbandon );
+		$this.off( 'mouseleave.popups blur.popups', onLinkAbandon );
 
 		mw.track( 'ext.popups.event', {
 			pageTitleHover: $this.attr( 'title' ),
@@ -52,19 +52,28 @@
 				return;
 			}
 
-			mw.popups.removeTooltips( $link );
+			mw.popups.removeTooltip( $link );
 			mw.popups.render.render( $link, event, eventData.linkInteractionToken );
 		} else {
+			// Remove existing handlers and replace with logging only
 			$link
-				.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
+				.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
 				// 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.
@@ -77,7 +86,7 @@
 			href = $this.attr( 'href' ),
 			data = event.data || {};
 
-		$this.off( 'click', onLinkClick );
+		$this.off( 'click.popups', onLinkClick );
 
 		mw.track( 'ext.popups.event', {
 			pageTitleHover: $this.attr( 'title' ),
@@ -146,11 +155,11 @@
 	 * @method checkScroll
 	 */
 	mw.popups.checkScroll = function () {
-		$( window ).on( 'scroll', function () {
+		$( window ).on( 'scroll.popups', function () {
 			mw.popups.scrolled = true;
 		} );
 
-		$( window ).on( 'mousemove', function () {
+		$( window ).on( 'mousemove.popups', function () {
 			mw.popups.scrolled = false;
 		} );
 	};
@@ -166,7 +175,8 @@
 	function setupMouseEvents( $content ) {
 		mw.popups.$content = $content;
 		mw.popups.selectPopupElements()
-			.on( 'mouseenter focus', onLinkHover );
+			.on( 'mouseenter.popups focus.popups', onLinkHover )
+			.on( 'mouseleave.popups blur.popups', onLinkBlur );
 	}
 
 	mw.hook( 'wikipage.content' ).add( setupMouseEvents );

tests/qunit/ext.popups.core.test.js

@@ -33,7 +33,9 @@
 		}
 	} );
 
-	QUnit.test( 'removeTooltips', function ( assert ) {
+	// 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 ) {
 		var $link = $( '<a>', {
 			text: 'link with tooltip',
 			title: 'link title',
@@ -42,22 +44,20 @@
 
 		QUnit.expect( 5 );
 
-		mw.popups.removeTooltips( $link );
+		mw.popups.removeTooltip( $link );
+		assert.equal( $link.attr( 'title' ), '', 'The title should be removed by `removeTooltip`.' );
 
-		$link.trigger( 'mouseenter' );
-		assert.equal( $link.attr( 'title' ), '', 'The link does not have a title on mouseenter.' );
+		mw.popups.restoreTooltip( $link );
+		assert.equal( $link.attr( 'title' ), 'link title', 'The title should be restored by `restoreTooltip`.' );
 
-		$link.trigger( 'mouseleave' );
-		assert.equal( $link.attr( 'title' ), 'link title', 'The link has a title on mouseleave.' );
+		mw.popups.removeTooltip( $link );
+		assert.equal( $link.attr( 'title' ), '', 'Multiple calls should still remove 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.' );
+		mw.popups.restoreTooltip( $link );
+		assert.equal( $link.attr( 'title' ), 'link title', 'Multiple calls should still restore title attribute.' );
 
 		$link.data( 'dont-empty-title', true );
-		$link.trigger( 'mouseenter' );
+		mw.popups.removeTooltip( $link );
 		assert.equal( $link.attr( 'title' ), 'link title',
 			'The link title is not removed when `dont-empty-title` data attribute is `true`.' );