From fca9b5c0e6c797b0120325dfb4ec1b64c48f73e6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Niklas=20Laxstr=C3=B6m?= <niklas.laxstrom@gmail.com>
Date: Wed, 31 Jan 2018 15:29:04 +0100
Subject: [PATCH] Make ULS trigger and settings more keyboard accessible

It's now possible to tab to "display settings" and "input settings"
and access them with enter or space. Also escape can now be used
to close the settings screen.

The ULS trigger in interlanguage position is a button to get
native accessibility features.

Also removed `mw.hook( 'mw.uls.settings.open' ).fire( 'uls' );` as
it didn't seem very useful and there wasn't immediately obvious
place to put it. The existing click handler could be removed because
the settings dialog themselves place event listeners.

The patch is a bit longer than strictly necessary because the CSS
was mess (rules in different modules, poorly organized) and I had
to bring related rules together to understand them.

Bug: T52793
Change-Id: Id37c2665b1c97b81ef57be27a1abfae0db6b34d5
---
 resources/css/ext.uls.displaysettings.less  |   9 --
 resources/css/ext.uls.inputsettings.less    |   9 --
 resources/css/ext.uls.interface.less        |  35 ++++--
 resources/css/ext.uls.interlanguage.less    |  18 ++-
 resources/css/ext.uls.languagesettings.less | 130 +++++++++++---------
 resources/js/ext.uls.interface.js           | 113 ++++-------------
 resources/js/ext.uls.languagesettings.js    |  13 +-
 7 files changed, 151 insertions(+), 176 deletions(-)

diff --git a/resources/css/ext.uls.displaysettings.less b/resources/css/ext.uls.displaysettings.less
index a6ee5638..a15c31ef 100644
--- a/resources/css/ext.uls.displaysettings.less
+++ b/resources/css/ext.uls.displaysettings.less
@@ -1,14 +1,5 @@
 @import 'mediawiki.mixins';
 
-div.display-settings-block {
-	background: transparent no-repeat left top;
-	.background-image-svg( '../images/display.svg', '../images/display.png' );
-	background-size: 20px auto;
-	color: #222;
-	padding-left: 26px;
-	cursor: pointer;
-}
-
 .uls-display-settings-tab-switcher {
 	text-align: center;
 }
diff --git a/resources/css/ext.uls.inputsettings.less b/resources/css/ext.uls.inputsettings.less
index 42481170..36094e55 100644
--- a/resources/css/ext.uls.inputsettings.less
+++ b/resources/css/ext.uls.inputsettings.less
@@ -1,14 +1,5 @@
 @import 'mediawiki.mixins';
 
-.input-settings-block {
-	background: transparent no-repeat left top;
-	.background-image-svg( '../images/input.svg', '../images/input.png' );
-	background-size: 20px auto;
-	color: #222;
-	padding-left: 26px;
-	cursor: pointer;
-}
-
 .imelabel {
 	display: block;
 	padding-bottom: 10px;
diff --git a/resources/css/ext.uls.interface.less b/resources/css/ext.uls.interface.less
index 899a7620..d8003ad3 100644
--- a/resources/css/ext.uls.interface.less
+++ b/resources/css/ext.uls.interface.less
@@ -7,16 +7,31 @@
 	padding-left: 10px;
 	line-height: 1.2em;
 	border-radius: 0 0 2px 2px;
-}
 
-#uls-settings-block div.display-settings-block,
-#uls-settings-block div.input-settings-block {
-	display: inline-block;
-	margin: 8px 15px;
-	color: #54595d;
-}
+	> button {
+		background: left top transparent no-repeat;
+		// Support: Safari 3.1-6.1 & Android browser 3-4.3
+		// Don't support `background-size` values in `background` shorthand
+		background-size: 20px auto;
+		color: #54595d;
+		display: inline-block;
+		// Normalize the button
+		margin: 8px 15px;
+		border: 0;
+		padding: 0 0 0 26px;
+		font-size: medium;
+		cursor: pointer;
 
-#uls-settings-block div.display-settings-block:hover,
-#uls-settings-block div.input-settings-block:hover {
-	color: #222;
+		&:hover {
+			color: #222;
+		}
+
+		&.display-settings-block {
+			.background-image-svg( '../images/display.svg', '../images/display.png' );
+		}
+
+		&.input-settings-block {
+			.background-image-svg( '../images/input.svg', '../images/input.png' );
+		}
+	}
 }
diff --git a/resources/css/ext.uls.interlanguage.less b/resources/css/ext.uls.interlanguage.less
index 43ce9479..20d784ea 100644
--- a/resources/css/ext.uls.interlanguage.less
+++ b/resources/css/ext.uls.interlanguage.less
@@ -2,12 +2,22 @@
 @import 'mediawiki.mixins';
 
 #p-lang .uls-settings-trigger {
-	background: transparent no-repeat right top;
+	background: transparent no-repeat center top;
 	.background-image-svg( '../images/cog-sprite.svg', '../images/cog-sprite.png' );
-	height: 16px;
-	width: 14px;
+	border: 0;
+	min-height: 16px;
+	min-width: 16px;
 	float: right;
 	cursor: pointer;
+
+	// Remove the dotted border for Firefox
+	&::-moz-focus-inner {
+		border: 0;
+	}
+
+	&:focus {
+		outline: 1px solid #36c;
+	}
 }
 
 .skin-vector #p-lang .uls-settings-trigger {
@@ -16,5 +26,5 @@
 }
 
 #p-lang .uls-settings-trigger:hover {
-	background-position: right -16px;
+	background-position: center -16px;
 }
diff --git a/resources/css/ext.uls.languagesettings.less b/resources/css/ext.uls.languagesettings.less
index 7b1e437d..e92b2115 100644
--- a/resources/css/ext.uls.languagesettings.less
+++ b/resources/css/ext.uls.languagesettings.less
@@ -1,14 +1,6 @@
 @import 'mediawiki.mixins';
 
 /* stylelint-disable selector-max-id */
-#uls-settings-block {
-	background-color: #f8f9fa;
-	border-top: 1px solid #c8ccd1;
-	padding-left: 10px;
-	line-height: 1.2em;
-	border-radius: 0 0 2px 2px;
-}
-
 .uls-menu h3 {
 	border-bottom: medium none;
 	font-size: 14pt;
@@ -18,32 +10,84 @@
 	padding-left: 0;
 }
 
-#languagesettings-panels .languagesettings-menu {
-	padding-left: 0;
-	padding-right: 0;
-}
+.languagesettings-menu {
 
-.settings-title {
-	font-size: 13pt;
-}
+	/* Override grid padding */
+	.grid & {
+		padding-left: 0;
+		padding-right: 0;
+	}
 
-.settings-text {
-	color: #54595d;
-	font-size: 9pt;
-}
+	h1 {
+		color: #000;
+		font-size: 16pt;
+		line-height: 20pt;
+		margin-top: 0;
+		font-weight: normal;
+		border: 0;
+		padding: 8px 0 3px 15px;
+	}
 
-.display-settings-block:hover .settings-text {
-	color: #222;
-}
+	.menu-section {
+		/* Unstyle the button. Other properties seem already overriden. */
+		text-align: left;
+		width: 100%;
+		/* Using border instead of outline for focus (outline doesn't have enough space) */
+		outline: 0;
 
-.languagesettings-menu h1 {
-	color: #000;
-	font-size: 16pt;
-	line-height: 20pt;
-	margin-top: 0;
-	font-weight: normal;
-	border: 0;
-	padding: 8px 0 3px 15px;
+		color: #222;
+		padding: 5px 0 5px 42px;
+		border: 1px solid transparent;
+		background: left 15px top 6px transparent no-repeat;
+		background-size: 20px auto;
+
+		cursor: pointer;
+
+		// Remove the dotted border for Firefox
+		&::-moz-focus-inner {
+			border: 0;
+		}
+
+		/* Active "tab" has thicker always visible left border. background-position
+		 * and padding must be tweaked to avoid the image from moving */
+		&.active {
+			color: #000;
+			background-color: #fff;
+			padding-left: 40px;
+			background-position: left 13px top 6px;
+			border-left-width: 3px;
+			border-left-color: #36c;
+		}
+
+		&:hover {
+			background-color: #fff;
+		}
+
+		&:focus {
+			border-color: #36c;
+		}
+
+		.settings-title {
+			font-size: 13pt;
+		}
+
+		.settings-text {
+			color: #54595d;
+			font-size: 9pt;
+		}
+
+		&:hover .settings-text {
+			color: #222;
+		}
+
+		&.display-settings-block {
+			.background-image-svg( '../images/display.svg', '../images/display.png' );
+		}
+
+		&.input-settings-block {
+			.background-image-svg( '../images/input.svg', '../images/input.png' );
+		}
+	}
 }
 
 #languagesettings-settings-panel {
@@ -65,15 +109,6 @@
 	font-size: 12pt;
 }
 
-/* Buttons in the selector panel */
-.menu-section {
-	color: #72777d;
-	padding: 5px 0 5px 50px;
-	border-left: 3px solid transparent;
-	display: block;
-	font-size: 12pt;
-}
-
 .language-settings-dialog {
 	background: #f8f9fa;
 	/* Ensure base font size is same in different skins */
@@ -84,23 +119,6 @@
 	background: #fff;
 }
 
-#languagesettings-panels .menu-section.active,
-#languagesettings-panels .menu-section.active:hover {
-	background-color: #fff;
-	color: #000;
-	border-left-color: #36c;
-}
-
-.settings-menu-items .menu-section {
-	background-position: 6% 6px;
-	padding-left: 40px;
-	cursor: pointer;
-}
-
-.settings-menu-items .menu-section:hover {
-	background-color: #fff;
-}
-
 .language-settings-buttons {
 	border-top: 1px solid #eaecf0;
 	margin-top: 25px;
diff --git a/resources/js/ext.uls.interface.js b/resources/js/ext.uls.interface.js
index ea5726df..120da8b6 100644
--- a/resources/js/ext.uls.interface.js
+++ b/resources/js/ext.uls.interface.js
@@ -26,18 +26,13 @@
 	 * @return {jQuery}
 	 */
 	function displaySettings() {
-		var $displaySettingsTitle, displaySettingsText, $displaySettings;
-
-		displaySettingsText = $.i18n( 'ext-uls-display-settings-desc' );
-		$displaySettingsTitle = $( '<div data-i18n="ext-uls-display-settings-title">' )
-			.addClass( 'settings-title' )
-			.attr( 'title', displaySettingsText );
-		$displaySettings = $( '<div>' )
+		return $( '<button>' )
 			.addClass( 'display-settings-block' )
-			.prop( 'id', 'display-settings-block' )
-			.append( $displaySettingsTitle.i18n() );
-
-		return $displaySettings;
+			.attr( {
+				title: $.i18n( 'ext-uls-display-settings-desc' ),
+				'data-i18n': 'ext-uls-display-settings-title'
+			} )
+			.i18n();
 	}
 
 	/**
@@ -46,18 +41,13 @@
 	 * @return {jQuery}
 	 */
 	function inputSettings() {
-		var $inputSettingsTitle, inputSettingsText, $inputSettings;
-
-		inputSettingsText = $.i18n( 'ext-uls-input-settings-desc' );
-		$inputSettingsTitle = $( '<div data-i18n="ext-uls-input-settings-title">' )
-			.addClass( 'settings-title' )
-			.attr( 'title', inputSettingsText );
-		$inputSettings = $( '<div>' )
+		return $( '<button>' )
 			.addClass( 'input-settings-block' )
-			.prop( 'id', 'input-settings-block' )
-			.append( $inputSettingsTitle.i18n() );
-
-		return $inputSettings;
+			.attr( {
+				title: $.i18n( 'ext-uls-input-settings-desc' ),
+				'data-i18n': 'ext-uls-input-settings-title'
+			} )
+			.i18n();
 	}
 
 	/**
@@ -91,12 +81,6 @@
 			$.extend( displaySettingsOptions, uls.position() );
 			$displaySettings.languagesettings( displaySettingsOptions ).click();
 		} );
-
-		// On every click
-		$displaySettings.on( 'click', function () {
-			mw.hook( 'mw.uls.settings.open' ).fire( 'uls' );
-			uls.hide();
-		} );
 	}
 
 	/**
@@ -121,51 +105,7 @@
 				top: position.top,
 				left: position.left
 			} ).click();
-		} );
 
-		// On every click
-		$inputSettings.on( 'click', function () {
-			mw.hook( 'mw.uls.settings.open' ).fire( 'uls' );
-			uls.hide();
-		} );
-	}
-
-	/**
-	 * Helper function to make the uls triggers accessible with the keyboard.
-	 *
-	 * @param {jQuery} $target One or more jQuery elements.
-	 * @since 2013.07
-	 */
-	function addAccessibilityFeatures( $target ) {
-		// tabindex=0 makes it appear when tabbing targets.
-		// See also http://www.w3.org/TR/wai-aria/roles#button
-		$target.attr( {
-			tabIndex: 0,
-			role: 'button',
-			'aria-haspopup': true
-		} );
-		// TODO:
-		// * aria-pressed true/false when popup is open
-		// * aria-controls to reference to the popup
-
-		// Remove outline when clicking
-		$target.click( function () {
-			$( this ).css( 'outline', 'none' );
-		} );
-		// Allow outline to appear again if keyboard activated
-		$target.blur( function () {
-			$( this ).css( 'outline', '' );
-		} );
-
-		// Make Enter act the same as clicking. This has the unfortunate side
-		// effect of removing the outline.
-		$target.keydown( function ( event ) {
-			// Enter
-			if ( event.keyCode === 13 ) {
-				$( this ).click();
-				event.preventDefault();
-				event.stopPropagation();
-			}
 		} );
 	}
 
@@ -304,8 +244,8 @@
 	}
 
 	function initInterface() {
-		var $triggers,
-			$pLang,
+		var $pLang,
+			clickHandler,
 			$ulsTrigger = $( '.uls-trigger' ),
 			anonMode = ( mw.user.isAnon() &&
 				!mw.config.get( 'wgULSAnonCanChangeLanguage' ) ),
@@ -316,7 +256,8 @@
 			// The interlanguage links section
 			$pLang = $( '#p-lang' );
 			// Add an element near the interlanguage links header
-			$ulsTrigger = $( '<span>' ).addClass( 'uls-settings-trigger' );
+			$ulsTrigger = $( '<button>' )
+				.addClass( 'uls-settings-trigger' );
 			// Append ULS cog to languages section.
 			$pLang.prepend( $ulsTrigger );
 			// Take care of any other elements with this class.
@@ -333,7 +274,7 @@
 				title: mw.msg( 'ext-uls-select-language-settings-icon-tooltip' )
 			} );
 
-			$ulsTrigger.on( 'click', function ( e, eventParams ) {
+			clickHandler = function ( e, eventParams ) {
 				var languagesettings = $ulsTrigger.data( 'languagesettings' ),
 					languageSettingsOptions;
 
@@ -348,7 +289,7 @@
 						onVisible: function () {
 							var caretRadius,
 								ulsTriggerHeight = this.$element.height(),
-								ulsTriggerWidth = this.$element.width(),
+								ulsTriggerWidth = this.$element[ 0 ].offsetWidth,
 								ulsTriggerOffset = this.$element.offset();
 
 							this.$window.addClass( 'callout' );
@@ -367,10 +308,10 @@
 
 							// The top of the dialog is aligned in relation to
 							// the middle of the trigger, so that middle of the
-							// caret aligns with it. 17 is a random number.
+							// caret aligns with it. 16 is trigger icon height in pixels
 							this.top = ulsTriggerOffset.top +
 								( ulsTriggerHeight / 2 ) -
-								( caretRadius + 17 );
+								( caretRadius + 16 );
 
 							this.position();
 						}
@@ -382,9 +323,9 @@
 
 					e.stopPropagation();
 				}
-			} );
+			};
 		} else if ( anonMode ) {
-			$ulsTrigger.on( 'click', function ( e, eventParams ) {
+			clickHandler = function ( e, eventParams ) {
 				var languagesettings = $ulsTrigger.data( 'languagesettings' );
 
 				e.preventDefault();
@@ -400,9 +341,9 @@
 						$ulsTrigger.trigger( 'click', eventParams );
 					} );
 				}
-			} );
+			};
 		} else {
-			$ulsTrigger.on( 'click', function ( e, eventParams ) {
+			clickHandler = function ( e, eventParams ) {
 				var uls = $ulsTrigger.data( 'uls' );
 
 				e.preventDefault();
@@ -436,12 +377,10 @@
 						}, 0 );
 					} );
 				}
-			} );
+			};
 		}
 
-		// At this point we don't care which kind of trigger it is
-		$triggers = $( '.uls-settings-trigger, .uls-trigger' );
-		addAccessibilityFeatures( $triggers );
+		$ulsTrigger.on( 'click', clickHandler );
 
 		// Bind language settings to preferences page link
 		$( '#uls-preferences-link' )
diff --git a/resources/js/ext.uls.languagesettings.js b/resources/js/ext.uls.languagesettings.js
index b806d193..12dd92f8 100644
--- a/resources/js/ext.uls.languagesettings.js
+++ b/resources/js/ext.uls.languagesettings.js
@@ -87,6 +87,15 @@
 			this.$window.on( 'click', function ( event ) {
 				event.stopPropagation();
 			} );
+
+			// Map Escape to same action as the close button. This is keyup (and not keydown)
+			// because ULS also listens to keyup and we need to stop propagation.
+			this.$window.on( 'keyup', function ( event ) {
+				if ( event.which === 27 ) {
+					event.stopPropagation();
+					mw.hook( 'mw.uls.settings.cancel' ).fire();
+				}
+			} );
 		},
 
 		render: function () {
@@ -130,7 +139,7 @@
 			$settingsText = $( '<span>' )
 				.addClass( 'settings-text' )
 				.attr( 'data-i18n', module.descriptionI18n );
-			$settingsLink = $( '<div>' )
+			$settingsLink = $( '<button>' )
 				.addClass( moduleName + '-settings-block menu-section' )
 				.prop( 'id', moduleName + '-panel-trigger' )
 				.data( 'module', module )
@@ -194,6 +203,8 @@
 			this.$window.show();
 			this.visible();
 			this.$window.scrollIntoView();
+			// For keyboard navigation, put the focus on an element inside the dialog
+			this.$window.find( '.menu-section.active' ).focus();
 		},
 
 		/**