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 edd0a7d949)
2024-11-23 13:56:53 +00:00 · 2021-02-01 10:54:49 +01:00 · 2021-02-01 10:54:49 +01:00 · 4f3981580d
parent 542f7e82ef
commit 4f3981580d
8 changed files with 127 additions and 218 deletions
--- a/.stylelintrc.json
+++ b/.stylelintrc.json
@ -1,6 +1,3 @@
 {
-	"extends": "stylelint-config-wikimedia",
-	"rules": {
-		"rule-empty-line-before": null
-	}
+	"extends": "stylelint-config-wikimedia"
 }
--- 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": {
--- 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();
 	}

 	/**
--- 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() ) {
--- 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;
+		}
 	}
 }
--- a/resources/ext.CookieWarning/ext.CookieWarning.mobile.less
+++ b/resources/ext.CookieWarning/ext.CookieWarning.mobile.less
@ -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;
-		}
-	}
-}
--- a/skinStyles/ext.CookieWarning.minerva.styles.less
+++ b/skinStyles/ext.CookieWarning.minerva.styles.less
@ -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;
-	}
-}
--- 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,
-					'<div class="mw-cookiewarning-container banner-container"><div class="mw-cookiewarning-text"><span>Cookies help us deliver our services. By using our services, you agree to our use of cookies.</span>$1</div><form method="POST"><input name="disablecookiewarning" class="mw-cookiewarning-dismiss mw-ui-button" type="submit" value="OK"/></form></div>' );
-			// @codingStandardsIgnoreEnd
+			$this->assertEmpty( $data, 'Cookie warning should not be present' );
+			return;
+		}
+
+		if ( $expectedLink === false ) {
+			$this->assertNotRegExp(
+				'/<a[^>]+href=[\'"]([^\'"]+)[\'"].+?>/',
+				$data,
+				'More information link should not be present'
+			);
+		} else {
+			$this->assertRegExp(
+				'/<a[^>]+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}<a href=\"http://google.de\">More information</a>",
+				'http://google.de',
 			],
 			[
 				true,
 				'',
 				'http://google.de',
 				false,
-				"\u{00A0}<a href=\"http://google.de\">More information</a>",
+				'http://google.de',
 			],
 			[
 				true,
 				'',
 				false,
 				'http://google.de',
-				"\u{00A0}<a href=\"http://google.de\">More information</a>",
+				'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}<a href=\"http://google.de\">More information</a>",
+				'http://google.de',
 			],
 			[
 				true,
 				'http://google.de',
 				'http://google1234.de',
 				'http://google123.de',
-				"\u{00A0}<a href=\"http://google.de\">More information</a>",
+				'http://google.de',
 			],
 			[
 				true,
 				'',
 				'http://google.de',
 				'http://google123.de',
-				"\u{00A0}<a href=\"http://google.de\">More information</a>",
+				'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,