Standardize use of buttons and icons in Minerva

* Adds button hover/focus states to all Minerva button icons
* Removes the deprecated mw-ui-icon-before selector

Depends-On: I4eb28eae4c4e23d58f1f85bc41c0caf77197d8a1
Bug: T288678
Change-Id: I490534f9f704a733191b459c8ee071848c436001

components/ToggleList/ToggleList.less

@@ -8,32 +8,6 @@
 	opacity: 0;
 }
 
-.toggle-list__toggle {
-	// labels are inline by default and this is also an icon
-	display: inline-block;
-	// Use the hand icon for the toggle button which is actually a checkbox label.
-	cursor: pointer;
-}
-
-.toggle-list__checkbox:focus + .toggle-list__toggle {
-	// The toggle button / label itself cannot receive focus but the underlying checkbox can. Keep
-	// the button and checkbox focus presentation in sync. From
-	// resources/src/mediawiki.toc.styles/screen.less.
-	outline: dotted 1px; /* Firefox style for focus */
-	outline: auto @colorProgressiveHighlight; /* Webkit style for focus */
-}
-
-.toggle-list__checkbox:checked + .toggle-list__toggle {
-	// show background when the toggle list is open
-	outline: 0;
-	background: rgba( 1, 1, 1, 0.1 );
-}
-
-.touch-events .toggle-list__checkbox:focus + .toggle-list__toggle {
-	// Buttons have no focus outline on mobile.
-	outline: 0;
-}
-
 .toggle-list__list {
 	background: @background-color-base;
 	// The menu appears over the content and occupies no room within it.

components/ToggleList/ToggleListItem.mustache

@@ -9,6 +9,7 @@
 	<li class="toggle-list-item">
 		<a class="toggle-list-item__anchor {{class}}" href="{{href}}"
 		   data-event-name="{{data-event-name}}" data-mw="interface">
+			{{#icon}}<span class="mw-ui-icon mw-ui-icon-{{.}}"></span>&nbsp;{{/icon}}
 			<span class="toggle-list-item__label">{{text}}</span>
 		</a>
 	</li>

includes/Menu/Definitions.php

@@ -28,7 +28,6 @@ use MediaWiki\Minerva\Menu\Entries\LogOutMenuEntry;
 use MediaWiki\Minerva\Menu\Entries\SingleMenuEntry;
 use MediaWiki\Special\SpecialPageFactory;
 use Message;
-use MinervaUI;
 use MWException;
 use MWHttpRequest;
 use SpecialMobileWatchlist;
@@ -176,10 +175,13 @@ final class Definitions {
 		$group->insert( 'random' )
 			->addComponent( $this->context->msg( 'mobile-frontend-random-button' )->text(),
 				Title::newFromText( $pageMsg->escaped() )->getLocalURL() . '#/random',
-				MinervaUI::iconClass( 'die', 'before' ), [
+				'',
+				[
 					'id' => 'randomButton',
 					'data-event-name' => 'menu.random',
-				] );
+				],
+				'minerva-die'
+			);
 	}
 
 	/**
@@ -194,8 +196,9 @@ final class Definitions {
 				->addComponent(
 					$this->context->msg( 'mobile-frontend-main-menu-nearby' )->text(),
 					SpecialPage::getTitleFor( 'Nearby' )->getLocalURL(),
-					MinervaUI::iconClass( 'mapPin', 'before', 'nearby' ),
-					[ 'data-event-name' => 'menu.nearby' ]
+					'',
+					[ 'data-event-name' => 'menu.nearby' ],
+					'minerva-mapPin'
 				);
 		}
 	}
@@ -288,8 +291,9 @@ final class Definitions {
 			->addComponent(
 				$this->context->msg( 'recentchanges' )->escaped(),
 				$title->getLocalURL(),
-				MinervaUI::iconClass( 'recentChanges', 'before' ),
-				[ 'data-event-name' => 'menu.recentchanges' ]
+				'',
+				[ 'data-event-name' => 'menu.recentchanges' ],
+				'minerva-recentChanges'
 			);
 	}
 
@@ -396,12 +400,13 @@ final class Definitions {
 		 $group->insert( 'donate' )->addComponent(
 			$ctx->msg( 'sitesupport' )->text(),
 			$url,
-			MinervaUI::iconClass( 'heart', 'before' ),
+			'',
 			[
 				// for consistency with desktop
 				'id' => 'n-sitesupport',
 				'data-event-name' => 'menu.donate',
-			]
+			],
+			'minerva-heart'
 		 );
 	 }
 }

includes/Menu/Entries/HomeMenuEntry.php

@@ -17,8 +17,6 @@
 
 namespace MediaWiki\Minerva\Menu\Entries;
 
-use MinervaUI;
-
 /**
  * Class for defining a home menu entry in Special:MobileMenu
  */
@@ -70,7 +68,7 @@ final class HomeMenuEntry implements IMenuEntry {
 		$this->component = [
 			'text' => $text,
 			'href' => $url,
-			'class' => trim( MinervaUI::iconClass( $name, 'before' ) )
+			'icon' => 'minerva-' . $name,
 		];
 		if ( $trackClicks !== false ) {
 			$eventName = $trackClicks === true ? $name : $trackClicks;

includes/Menu/Entries/LanguageSelectorEntry.php

@@ -18,6 +18,7 @@
 namespace MediaWiki\Minerva\Menu\Entries;
 
 use MessageLocalizer;
+use MinervaUI;
 use SpecialPage;
 use Title;
 
@@ -39,36 +40,49 @@ class LanguageSelectorEntry implements IMenuEntry {
 	 */
 	private $doesPageHaveLanguages;
 	/**
-	 * @var string An icon class generated via MinervaUI::iconClass()
+	 * @var string Associated icon name
 	 */
-	private $iconClass;
+	private $icon;
 
 	/**
 	 * @var string A translatable label used as text and title
 	 */
 	private $label;
 
+	/**
+	 * @var string additional classes
+	 */
+	private $classes;
+
 	/**
 	 * LanguageSelectorEntry constructor.
 	 * @param Title $title Current Title
 	 * @param bool $doesPageHaveLanguages Whether the page is also available in other
 	 * languages or variants
 	 * @param MessageLocalizer $messageLocalizer Used for translation texts
-	 * @param string $iconClass An icon class generated via MinervaUI::iconClass()
+	 * @param bool $isButton
+	 * @param string $classes page classes
 	 * @param string $label Menu entry label and title
 	 */
 	public function __construct(
 		Title $title,
 		$doesPageHaveLanguages,
 		MessageLocalizer $messageLocalizer,
-		$iconClass,
+		$isButton = false,
+		$classes = '',
 		$label = 'mobile-frontend-language-article-heading'
 	) {
 		$this->title = $title;
 		$this->doesPageHaveLanguages = $doesPageHaveLanguages;
 		$this->messageLocalizer = $messageLocalizer;
-		$this->iconClass = $iconClass;
+		$this->icon = 'wikimedia-language-base20';
 		$this->label = $label;
+		$this->classes = $classes;
+		if ( $isButton ) {
+			$this->classes .= MinervaUI::iconClass(
+				'language-base20', 'element', 'mw-ui-button mw-ui-quiet mw-ui-icon-with-label-desktop', 'wikimedia'
+			);
+		}
 	}
 
 	/**
@@ -105,7 +119,8 @@ class LanguageSelectorEntry implements IMenuEntry {
 		return [
 			[
 				'href' => $switcherLink,
-				'class' => $this->iconClass . $switcherClasses,
+				'icon' => $this->icon,
+				'class' => $this->classes . ' ' . $switcherClasses,
 				'text' => $msg,
 				'title' => $msg,
 				'data-event-name' => 'menu.languages'

includes/Menu/Entries/MenuEntry.php

@@ -81,14 +81,16 @@ final class MenuEntry implements IMenuEntry {
 	 * @param string $className Any additional CSS classes that should added to the output,
 	 *  separated by spaces
 	 * @param array $attrs Additional data that can be associated with the component
+	 * @param null|string $icon the icon identifier
 	 *
 	 * @return MenuEntry
 	 */
-	public function addComponent( $label, $url, $className = '', $attrs = [] ) {
+	public function addComponent( $label, $url, $className = '', $attrs = [], $icon = null ) {
 		$this->components[] = [
 			'text' => $label,
 			'href' => $url,
-			'class' => $className
+			'class' => $className,
+			'icon' => $icon,
 		] + $attrs;
 		return $this;
 	}

includes/Menu/Entries/ProfileMenuEntry.php

@@ -17,7 +17,6 @@
 
 namespace MediaWiki\Minerva\Menu\Entries;
 
-use MinervaUI;
 use Title;
 use User;
 
@@ -82,10 +81,9 @@ final class ProfileMenuEntry implements IProfileMenuEntry {
 	public function getComponents(): array {
 		$username = $this->user->getName();
 		return [ [
+			'icon' => 'wikimedia-userAvatar-base20',
 			'text' => $this->customProfileLabel ?? $username,
 			'href' => $this->customProfileURL ?? Title::newFromText( $username, NS_USER )->getLocalURL(),
-			'class' => MinervaUI::iconClass( 'userAvatar-base20',
-				'before', 'primary-action', 'wikimedia' ),
 			'data-event-name' => 'menu.' . (
 				$this->profileTrackingCode ?? self::DEFAULT_PROFILE_TRACKING_CODE )
 		] ];

includes/Menu/Entries/SingleMenuEntry.php

@@ -53,6 +53,7 @@ class SingleMenuEntry implements IMenuEntry {
 		$className .= 'menu__item--' . $name;
 
 		$this->attributes = [
+			'icon' => null,
 			'text' => $text,
 			'href' => $url,
 			'class' => $className
@@ -117,8 +118,13 @@ class SingleMenuEntry implements IMenuEntry {
 	public function setIcon( $iconName, $iconType = 'before',
 		$additionalClassNames = '', $iconPrefix = 'minerva'
 	) {
-		$this->attributes['class'] .= ' '
-			. MinervaUI::iconClass( $iconName, $iconType, $additionalClassNames, $iconPrefix );
+		if ( $iconType === 'before' ) {
+			$this->attributes['icon'] = $iconPrefix . '-' . $iconName;
+		} else {
+			$this->attributes['class'] .= ' ' . MinervaUI::iconClass(
+				$iconName, $iconType, $additionalClassNames, $iconPrefix
+			);
+		}
 		return $this;
 	}
 

includes/Menu/PageActions/ToolbarBuilder.php

@@ -146,9 +146,7 @@ class ToolbarBuilder {
 				$this->title,
 				$this->languagesHelper->doesTitleHasLanguagesOrVariants( $this->title ),
 				$this->messageLocalizer,
-				MinervaUI::iconClass(
-					'language-base20', 'element', 'mw-ui-icon-with-label-desktop', 'wikimedia'
-				)
+				true
 			) );
 		}
 

includes/Menu/PageActions/UserNamespaceOverflowBuilder.php

@@ -28,7 +28,6 @@ use MediaWiki\Minerva\Menu\Entries\SingleMenuEntry;
 use MediaWiki\Minerva\Menu\Group;
 use MediaWiki\Minerva\Permissions\IMinervaPagePermissions;
 use MessageLocalizer;
-use MinervaUI;
 use MWException;
 use Title;
 
@@ -84,8 +83,9 @@ class UserNamespaceOverflowBuilder implements IOverflowBuilder {
 				$this->title,
 				$this->languagesHelper->doesTitleHasLanguagesOrVariants( $this->title ),
 				$this->messageLocalizer,
-				MinervaUI::iconClass( 'language-base20', 'before',
-					'minerva-page-actions-language-switcher', 'wikimedia' ),
+				false,
+				// no additional classes
+				'',
 				'minerva-page-actions-language-switcher'
 			) );
 		}

includes/Menu/User/AdvancedUserMenuBuilder.php

@@ -75,7 +75,6 @@ final class AdvancedUserMenuBuilder implements IUserMenuBuilder {
 				$this->messageLocalizer->msg( 'mobile-frontend-user-page-talk' )->escaped(),
 				$talkPage->getLocalURL()
 			);
-			$entry->setIcon( 'userTalk', 'before' );
 			$group->insertEntry( $entry );
 		}
 		$sandbox = $personalTools['sandbox']['links'][0] ?? false;

includes/MinervaUI.php

@@ -42,6 +42,11 @@ class MinervaUI {
 		if ( $iconName ) {
 			$modifiers .= ' mw-ui-icon-' . $iconPrefix . '-' . $iconName;
 		}
+		if ( $iconType === 'element' ) {
+			$additionalClassNames .= ' mw-ui-button mw-ui-quiet';
+		} elseif ( $iconType === 'before' ) {
+			throw new RuntimeException( 'iconClass before type is no longer supported.' );
+		}
 		return $base . ' ' . $modifiers . ' ' . $additionalClassNames;
 	}
 

includes/Skins/menuGroup.mustache

@@ -1,6 +1,8 @@
 <li class="{{class}}">
   {{#components}}
     <a href="{{href}}" class="{{class}}"
-	   data-mw="interface" data-event-name="{{data-event-name}}"><span>{{text}}</span></a>
+	   data-mw="interface" data-event-name="{{data-event-name}}">
+      {{#icon}}<span class="mw-ui-icon-{{*}} mw-ui-icon"></span>{{/icon}}&nbsp;<span>{{text}}</span>
+     </a>
   {{/components}}
 </li>

includes/Skins/skin.mustache

@@ -6,7 +6,7 @@
 				<nav class="navigation-drawer toggle-list view-border-box">
 					<input type="checkbox" id="main-menu-input" class="toggle-list__checkbox" role="button" aria-labelledby="mw-mf-main-menu-button">
 					<label for="main-menu-input" id="mw-mf-main-menu-button"
-						class="mw-ui-icon mw-ui-icon-element mw-ui-icon-wikimedia-menu-base20 mw-ui-icon-flush-left toggle-list__toggle"
+						class=" mw-ui-button mw-ui-quiet mw-ui-icon mw-ui-icon-element mw-ui-icon-wikimedia-menu-base20 mw-ui-icon-flush-left toggle-list__toggle"
 						title="{{main-menu-tooltip}}" data-event-name="ui.mainmenu">{{main-menu-tooltip}}</label>
 					{{#data-main-menu}}{{>menu}}{{/data-main-menu}}
 					<label class="main-menu-mask" for="main-menu-input"></label>

resources/skins.minerva.base.styles/pageactions.less

@@ -45,16 +45,6 @@
 	align-items: center;
 	min-width: 0;
 	overflow: hidden;
-
-	> a {
-		// Special case outside of standard buttons styling due to surrounding
-		// interface elements, see T237019.
-		font-weight: 500;
-	}
-
-	li > *:hover {
-		box-shadow: none;
-	}
 }
 
 // Layout for less than 5 items - one item at the beginning, rest at the end.

tests/phpunit/menu/GroupTest.php

@@ -17,6 +17,7 @@ class GroupTest extends \MediaWikiTestCase {
 		'href' => '/Main_page',
 		'class' => 'mw-ui-icon mw-ui-icon-before mw-ui-icon-home',
 		'data-event-name' => 'home',
+		'icon' => null,
 	];
 
 	/** @var string[] */
@@ -24,6 +25,7 @@ class GroupTest extends \MediaWikiTestCase {
 		'text' => 'Nearby',
 		'href' => '/wiki/Special:Nearby',
 		'class' => 'mw-ui-icon mw-ui-icon-before mw-ui-icon-nearby',
+		'icon' => null
 	];
 
 	/**
@@ -181,12 +183,14 @@ class GroupTest extends \MediaWikiTestCase {
 			'href' => '/wiki/User:Phuedx_(WMF)',
 			'class' =>
 				'mw-ui-icon mw-ui-icon-before mw-ui-icon-profile truncated-text primary-action',
+			'icon' => null,
 		];
 		$authLogoutComponent = [
 			'text' => 'Logout',
 			'href' => '/wiki/Special:UserLogout',
 			'class' =>
 				'mw-ui-icon mw-ui-icon-element secondary-logout secondary-action truncated-text',
+			'icon' => null,
 		];
 
 		$menu = new Group( 'p-test' );

tests/phpunit/unit/menu/Entries/HomeMenuEntryTest.php

@@ -26,7 +26,7 @@ class HomeMenuEntryTest extends \MediaWikiUnitTestCase {
 		$this->assertSame( [ [
 			'text' => $text,
 			'href' => $url,
-			'class' => 'mw-ui-icon mw-ui-icon-before mw-ui-icon-minerva-foo',
+			'icon' => 'minerva-foo',
 			'data-event-name' => 'menu.foo'
 		] ], $entry->getComponents() );
 	}
@@ -41,8 +41,8 @@ class HomeMenuEntryTest extends \MediaWikiUnitTestCase {
 		$component = current( $entry->getComponents() );
 		$this->assertSame( 'bar', $component['text'] );
 		$this->assertSame(
-			'mw-ui-icon mw-ui-icon-before mw-ui-icon-minerva-foo',
-			$component['class']
+			'minerva-foo',
+			$component['icon']
 		);
 		$entry->overrideText( 'blah' )
 			->overrideCssClass( 'classy' );

tests/selenium/features/step_definitions/menu_steps.js

@@ -25,8 +25,8 @@ const iShouldSeeALinkInMenu = ( text ) => {
 };
 
 const iShouldSeeALinkToDisclaimer = () => {
-	ArticlePage.menu_element.$( '=Disclaimers' ).waitForDisplayed();
-	assert.strictEqual( ArticlePage.menu_element.$( '=Disclaimers' ).isDisplayed(), true );
+	ArticlePage.menu_element.$( 'span=Disclaimers' ).waitForDisplayed();
+	assert.strictEqual( ArticlePage.menu_element.$( 'span=Disclaimers' ).isDisplayed(), true );
 };
 
 module.exports = {