From 4f3981580db3e0b1a1fe0b4c267d14914f29099e Mon Sep 17 00:00:00 2001 From: Ostrzyciel Date: Mon, 1 Feb 2021 10:54:49 +0100 Subject: [PATCH] Move the warning outside sitenotice, tidy up code There's a lot of things going on here, so a quick summary: * Moved the warning outside sitenotice to avoid reflows and all kinds of other issues, including those with dismissable notices. * Placed the warning on the bottom of the page, on all skins, for consistency and to avoid obstructing vital UI elements such as site name, user tools, search box. * Changed the UI to use OOUI for, again, consistency and to avoid reinventing the wheel with button styling. * Removed the cookie emoji as it was only taking up valuable space on mobile and making designing a sensible layout much harder. * Merged the mobile and desktop RL modules into a single, responsive one. The warning will work correctly for any skin and screen width. * The integration tests are truly horrible, I tried to at least make them work. They deserve a proper rewrite, but that should be done after the extension switches to the new hook system. I will post screenshots of this in action in the first of the linked tasks. Bug: T271047 Bug: T173335 Change-Id: I6e2a3d7aeccc0d4df1b3238e52c67e72099d27d8 (cherry picked from commit edd0a7d9498320e5950e1b37f3cf4835a7e809c2) --- .stylelintrc.json | 5 +- extension.json | 20 ++-- includes/Hooks.php | 84 ++++++--------- .../ext.CookieWarning/ext.CookieWarning.js | 2 +- .../ext.CookieWarning/ext.CookieWarning.less | 57 ++++------ .../ext.CookieWarning.mobile.less | 65 ------------ .../ext.CookieWarning.minerva.styles.less | 12 --- tests/phpunit/includes/HooksTest.php | 100 +++++++++++------- 8 files changed, 127 insertions(+), 218 deletions(-) delete mode 100644 resources/ext.CookieWarning/ext.CookieWarning.mobile.less delete mode 100644 skinStyles/ext.CookieWarning.minerva.styles.less diff --git a/.stylelintrc.json b/.stylelintrc.json index 5c5449e..2c90730 100644 --- a/.stylelintrc.json +++ b/.stylelintrc.json @@ -1,6 +1,3 @@ { - "extends": "stylelint-config-wikimedia", - "rules": { - "rule-empty-line-before": null - } + "extends": "stylelint-config-wikimedia" } diff --git a/extension.json b/extension.json index c1ad7fe..081a6d1 100644 --- a/extension.json +++ b/extension.json @@ -1,10 +1,11 @@ { "name": "CookieWarning", - "version": "0.2.0", + "version": "0.3.0", "author": [ "Florian Schmidt", "Liz Lee", - "Jack Phoenix" + "Jack Phoenix", + "Ostrzyciel" ], "url": "https://www.mediawiki.org/wiki/Extension:CookieWarning", "descriptionmsg": "cookiewarning-desc", @@ -19,10 +20,10 @@ ] }, "Hooks": { - "SiteNoticeAfter": "CookieWarning\\Hooks::onSiteNoticeAfter", - "BeforePageDisplay": "CookieWarning\\Hooks::onBeforePageDisplay", + "SkinAfterContent": "CookieWarning\\Hooks::onSkinAfterContent", "GetPreferences": "CookieWarning\\Hooks::onGetPreferences", "BeforeInitialize": "CookieWarning\\Hooks::onBeforeInitialize", + "BeforePageDisplay": "CookieWarning\\Hooks::onBeforePageDisplay", "ResourceLoaderGetConfigVars": "CookieWarning\\Hooks::onResourceLoaderGetConfigVars" }, "config": { @@ -94,15 +95,10 @@ ] }, "ext.CookieWarning.styles": { - "styles": "resources/ext.CookieWarning/ext.CookieWarning.less" - }, - "ext.CookieWarning.mobile.styles": { - "styles": "resources/ext.CookieWarning/ext.CookieWarning.mobile.less", - "skinStyles": { - "minerva": "skinStyles/ext.CookieWarning.minerva.styles.less" - }, + "styles": "resources/ext.CookieWarning/ext.CookieWarning.less", "targets": [ - "mobile" + "mobile", + "desktop" ] }, "ext.CookieWarning.geolocation": { diff --git a/includes/Hooks.php b/includes/Hooks.php index 358b88a..762f83b 100644 --- a/includes/Hooks.php +++ b/includes/Hooks.php @@ -4,12 +4,13 @@ namespace CookieWarning; use Config; use ConfigException; -use ExtensionRegistry; use Html; use MediaWiki; use MediaWiki\MediaWikiServices; -use MobileContext; use MWException; +use OOUI\ButtonInputWidget; +use OOUI\ButtonWidget; +use OOUI\HorizontalLayout; use OutputPage; use Skin; use Title; @@ -38,7 +39,7 @@ class Hooks { return; } - if ( $user->isLoggedIn() ) { + if ( $user->isRegistered() ) { $user->setOption( 'cookiewarning_dismissed', 1 ); $user->saveSettings(); } else { @@ -48,20 +49,16 @@ class Hooks { } /** - * SiteNoticeAfter hook handler. + * SkinAfterContent hook handler. * - * Adds the CookieWarning information bar to the output html for mobile. + * Adds the CookieWarning information bar to the output html. * - * @param string &$siteNotice + * @param string &$data * @param Skin $skin - * @throws ConfigException + * * @throws MWException - * @return bool|void True or no return value to continue or false to abort */ - public static function onSiteNoticeAfter( - string &$siteNotice, - Skin $skin - ) { + public static function onSkinAfterContent( string &$data, Skin $skin ) { /** @var Decisions $cookieWarningDecisions */ $cookieWarningDecisions = MediaWikiServices::getInstance() ->getService( 'CookieWarning.Decisions' ); @@ -70,64 +67,53 @@ class Hooks { return; } - $isMobile = ExtensionRegistry::getInstance()->isLoaded( 'MobileFrontend' ) && - MobileContext::singleton()->shouldDisplayMobileView(); - - $siteNotice .= self::generateElements( $skin, $isMobile ); + $data .= self::generateElements( $skin ); } /** * Generates the elements for the banner. * * @param Skin $skin - * @param bool $isMobile This will return true if using mobile site. * @return string|null The html for cookie notice. */ - private static function generateElements( Skin $skin, bool $isMobile ) { + private static function generateElements( Skin $skin ) : ?string { $moreLink = self::getMoreLink(); + $buttons = []; if ( $moreLink ) { - $moreLink = "\u{00A0}" . Html::element( - 'a', - [ 'href' => $moreLink ], - $skin->msg( 'cookiewarning-moreinfo-label' )->text() - ); + $buttons[] = new ButtonWidget( [ + 'href' => $moreLink, + 'label' => $skin->msg( 'cookiewarning-moreinfo-label' )->text(), + 'flags' => [ 'progressive' ] + ] ); } + $buttons[] = new ButtonInputWidget( [ + 'type' => 'submit', + 'label' => $skin->msg( 'cookiewarning-ok-label' )->text(), + 'name' => 'disablecookiewarning', + 'value' => 'OK', + 'flags' => [ 'primary', 'progressive' ] + ] ); - $form = Html::openElement( 'form', [ 'method' => 'POST' ] ) . - Html::submitButton( - $skin->msg( 'cookiewarning-ok-label' )->text(), - [ - 'name' => 'disablecookiewarning', - 'class' => 'mw-cookiewarning-dismiss' - ] - ) . - Html::closeElement( 'form' ); - - $cookieImage = Html::element( - 'div', - [ 'class' => 'mw-cookiewarning-cimage' ], - "\u{1F36A}" + $form = Html::rawElement( + 'form', + [ 'method' => 'POST' ], + new HorizontalLayout( [ 'items' => $buttons ] ) ); return Html::openElement( 'div', - // banner-container marks this as a banner for Minerva - // Note to avoid this class, in future we may want to make use of SiteNotice - // or banner display - [ 'class' => 'mw-cookiewarning-container banner-container' ] + [ 'class' => 'mw-cookiewarning-container' ] ) . Html::openElement( 'div', [ 'class' => 'mw-cookiewarning-text' ] ) . - ( $isMobile ? $cookieImage : '' ) . Html::element( 'span', [], $skin->msg( 'cookiewarning-info' )->text() ) . - $moreLink . Html::closeElement( 'div' ) . $form . Html::closeElement( 'div' ); @@ -143,7 +129,7 @@ class Hooks { * @return string|null The url or null if none set * @throws ConfigException */ - private static function getMoreLink() { + private static function getMoreLink() : ?string { $conf = self::getConfig(); if ( $conf->get( 'CookieWarningMoreUrl' ) ) { return $conf->get( 'CookieWarningMoreUrl' ); @@ -180,15 +166,8 @@ class Hooks { return; } - if ( - ExtensionRegistry::getInstance()->isLoaded( 'MobileFrontend' ) && - MobileContext::singleton()->shouldDisplayMobileView() - ) { - $moduleStyles = [ 'ext.CookieWarning.mobile.styles' ]; - } else { - $moduleStyles = [ 'ext.CookieWarning.styles' ]; - } $modules = [ 'ext.CookieWarning' ]; + $moduleStyles = [ 'ext.CookieWarning.styles' ]; if ( $cookieWarningDecisions->shouldAddResourceLoaderComponents() ) { $modules[] = 'ext.CookieWarning.geolocation'; @@ -196,6 +175,7 @@ class Hooks { } $out->addModules( $modules ); $out->addModuleStyles( $moduleStyles ); + $out->enableOOUI(); } /** diff --git a/resources/ext.CookieWarning/ext.CookieWarning.js b/resources/ext.CookieWarning/ext.CookieWarning.js index 1052e1d..3e90df3 100644 --- a/resources/ext.CookieWarning/ext.CookieWarning.js +++ b/resources/ext.CookieWarning/ext.CookieWarning.js @@ -13,7 +13,7 @@ $( '.mw-cookiewarning-container' ).detach(); } else { // Click handler for the "Ok" element in the cookiewarning information bar - $( '.mw-cookiewarning-dismiss' ).on( 'click', function ( ev ) { + $( '.mw-cookiewarning-container button' ).on( 'click', function ( ev ) { // an anonymous user doesn't have preferences, so don't try to save this in // the user preferences. if ( !mw.user.isAnon() ) { diff --git a/resources/ext.CookieWarning/ext.CookieWarning.less b/resources/ext.CookieWarning/ext.CookieWarning.less index 01fccdf..462871f 100644 --- a/resources/ext.CookieWarning/ext.CookieWarning.less +++ b/resources/ext.CookieWarning/ext.CookieWarning.less @@ -1,49 +1,38 @@ -@z-index: 1999; - -/* stylelint-disable selector-max-id */ -#siteNotice { - z-index: @z-index; -} -/* stylelint-enable selector-max-id */ - .mw-cookiewarning-container { position: fixed; background-color: rgba( 90, 90, 90, 0.85 ); box-sizing: border-box; - color: #fff; - font-weight: bold; - font-size: 13px; padding: 7px 15px; - top: 0; + bottom: 0; left: 0; width: 100%; - z-index: @z-index; + z-index: 1999; + display: flex; + justify-content: center; - a, - .mw-cookiewarning-dismiss { - background-color: #3c3c3c; - height: 100%; - padding: 3px 10px; - border-radius: 2px; - border: 0; - cursor: pointer; - text-decoration: none !important; /* stylelint-disable-line declaration-no-important */ - color: #fff; - margin-right: 0.5em; - white-space: nowrap; - font-weight: bold; - font-size: 13px; - } + // Narrow mobile screens + // 550px is a mostly arbitrary number, though it's used by a few other exts and skins + @media screen and ( max-width: 550px ) { + flex-wrap: wrap; - .mw-cookiewarning-text { - display: inline-block; - - span { - margin-right: 0.5em; + .mw-cookiewarning-text { + margin-bottom: 0.7em; } } + .mw-cookiewarning-text { + display: flex; + align-items: center; + color: #fff; + font-weight: bold; + font-size: 92%; + } + form { - display: inline-block; + flex-shrink: 0; + + .oo-ui-widget { + margin: 0 0 0 8px; + } } } diff --git a/resources/ext.CookieWarning/ext.CookieWarning.mobile.less b/resources/ext.CookieWarning/ext.CookieWarning.mobile.less deleted file mode 100644 index 585fb0a..0000000 --- a/resources/ext.CookieWarning/ext.CookieWarning.mobile.less +++ /dev/null @@ -1,65 +0,0 @@ -@import 'mediawiki.ui/variables.less'; - -@cookieWarningFontSize: 12px; -@contentPaddingTablet: @iconGutterWidth + @iconGutterWidth + @iconSize; - -.mw-cookiewarning-container { - background-color: #fff; - padding: 16px; - font-size: @cookieWarningFontSize; - font-weight: 500; - font-style: normal; - font-stretch: normal; - display: flex; - align-items: center; - justify-content: space-between; - - .mw-cookiewarning-text { - display: flex; - align-items: center; - - .mw-cookiewarning-cimage { - width: 28px; - height: 100%; - font-size: 28px; - margin-right: 15px; - float: left; - text-align: center; - line-height: 100%; - } - } - - form { - margin-left: 5px; - /* borrowed from mediawiki/core Login Button, following `ButtonWidget (progressive)‎` from OOjs UI */ - .mw-cookiewarning-dismiss { - height: 32px; - background-color: #f8f9fa; - color: #36c; - } - .mw-cookiewarning-dismiss:hover { - background-color: #fff; - border-color: #859ecc; - box-shadow: none; - } - .mw-cookiewarning-dismiss:active { - background-color: #eff3fa; - color: #2a4b8d; - border-color: #2a4b8d; - } - .mw-cookiewarning-dismiss:focus { - border-color: #36c; - box-shadow: inset 0 0 0 1px #36c; - } - } -} - -@media all and ( min-width: @width-breakpoint-tablet ) { - .mw-cookiewarning-container { - line-height: @contentPaddingTablet; - - .mw-cookiewarning-cimage { - line-height: inherit; - } - } -} diff --git a/skinStyles/ext.CookieWarning.minerva.styles.less b/skinStyles/ext.CookieWarning.minerva.styles.less deleted file mode 100644 index 9c01495..0000000 --- a/skinStyles/ext.CookieWarning.minerva.styles.less +++ /dev/null @@ -1,12 +0,0 @@ -@import 'mediawiki.ui/variables.less'; - -.mw-cookiewarning-container { - border-bottom: 1px solid #c8ccd1; - padding: 2.5em; -} - -@media all and ( min-width: @width-breakpoint-tablet ) { - .mw-cookiewarning-container { - line-height: normal; - } -} diff --git a/tests/phpunit/includes/HooksTest.php b/tests/phpunit/includes/HooksTest.php index b1a0603..a4b3662 100644 --- a/tests/phpunit/includes/HooksTest.php +++ b/tests/phpunit/includes/HooksTest.php @@ -2,19 +2,14 @@ namespace CookieWarning\Tests; -use CommentStoreComment; use CookieWarning\GeoLocation; use CookieWarning\Hooks; use DerivativeContext; use FauxRequest; use MediaWiki\MediaWikiServices; -use MediaWiki\Revision\SlotRecord; use MediaWikiLangTestCase; use RequestContext; use SkinTemplate; -use Title; -use WikiPage; -use WikitextContent; /** * @covers Hooks @@ -24,49 +19,78 @@ class HooksTest extends MediaWikiLangTestCase { /** * @dataProvider providerOnSiteNoticeAfter + * + * @param bool $enabled + * @param false|string $morelinkConfig + * @param false|string $morelinkCookieWarningMsg + * @param false|string $morelinkCookiePolicyMsg + * @param false|string $expectedLink + * * @throws \MWException - * @throws \ConfigException */ - public function testOnSiteNoticeAfter( $enabled, $morelinkConfig, - $morelinkCookieWarningMsg, $morelinkCookiePolicyMsg, $expectedLink - ) { + public function testOnSiteNoticeAfter( + bool $enabled, + $morelinkConfig, + $morelinkCookieWarningMsg, + $morelinkCookiePolicyMsg, + $expectedLink + ) : void { $this->setMwGlobals( [ 'wgCookieWarningEnabled' => $enabled, 'wgCookieWarningMoreUrl' => $morelinkConfig, 'wgCookieWarningForCountryCodes' => false, - 'wgUseMediaWikiUIEverywhere' => true, ] ); MediaWikiServices::getInstance()->getMessageCache()->enable(); + if ( $morelinkCookieWarningMsg ) { - $title = Title::newFromText( 'cookiewarning-more-link', NS_MEDIAWIKI ); - $wikiPage = WikiPage::factory( $title ); - $pageUpdater = $wikiPage->newPageUpdater( \User::newFromName( 'UTSysop' ) ); - $pageUpdater->setContent( SlotRecord::MAIN, new WikitextContent( $morelinkCookieWarningMsg ) ); - $pageUpdater->saveRevision( CommentStoreComment::newUnsavedComment( 'CookieWarning test' ) ); + $this->editPage( + 'cookiewarning-more-link', + strval( $morelinkCookieWarningMsg ), + '', + NS_MEDIAWIKI, + $this->getTestSysop()->getUser() + ); } if ( $morelinkCookiePolicyMsg ) { - $title = Title::newFromText( 'cookie-policy-link', NS_MEDIAWIKI ); - $wikiPage = WikiPage::factory( $title ); - $pageUpdater = $wikiPage->newPageUpdater( \User::newFromName( 'UTSysop' ) ); - $pageUpdater->setContent( SlotRecord::MAIN, new WikitextContent( $morelinkCookiePolicyMsg ) ); - $pageUpdater->saveRevision( CommentStoreComment::newUnsavedComment( 'CookieWarning test' ) ); + $this->editPage( + 'cookie-policy-link', + strval( $morelinkCookiePolicyMsg ), + '', + NS_MEDIAWIKI, + $this->getTestSysop()->getUser() + ); } + $sk = new SkinTemplate(); + // setup OOUI, that would be normally done in BeforePageDisplay hook + $sk->getOutput()->enableOOUI(); + $data = ''; - Hooks::onSiteNoticeAfter( $data, $sk ); - if ( $expectedLink === false ) { - $expected = ''; + Hooks::onSkinAfterContent( $data, $sk ); + + if ( $enabled ) { + $this->assertNotEmpty( $data, 'Cookie warning should be present' ); } else { - // @codingStandardsIgnoreStart Generic.Files.LineLength - $expected = - str_replace( '$1', $expectedLink, - '' ); - // @codingStandardsIgnoreEnd + $this->assertEmpty( $data, 'Cookie warning should not be present' ); + return; + } + + if ( $expectedLink === false ) { + $this->assertNotRegExp( + '/]+href=[\'"]([^\'"]+)[\'"].+?>/', + $data, + 'More information link should not be present' + ); + } else { + $this->assertRegExp( + '/]+href=[\'"]' . preg_quote( $expectedLink, '/' ) . '[\'"].+?>/', + $data, + 'More information link should be present and match the expectation' + ); } - $this->assertEquals( $expected, $data ); } - public function providerOnSiteNoticeAfter() { + public function providerOnSiteNoticeAfter() : array { return [ [ // $wgCookieWarningEnabled @@ -78,7 +102,7 @@ class HooksTest extends MediaWikiLangTestCase { // MediaWiki:Cookie-policy-link false, // expected cookie warning link (when string), nothing if false - '', + false, ], [ false, @@ -92,21 +116,21 @@ class HooksTest extends MediaWikiLangTestCase { 'http://google.de', false, false, - "\u{00A0}More information", + 'http://google.de', ], [ true, '', 'http://google.de', false, - "\u{00A0}More information", + 'http://google.de', ], [ true, '', false, 'http://google.de', - "\u{00A0}More information", + 'http://google.de', ], // the config should be the used, if set (no matter if the messages are used or not) [ @@ -114,21 +138,21 @@ class HooksTest extends MediaWikiLangTestCase { 'http://google.de', false, 'http://google123.de', - "\u{00A0}More information", + 'http://google.de', ], [ true, 'http://google.de', 'http://google1234.de', 'http://google123.de', - "\u{00A0}More information", + 'http://google.de', ], [ true, '', 'http://google.de', 'http://google123.de', - "\u{00A0}More information", + 'http://google.de', ], ]; } @@ -155,7 +179,7 @@ class HooksTest extends MediaWikiLangTestCase { $sk = new SkinTemplate(); $sk->setContext( $context ); $data = ''; - Hooks::onSiteNoticeAfter( $data, $sk ); + Hooks::onSkinAfterContent( $data, $sk ); $this->assertEquals( $expected,