diff --git a/includes/Hooks.php b/includes/Hooks.php index 2a270d5f..32b51853 100644 --- a/includes/Hooks.php +++ b/includes/Hooks.php @@ -168,9 +168,9 @@ class Hooks implements // Promote watch link from actions to views and add an icon if ( $key !== null ) { - self::appendClassToListItem( - $content_navigation['actions'][$key], - 'icon' + self::appendClassToItem( + $content_navigation['actions'][$key]['class'], + [ 'icon' ] ); $content_navigation['views'][$key] = $content_navigation['actions'][$key]; unset( $content_navigation['actions'][$key] ); @@ -178,36 +178,30 @@ class Hooks implements } /** - * Updates class list on list item + * Adds class to a property * - * @param array &$item to update for use in makeListItem - * @param array $classes to add to the item - * @param bool $applyToLink (optional) and defaults to false. - * If set will modify `link-class` instead of `class` + * @param array &$item to update + * @param array|string $classes to add to the item */ - private static function addListItemClass( &$item, $classes, $applyToLink = false ) { - $property = $applyToLink ? 'link-class' : 'class'; - $existingClass = $item[$property] ?? []; + private static function appendClassToItem( &$item, $classes ) { + $existingClasses = $item; - if ( is_array( $existingClass ) ) { - $item[$property] = array_merge( $existingClass, $classes ); - } elseif ( is_string( $existingClass ) ) { - // treat as string - $item[$property] = array_merge( [ $existingClass ], $classes ); + if ( is_array( $existingClasses ) ) { + // Treat as array + $newArrayClasses = is_array( $classes ) ? $classes : [ trim( $classes ) ]; + $item = array_merge( $existingClasses, $newArrayClasses ); + } elseif ( is_string( $existingClasses ) ) { + // Treat as string + $newStrClasses = is_string( $classes ) ? trim( $classes ) : implode( ' ', $classes ); + $item .= ' ' . $newStrClasses; } else { - $item[$property] = $classes; + // Treat as whatever $classes is + $item = $classes; } - } - /** - * Updates the class on an existing item taking into account whether - * a class exists there already. - * - * @param array &$item - * @param string $newClass - */ - private static function appendClassToListItem( &$item, $newClass ) { - self::addListItemClass( $item, [ $newClass ] ); + if ( is_string( $item ) ) { + $item = trim( $item ); + } } /** @@ -227,12 +221,12 @@ class Hooks implements if ( isset( $content_navigation['user-page']['tmpuserpage'] ) ) { $content_navigation['user-page']['tmpuserpage']['collapsible'] = true; $content_navigation['user-page']['tmpuserpage'] = - self::updateMenuItem( $content_navigation['user-page']['tmpuserpage'] ); + self::updateMenuItemData( $content_navigation['user-page']['tmpuserpage'] ); } if ( isset( $content_navigation['user-menu']['tmpuserpage'] ) ) { $content_navigation['user-menu']['tmpuserpage']['collapsible'] = true; $content_navigation['user-menu']['tmpuserpage'] = - self::updateMenuItem( $content_navigation['user-menu']['tmpuserpage'] ); + self::updateMenuItemData( $content_navigation['user-menu']['tmpuserpage'] ); } } elseif ( $isRegistered ) { // Remove user page from personal menu dropdown for logged in use @@ -360,10 +354,7 @@ class Hooks implements */ private static function makeMenuItemCollapsible( array &$item, string $prefix = 'user-links-' ) { $COLLAPSE_MENU_ITEM_CLASS = $prefix . 'collapsible-item'; - self::appendClassToListItem( - $item, - $COLLAPSE_MENU_ITEM_CLASS - ); + self::appendClassToItem( $item[ 'class' ], $COLLAPSE_MENU_ITEM_CLASS ); } /** @@ -379,14 +370,15 @@ class Hooks implements } /** - * Updates template data for Vector menu items. + * Update template data to include classes and html that handle buttons, icons, and collapsible items. * - * @param array $item Menu data to update - * @param bool $isLinkData false if data is for li element (i.e. makeListItem()), - * true if for link element (i.e. makeLink()) - * @return array $item Updated menu data + * @internal for use inside Vector skin. + * @param array $item data to update + * @param string $buttonClassProp property to append button classes + * @param string $iconHtmlProp property to set icon HTML + * @return array $item Updated data */ - public static function updateMenuItem( $item, $isLinkData = false ) { + private static function updateItemData( $item, $buttonClassProp, $iconHtmlProp ) { $hasButton = $item['button'] ?? false; $hideText = $item['text-hidden'] ?? false; $isCollapsible = $item['collapsible'] ?? false; @@ -395,11 +387,12 @@ class Hooks implements unset( $item['icon'] ); unset( $item['text-hidden'] ); unset( $item['collapsible'] ); + if ( $isCollapsible ) { self::makeMenuItemCollapsible( $item ); } if ( $hasButton ) { - self::addListItemClass( $item, [ 'mw-ui-button', 'mw-ui-quiet' ], !$isLinkData ); + self::appendClassToItem( $item[ $buttonClassProp ], [ 'mw-ui-button', 'mw-ui-quiet' ] ); } if ( $icon ) { if ( $hideText ) { @@ -410,14 +403,50 @@ class Hooks implements // We should seek to remove all these instances. 'mw-ui-icon-wikimedia-' . $icon ]; - self::addListItemClass( $item, $iconElementClasses, !$isLinkData ); + self::appendClassToItem( $item[ $buttonClassProp ], $iconElementClasses ); } else { - $item['link-html'] = self::makeIcon( $icon ); + $item[ $iconHtmlProp ] = self::makeIcon( $icon ); } } return $item; } + /** + * Updates template data for Vector dropdown menus. + * + * @param array $item Menu data to update + * @return array $item Updated menu data + */ + public static function updateDropdownMenuData( $item ) { + $buttonClassProp = 'heading-class'; + $iconHtmlProp = 'html-vector-heading-icon'; + return self::updateItemData( $item, $buttonClassProp, $iconHtmlProp ); + } + + /** + * Updates template data for Vector link items. + * + * @param array $item link data to update + * @return array $item Updated link data + */ + public static function updateLinkData( $item ) { + $buttonClassProp = 'class'; + $iconHtmlProp = 'link-html'; + return self::updateItemData( $item, $buttonClassProp, $iconHtmlProp ); + } + + /** + * Updates template data for Vector menu items. + * + * @param array $item menu item data to update + * @return array $item Updated menu item data + */ + public static function updateMenuItemData( $item ) { + $buttonClassProp = 'link-class'; + $iconHtmlProp = 'link-html'; + return self::updateItemData( $item, $buttonClassProp, $iconHtmlProp ); + } + /** * Updates user interface preferences for modern Vector to upgrade icon/button menu items. * @@ -426,7 +455,7 @@ class Hooks implements */ private static function updateMenuItems( &$content_navigation, $menu ) { foreach ( $content_navigation[$menu] as $key => $item ) { - $content_navigation[$menu][$key] = self::updateMenuItem( $item ); + $content_navigation[$menu][$key] = self::updateMenuItemData( $item ); } } diff --git a/includes/SkinVector.php b/includes/SkinVector.php index 8c7d8ce5..e8d4f6bc 100644 --- a/includes/SkinVector.php +++ b/includes/SkinVector.php @@ -126,9 +126,7 @@ abstract class SkinVector extends SkinMustache { ]; private const SEARCH_SHOW_THUMBNAIL_CLASS = 'vector-search-box-show-thumbnail'; private const SEARCH_AUTO_EXPAND_WIDTH_CLASS = 'vector-search-box-auto-expand-width'; - private const CLASS_QUIET_BUTTON = 'mw-ui-button mw-ui-quiet'; private const CLASS_PROGRESSIVE = 'mw-ui-progressive'; - private const CLASS_ICON_BUTTON = 'mw-ui-icon mw-ui-icon-element'; /** * T243281: Code used to track clicks to opt-out link. @@ -142,17 +140,6 @@ abstract class SkinVector extends SkinMustache { */ private const OPT_OUT_LINK_TRACKING_CODE = 'vctw1'; - /** - * @param string $icon the name of the icon without wikimedia- prefix. - * @return string - */ - private function iconClass( $icon ) { - if ( $icon ) { - return 'mw-ui-icon-wikimedia-' . $icon; - } - return ''; - } - abstract protected function isLegacy(): bool; /** @@ -249,7 +236,7 @@ abstract class SkinVector extends SkinMustache { 'icon' => $isDropdownItem ? $createAccountData['icon'] : null, 'button' => !$isDropdownItem, ] ); - $createAccountData = Hooks::updateMenuItem( $createAccountData, true ); + $createAccountData = Hooks::updateLinkData( $createAccountData ); return $this->makeLink( 'create-account', $createAccountData ); } @@ -265,7 +252,7 @@ abstract class SkinVector extends SkinMustache { $loginLinkData = array_merge( $this->buildLoginData( $returnto, $useCombinedLoginLink ), [ 'class' => [ 'vector-menu-content-item', 'vector-menu-content-item-login' ], ] ); - $loginLinkData = Hooks::updateMenuItem( $loginLinkData, true ); + $loginLinkData = Hooks::updateLinkData( $loginLinkData ); $templateData = [ 'htmlCreateAccount' => $this->getCreateAccountHTML( $returnto, true ), 'htmlLogin' => $this->makeLink( 'login', $loginLinkData ), @@ -296,7 +283,7 @@ abstract class SkinVector extends SkinMustache { $logoutLinkData = array_merge( $this->buildLogoutLinkData(), [ 'class' => [ 'vector-menu-content-item', 'vector-menu-content-item-logout' ], ] ); - $logoutLinkData = Hooks::updateMenuItem( $logoutLinkData, true ); + $logoutLinkData = Hooks::updateLinkData( $logoutLinkData ); $templateParser = $this->getTemplateParser(); return $templateParser->processTemplate( 'UserLinks__logout', [ @@ -379,11 +366,9 @@ abstract class SkinVector extends SkinMustache { 'html-items' => '', 'html-vector-menu-checkbox-attributes' => 'tabindex="-1"', 'html-vector-menu-heading-attributes' => 'tabindex="-1"', - 'heading-class' => implode( ' ', [ - self::CLASS_QUIET_BUTTON, - self::CLASS_ICON_BUTTON, - $this->iconClass( 'listBullet' ) - ] ), + 'button' => true, + 'text-hidden' => true, + 'icon' => 'listBullet' ] ); // Show sticky ULS if the ULS extension is enabled and the ULS in header is not hidden @@ -728,9 +713,9 @@ abstract class SkinVector extends SkinMustache { $this->getULSLabel( 'vector-language-button-aria-label', $numLanguages ), // ext.uls.interface attaches click handler to this selector. 'checkbox-class' => ' mw-interlanguage-selector ', - 'html-vector-heading-icon' => Hooks::makeIcon( 'wikimedia-language-progressive' ), - 'heading-class' => self::CLASS_QUIET_BUTTON . ' ' . self::CLASS_PROGRESSIVE . - ' mw-portlet-lang-heading-' . strval( $numLanguages ), + 'icon' => 'language-progressive', + 'button' => true, + 'heading-class' => self::CLASS_PROGRESSIVE . ' mw-portlet-lang-heading-' . strval( $numLanguages ), ]; // Adds class to hide language button @@ -743,13 +728,43 @@ abstract class SkinVector extends SkinMustache { } /** - * helper for applying Vector menu classes to portlets + * Creates portlet data for the user menu dropdown + * + * @param array $portletData + * @return array + */ + private function getUserMenuPortletData( $portletData ) { + // Add target class to apply different icon to personal menu dropdown for logged in users. + $portletData['class'] .= ' vector-user-menu'; + $portletData['class'] .= $this->loggedin ? + ' vector-user-menu-logged-in' : + ' vector-user-menu-logged-out'; + if ( $this->getUser()->isTemp() ) { + $icon = 'userAnonymous'; + } elseif ( $this->loggedin ) { + $icon = 'userAvatar'; + } else { + $icon = 'ellipsis'; + // T287494 We use tooltip messages to provide title attributes on hover over certain menu icons. + // For modern Vector, the "tooltip-p-personal" key is set to "User menu" which is appropriate for + // the user icon (dropdown indicator for user links menu) for logged-in users. + // This overrides the tooltip for the user links menu icon which is an ellipsis for anonymous users. + $portletData['html-tooltip'] = Linker::tooltip( 'vector-anon-user-menu-title' ); + } + $portletData['icon'] = $icon; + $portletData['button'] = true; + $portletData['text-hidden'] = true; + return $portletData; + } + + /** + * Helper for applying Vector menu classes to portlets * * @param array $portletData returned by SkinMustache to decorate * @param int $type representing one of the menu types (see MENU_TYPE_* constants) * @return array modified version of portletData input */ - private function decoratePortletClass( + private function updatePortletClasses( array $portletData, int $type = self::MENU_TYPE_DEFAULT ) { @@ -762,47 +777,17 @@ abstract class SkinVector extends SkinMustache { if ( $this->isLegacy() ) { $extraClasses[self::MENU_TYPE_TABS] .= ' vector-menu-tabs-legacy'; } + $portletData['class'] .= ' ' . $extraClasses[$type]; + if ( !isset( $portletData['heading-class'] ) ) { $portletData['heading-class'] = ''; } - // Add target class to apply different icon to personal menu dropdown for logged in users. - if ( $portletData['id'] === 'p-personal' ) { - if ( $this->isLegacy() ) { - $portletData['class'] .= ' vector-user-menu-legacy'; - } else { - $portletData['class'] .= ' vector-user-menu'; - $portletData['class'] .= $this->loggedin ? - ' vector-user-menu-logged-in' : - ' vector-user-menu-logged-out'; - $portletData['heading-class'] .= ' ' . self::CLASS_QUIET_BUTTON . ' ' . - self::CLASS_ICON_BUTTON . ' '; - if ( $this->getUser()->isTemp() ) { - $icon = 'userAnonymous'; - } elseif ( $this->loggedin ) { - $icon = 'userAvatar'; - } else { - $icon = 'ellipsis'; - } - $portletData['heading-class'] .= $this->iconClass( $icon ); - } - } - switch ( $portletData['id'] ) { - case 'p-variants': - case 'p-cactions': - $portletData['class'] .= ' vector-menu-dropdown-noicon'; - break; - case 'p-vector-user-menu-overflow': - $portletData['class'] .= ' vector-user-menu-overflow'; - break; - default: - break; + if ( $type === self::MENU_TYPE_DROPDOWN ) { + $portletData = Hooks::updateDropdownMenuData( $portletData ); } - if ( $portletData['id'] === 'p-lang' && $this->isLanguagesInContent() ) { - $portletData = array_merge( $portletData, $this->getULSPortletData() ); - } - $class = $portletData['class']; - $portletData['class'] = trim( "$class $extraClasses[$type]" ); + $portletData['class'] = trim( $portletData['class'] ); + $portletData['heading-class'] = trim( $portletData['heading-class'] ); return $portletData; } @@ -873,10 +858,24 @@ abstract class SkinVector extends SkinMustache { break; } - $portletData = $this->decoratePortletClass( - $portletData, - $type - ); + if ( $key === 'data-languages' && $this->isLanguagesInContent() ) { + $portletData = array_merge( $portletData, $this->getULSPortletData() ); + } + + if ( $key === 'data-user-menu' && !$this->isLegacy() ) { + $portletData = $this->getUserMenuPortletData( $portletData ); + } + + if ( $key === 'data-vector-user-menu-overflow' ) { + $portletData['class'] .= ' vector-user-menu-overflow'; + } + + if ( $key === 'data-personal' && $this->isLegacy() ) { + // Set tooltip to empty string for the personal menu for both logged-in and logged-out users + // to avoid showing the tooltip for legacy version. + $portletData['html-tooltip'] = ''; + $portletData['class'] .= ' vector-user-menu-legacy'; + } // Special casing for Variant to change label to selected. // Hopefully we can revisit and possibly remove this code when the language switcher is moved. @@ -891,19 +890,10 @@ abstract class SkinVector extends SkinMustache { $portletData['aria-label'] = $this->msg( 'vector-language-variant-switcher-label' ); } - // T287494 We use tooltip messages to provide title attributes on hover over certain menu icons. For modern - // Vector, the "tooltip-p-personal" key is set to "User menu" which is appropriate for the user icon (dropdown - // indicator for user links menu) for logged-in users. This overrides the tooltip for the user links menu icon - // which is an ellipsis for anonymous users. - if ( $key === 'data-user-menu' && !$this->isLegacy() && !$this->loggedin ) { - $portletData['html-tooltip'] = Linker::tooltip( 'vector-anon-user-menu-title' ); - } - - // Set tooltip to empty string for the personal menu for both logged-in and logged-out users to avoid showing - // the tooltip for legacy version. - if ( $key === 'data-personal' && $this->isLegacy() ) { - $portletData['html-tooltip'] = ''; - } + $portletData = $this->updatePortletClasses( + $portletData, + $type + ); return $portletData + [ 'is-dropdown' => $type === self::MENU_TYPE_DROPDOWN, diff --git a/tests/phpunit/integration/SkinVectorTest.php b/tests/phpunit/integration/SkinVectorTest.php index bf1a84aa..63590c22 100644 --- a/tests/phpunit/integration/SkinVectorTest.php +++ b/tests/phpunit/integration/SkinVectorTest.php @@ -271,11 +271,11 @@ class SkinVectorTest extends MediaWikiIntegrationTestCase { $namespaces['class'] ); $this->assertSame( - 'mw-portlet mw-portlet-variants vector-menu-dropdown-noicon vector-menu-dropdown', + 'mw-portlet mw-portlet-variants vector-menu-dropdown', $variants['class'] ); $this->assertSame( - 'mw-portlet mw-portlet-cactions vector-menu-dropdown-noicon vector-menu-dropdown', + 'mw-portlet mw-portlet-cactions vector-menu-dropdown', $actions['class'] ); $this->assertSame( diff --git a/tests/phpunit/integration/VectorHooksTest.php b/tests/phpunit/integration/VectorHooksTest.php index dde04c32..3bb116d6 100644 --- a/tests/phpunit/integration/VectorHooksTest.php +++ b/tests/phpunit/integration/VectorHooksTest.php @@ -400,12 +400,12 @@ class VectorHooksTest extends MediaWikiIntegrationTestCase { $skin->getContext()->setTitle( Title::newFromText( 'Foo' ) ); $contentNavWatch = [ 'actions' => [ - 'watch' => [ 'class' => 'watch' ], + 'watch' => [ 'class' => [ 'watch' ] ], ] ]; $contentNavUnWatch = [ 'actions' => [ - 'move' => [ 'class' => 'move' ], + 'move' => [ 'class' => [ 'move' ] ], 'unwatch' => [], ], ]; @@ -413,16 +413,16 @@ class VectorHooksTest extends MediaWikiIntegrationTestCase { Hooks::onSkinTemplateNavigation( $skin, $contentNavUnWatch ); Hooks::onSkinTemplateNavigation( $skin, $contentNavWatch ); - $this->assertTrue( - in_array( 'icon', $contentNavWatch['views']['watch']['class'] ) !== false, + $this->assertContains( + 'icon', $contentNavWatch['views']['watch']['class'], 'Watch list items require an "icon" class' ); - $this->assertTrue( - in_array( 'icon', $contentNavUnWatch['views']['unwatch']['class'] ) !== false, + $this->assertContains( + 'icon', $contentNavUnWatch['views']['unwatch']['class'], 'Unwatch list items require an "icon" class' ); - $this->assertFalse( - strpos( $contentNavUnWatch['actions']['move']['class'], 'icon' ) !== false, + $this->assertNotContains( + 'icon', $contentNavUnWatch['actions']['move']['class'], 'List item other than watch or unwatch should not have an "icon" class' ); } @@ -585,18 +585,79 @@ class VectorHooksTest extends MediaWikiIntegrationTestCase { ); } - public function provideUpdateMenuItem() { + public function provideAppendClassToItem() { + return [ + // Add array class to array + [ + [], + [ 'array1', 'array2' ], + [ 'array1', 'array2' ], + ], + // Add string class to array + [ + [], + 'array1 array2', + [ 'array1 array2' ], + ], + // Add array class to string + [ + '', + [ 'array1', 'array2' ], + 'array1 array2', + ], + // Add string class to string + [ + '', + 'array1 array2', + 'array1 array2', + ], + // Add string class to undefined + [ + null, + 'array1 array2', + 'array1 array2', + ], + // Add array class to undefined + [ + null, + [ 'array1', 'array2' ], + [ 'array1', 'array2' ], + ], + ]; + } + + /** + * @covers ::appendClassToItem + * @dataProvider provideAppendClassToItem + */ + public function testAppendClassToItem( + $item, + $classes, + $expected + ) { + $appendClassToItem = new ReflectionMethod( + Hooks::class, + 'appendClassToItem' + ); + $appendClassToItem->setAccessible( true ); + $appendClassToItem->invokeArgs( null, [ &$item, $classes ] ); + $this->assertEquals( $expected, $item ); + } + + public function provideUpdateItemData() { return [ // Removes extra attributes [ [ 'class' => [], 'icon' => '', 'button' => false, 'text-hidden' => false, 'collapsible' => false ], - false, + 'link-class', + 'link-html', [ 'class' => [] ], ], - // Adds link-html + // Adds icon html [ [ 'class' => [], 'icon' => 'userpage' ], - false, + 'link-class', + 'link-html', [ 'class' => [], 'link-html' => @@ -606,19 +667,22 @@ class VectorHooksTest extends MediaWikiIntegrationTestCase { // Adds collapsible class [ [ 'class' => [], 'collapsible' => true ], - false, + 'link-class', + 'link-html', [ 'class' => [ 'user-links-collapsible-item' ] ], ], // Adds button classes [ [ 'class' => [], 'button' => true ], - false, + 'link-class', + 'link-html', [ 'class' => [], 'link-class' => [ 'mw-ui-button', 'mw-ui-quiet' ] ], ], // Adds text hidden classes [ [ 'class' => [], 'text-hidden' => true, 'icon' => 'userpage' ], - false, + 'link-class', + 'link-html', [ 'class' => [], 'link-class' => [ 'mw-ui-icon', 'mw-ui-icon-element', @@ -626,37 +690,37 @@ class VectorHooksTest extends MediaWikiIntegrationTestCase { 'mw-ui-icon-wikimedia-userpage' ] ], ], - // Adds classes for link data + // Adds button and icon classes [ - [ 'class' => [], 'button' => true, 'text-hidden' => true, 'icon' => 'userpage' ], - true, + [ 'class' => [], 'button' => true, 'icon' => 'userpage' ], + 'class', + 'link-html', [ 'class' => [ 'mw-ui-button', - 'mw-ui-quiet', - 'mw-ui-icon', - 'mw-ui-icon-element', - 'mw-ui-icon-userpage', - 'mw-ui-icon-wikimedia-userpage' - ] ], + 'mw-ui-quiet' + ], 'link-html' => + '' + ], ] ]; } /** - * @covers ::updateMenuItem - * @dataProvider provideUpdateMenuItem + * @covers ::updateItemData + * @dataProvider provideUpdateItemData */ - public function testUpdateMenuItem( - array $menuItem, - bool $isLinkData, + public function testUpdateItemData( + array $item, + string $buttonClassProp, + string $iconHtmlProp, array $expected ) { - $updateMenuItem = new ReflectionMethod( + $updateItemData = new ReflectionMethod( Hooks::class, - 'updateMenuItem' + 'updateItemData' ); - $updateMenuItem->setAccessible( true ); - $data = $updateMenuItem->invokeArgs( null, [ $menuItem, $isLinkData ] ); + $updateItemData->setAccessible( true ); + $data = $updateItemData->invokeArgs( null, [ $item, $buttonClassProp, $iconHtmlProp ] ); $this->assertEquals( $expected, $data ); } }