From 6fbf08a1986a9dcd78bf4fe2c08df14983d6189f Mon Sep 17 00:00:00 2001 From: Nicholas Ray Date: Thu, 17 Mar 2022 17:02:39 -0600 Subject: [PATCH] Build A/B test bucketing infrastructure for the table of contents. * Bucket and sample on server by using the `WikimediaEvents.WebABTestArticleIdFactory` service from WikimediaEvents (soft dependency) * Add linkHijack.js so that users bucketed in one group have the possibility of remaining in that group if they click a link to another page. Bug: T302046 Depends-On: Ie6627de98effb3d37a3bedda5023d08af319837f Change-Id: Iff231a976c473217b0fa4da1aa9a8d1c2a1a19f2 --- includes/Constants.php | 10 +- includes/Hooks.php | 62 ++++++- includes/SkinVector.php | 5 - includes/SkinVector22.php | 54 +++++- resources/skins.vector.es6/linkHijack.js | 71 ++++++++ resources/skins.vector.es6/main.js | 27 ++- .../skins.vector.styles/layouts/screen.less | 6 + skin.json | 14 +- tests/jest/linkHijack.test.js | 158 ++++++++++++++++++ 9 files changed, 381 insertions(+), 26 deletions(-) create mode 100644 resources/skins.vector.es6/linkHijack.js create mode 100644 tests/jest/linkHijack.test.js diff --git a/includes/Constants.php b/includes/Constants.php index 60a4f54a..ce3453a1 100644 --- a/includes/Constants.php +++ b/includes/Constants.php @@ -173,12 +173,11 @@ final class Constants { public const FEATURE_STICKY_HEADER_EDIT = 'StickyHeaderEdit'; /** - * Defines whether the Sticky Header A/B test is running. See - * https://phabricator.wikimedia.org/T292587 for additional detail about the test. + * Defines whether an A/B test is running. * * @var string */ - public const CONFIG_STICKY_HEADER_TREATMENT_AB_TEST_ENROLLMENT = 'VectorWebABTestEnrollment'; + public const CONFIG_WEB_AB_TEST_ENROLLMENT = 'VectorWebABTestEnrollment'; /** * The `mediawiki.searchSuggest` protocol piece of the SearchSatisfaction instrumention reads @@ -270,6 +269,11 @@ final class Constants { */ public const FEATURE_TABLE_OF_CONTENTS = 'TableOfContents'; + /** + * @var string + */ + public const WEB_AB_TEST_ARTICLE_ID_FACTORY_SERVICE = 'WikimediaEvents.WebABTestArticleIdFactory'; + /** * This class is for namespacing constants only. Forbid construction. * @throws FatalError diff --git a/includes/Hooks.php b/includes/Hooks.php index 097f3ff5..7a388577 100644 --- a/includes/Hooks.php +++ b/includes/Hooks.php @@ -40,7 +40,7 @@ class Hooks { */ private static function getActiveABTest( $config ) { $ab = $config->get( - Constants::CONFIG_STICKY_HEADER_TREATMENT_AB_TEST_ENROLLMENT + Constants::CONFIG_WEB_AB_TEST_ENROLLMENT ); if ( count( $ab ) === 0 ) { // If array is empty then no experiment and need to validate. @@ -460,6 +460,59 @@ class Hooks { } } + /** + * Returns the necessary TOC classes. + * + * @param Skin $sk + * @param Config $config + * @return string[] + */ + private static function getTocClasses( Skin $sk, $config ): array { + if ( !( $sk instanceof SkinVector22 ) ) { + return []; + } + + $classes = []; + + $isTocABTestEnabled = $sk->isTOCABTestEnabled(); + if ( $isTocABTestEnabled || $sk->isTOCEnabled() ) { + $classes[] = 'vector-toc-enabled'; + } + + if ( + $sk->isTableOfContentsVisibleInSidebar() && + $isTocABTestEnabled + ) { + /** @var \WikimediaEvents\WebABTest\WebABTestArticleIdFactory */ + $webABTestArticleIdFactory = MediaWikiServices::getInstance()->getService( + Constants::WEB_AB_TEST_ARTICLE_ID_FACTORY_SERVICE + ); + $experimentConfig = $config->get( Constants::CONFIG_WEB_AB_TEST_ENROLLMENT ); + $bucketKeys = array_keys( $experimentConfig['buckets'] ); + + $ab = $webABTestArticleIdFactory->makeWebABTestArticleIdStrategy( + $webABTestArticleIdFactory->filterExcludedBucket( $bucketKeys ), + 1 - $experimentConfig['buckets']['unsampled']['samplingRate'], + Constants::QUERY_PARAM_TABLE_OF_CONTENTS, + $sk->getContext() + ); + + if ( !$ab ) { + return $classes; + } + + $bucket = $ab->getBucket(); + + if ( $bucket ) { + $experimentName = $experimentConfig[ 'name' ]; + $classes[] = $experimentName; + $classes[] = "$experimentName-$bucket"; + } + } + + return $classes; + } + /** * Called when OutputPage::headElement is creating the body tag to allow skins * and extensions to add attributes they might need to the body of the page. @@ -473,6 +526,7 @@ class Hooks { if ( !self::isVectorSkin( $skinName ) ) { return; } + $config = $sk->getConfig(); // As of 2020/08/13, this CSS class is referred to by the following deployed extensions: // @@ -486,7 +540,11 @@ class Hooks { $bodyAttrs['class'] .= ' skin-vector-legacy'; } - $config = $sk->getConfig(); + $tocClasses = self::getTocClasses( $sk, $config ); + if ( $tocClasses ) { + $bodyAttrs['class'] .= ' ' . implode( ' ', $tocClasses ); + } + // Should we disable the max-width styling? if ( !self::isSkinVersionLegacy( $skinName ) && $sk->getTitle() && self::shouldDisableMaxWidth( $config->get( 'VectorMaxWidthOptions' ), diff --git a/includes/SkinVector.php b/includes/SkinVector.php index 1ea8e37d..c0067feb 100644 --- a/includes/SkinVector.php +++ b/includes/SkinVector.php @@ -118,7 +118,6 @@ 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 STICKY_HEADER_ENABLED_CLASS = 'vector-sticky-header-enabled'; - private const TABLE_OF_CONTENTS_ENABLED_CLASS = 'vector-toc-enabled'; 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'; @@ -405,10 +404,6 @@ abstract class SkinVector extends SkinMustache { $original['class'] = implode( ' ', [ $original['class'] ?? '', self::STICKY_HEADER_ENABLED_CLASS ] ); } - if ( VectorServices::getFeatureManager()->isFeatureEnabled( Constants::FEATURE_TABLE_OF_CONTENTS ) ) { - $original['class'] = implode( ' ', [ $original['class'] ?? '', self::TABLE_OF_CONTENTS_ENABLED_CLASS ] ); - } - return $original; } diff --git a/includes/SkinVector22.php b/includes/SkinVector22.php index 032b0d6d..d6f985cd 100644 --- a/includes/SkinVector22.php +++ b/includes/SkinVector22.php @@ -2,12 +2,16 @@ namespace Vector; +use MediaWiki\MediaWikiServices; + /** * @ingroup Skins * @package Vector * @internal */ class SkinVector22 extends SkinVector { + private const TOC_AB_TEST_NAME = 'skin-vector-toc-experiment'; + /** * Updates the constructor to conditionally disable table of contents in article * body. Note, the constructor can only check feature flags that do not vary on @@ -15,22 +19,62 @@ class SkinVector22 extends SkinVector { * @inheritDoc */ public function __construct( array $options ) { - $options['toc'] = !$this->isTableOfContentsVisibleInSidebar(); + if ( !$this->isTOCABTestEnabled() ) { + $options['toc'] = !$this->isTableOfContentsVisibleInSidebar(); + } + parent::__construct( $options ); } + /** + * @internal + * @return bool + */ + public function isTOCABTestEnabled(): bool { + $experimentConfig = $this->getConfig()->get( Constants::CONFIG_WEB_AB_TEST_ENROLLMENT ); + + return $experimentConfig['name'] === self::TOC_AB_TEST_NAME && + $experimentConfig['enabled'] && + MediaWikiServices::getInstance()->hasService( Constants::WEB_AB_TEST_ARTICLE_ID_FACTORY_SERVICE ); + } + + /** + * Returns whether or not the table of contents is enabled through + * FeatureManager. + * + * @internal + * @return bool + */ + public function isTOCEnabled() { + $featureManager = VectorServices::getFeatureManager(); + + return $featureManager->isFeatureEnabled( Constants::FEATURE_TABLE_OF_CONTENTS ); + } + /** * Determines if the Table of Contents should be visible. * TOC is visible on main namespaces except for the Main Page * when the feature flag is on. * + * @internal * @return bool */ - private function isTableOfContentsVisibleInSidebar(): bool { - $featureManager = VectorServices::getFeatureManager(); + public function isTableOfContentsVisibleInSidebar(): bool { $title = $this->getTitle(); - $isMainPage = $title ? $title->isMainPage() : false; - return $featureManager->isFeatureEnabled( Constants::FEATURE_TABLE_OF_CONTENTS ) && !$isMainPage; + + if ( + !$title || + $title->getArticleID() === 0 || + $title->isMainPage() + ) { + return false; + } + + if ( $this->isTOCABTestEnabled() ) { + return true; + } + + return $this->isTOCEnabled(); } /** diff --git a/resources/skins.vector.es6/linkHijack.js b/resources/skins.vector.es6/linkHijack.js new file mode 100644 index 00000000..136ba3bc --- /dev/null +++ b/resources/skins.vector.es6/linkHijack.js @@ -0,0 +1,71 @@ +/** + * Appends a query param to the `href` attribute of any clicked anchor element + * or anchor element that is the parent of a clicked HTMLElement. Links that + * lead to different origins or have the same pathname and have a hash fragment + * are ignored. + * + * @param {string} key + * @param {string} value + * @return {Function} A cleanup function is returned that + * removes any event listeners that were added. + */ +function linkHijack( key, value ) { + /** + * @param {MouseEvent} e + */ + function handleClick( e ) { + if ( !e.target || !( e.target instanceof HTMLElement ) ) { + return; + } + + const anchor = e.target.closest( 'a' ); + if ( !anchor ) { + return; + } + + let locationUrl; + let oldUrl; + try { + // eslint-disable-next-line compat/compat + locationUrl = new URL( location.href ); + // eslint-disable-next-line compat/compat + oldUrl = new URL( anchor.href ); + + } catch ( error ) { + // A TypeError may be thrown for invalid URLs. In that case, return + // gracefully. + return; + } + + if ( + locationUrl.origin !== oldUrl.origin || + ( locationUrl.pathname === oldUrl.pathname && oldUrl.hash ) + ) { + // Return early if link leads to host outside the current one or if the + // url contains a pathname that is the same as the current one and also + // has a hash fragment (e.g. this occurs with links in the TOC and we + // don't want to trigger a refresh of the page by appending a query + // param). + return; + } + + // eslint-disable-next-line compat/compat + const params = new URLSearchParams( oldUrl.search ); + if ( !params.has( key ) ) { + params.append( key, value ); + } + + // eslint-disable-next-line compat/compat + const newUrl = new URL( `${oldUrl.origin}${oldUrl.pathname}?${params}${oldUrl.hash}` ); + anchor.setAttribute( 'href', newUrl.toString() ); + + } + + document.body.addEventListener( 'click', handleClick ); + + return () => { + document.body.removeEventListener( 'click', handleClick ); + }; +} + +module.exports = linkHijack; diff --git a/resources/skins.vector.es6/main.js b/resources/skins.vector.es6/main.js index ed33a098..7a16a5d5 100644 --- a/resources/skins.vector.es6/main.js +++ b/resources/skins.vector.es6/main.js @@ -7,6 +7,8 @@ const initSectionObserver = require( './sectionObserver.js' ), initTableOfContents = require( './tableOfContents.js' ), deferUntilFrame = require( './deferUntilFrame.js' ), + linkHijack = require( './linkHijack.js' ), + ABTestConfig = require( /** @type {string} */ ( './config.json' ) ).wgVectorWebABTestEnrollment || {}, TOC_ID = 'mw-panel-toc', TOC_ID_LEGACY = 'toc', BODY_CONTENT_ID = 'bodyContent', @@ -15,7 +17,8 @@ const TOC_LEGACY_PLACEHOLDER_TAG = 'mw:tocplace', TOC_SCROLL_HOOK = 'table_of_contents', PAGE_TITLE_SCROLL_HOOK = 'page_title', - ABTestConfig = require( /** @type {string} */ ( './config.json' ) ).wgVectorWebABTestEnrollment || {}; + TOC_QUERY_PARAM = 'tableofcontents', + TOC_EXPERIMENT_NAME = 'skin-vector-toc-experiment'; /** * @callback OnIntersection @@ -144,8 +147,26 @@ const main = () => { tocElement && bodyContent && window.IntersectionObserver && - window.requestAnimationFrame ) - ) { + window.requestAnimationFrame + ) ) { + return; + } + + const experiment = + !!ABTestConfig.enabled && + ABTestConfig.name === TOC_EXPERIMENT_NAME && + document.body.classList.contains( ABTestConfig.name ) && + // eslint-disable-next-line compat/compat + window.URLSearchParams && + initExperiment( ABTestConfig ); + const isInTreatmentBucket = !!experiment && experiment.isInTreatmentBucket(); + + if ( experiment && experiment.isInSample() ) { + linkHijack( TOC_QUERY_PARAM, isInTreatmentBucket ? '1' : '0' ); + } + + if ( experiment && !isInTreatmentBucket ) { + // Return early if the old TOC is shown. return; } diff --git a/resources/skins.vector.styles/layouts/screen.less b/resources/skins.vector.styles/layouts/screen.less index 0186da01..afd35bbe 100644 --- a/resources/skins.vector.styles/layouts/screen.less +++ b/resources/skins.vector.styles/layouts/screen.less @@ -267,6 +267,12 @@ body { .transform( translateX( -( @max-width-page-container - @max-width-workspace-container ) / 2 ) ); } +.skin-vector-toc-experiment-control .mw-table-of-contents-container, +.skin-vector-toc-experiment-unsampled .mw-table-of-contents-container, +.skin-vector-toc-experiment-treatment #toc { + display: none; +} + // Hide sidebar entirely when the checkbox is disabled and the TOC is enabled .vector-toc-enabled #mw-sidebar-checkbox:not( :checked ) ~ .mw-workspace-container .mw-sidebar { display: none; diff --git a/skin.json b/skin.json index eba78e3c..b6f492d4 100644 --- a/skin.json +++ b/skin.json @@ -285,6 +285,7 @@ "resources/skins.vector.es6/tableOfContents.js", "resources/skins.vector.es6/sectionObserver.js", "resources/skins.vector.es6/deferUntilFrame.js", + "resources/skins.vector.es6/linkHijack.js", { "name": "resources/skins.vector.es6/config.json", "callback": "Vector\\Hooks::getVectorResourceLoaderConfig" @@ -467,20 +468,17 @@ }, "VectorWebABTestEnrollment": { "value": { - "name": "vector.sticky_header", + "name": "skin-vector-toc-experiment", "enabled": false, "buckets": { "unsampled": { - "samplingRate": 0.1 + "samplingRate": 0.8 }, "control": { - "samplingRate": 0.3 + "samplingRate": 0.1 }, - "stickyHeaderDisabled": { - "samplingRate": 0.3 - }, - "stickyHeaderEnabledTreatment": { - "samplingRate": 0.3 + "treatment": { + "samplingRate": 0.1 } } }, diff --git a/tests/jest/linkHijack.test.js b/tests/jest/linkHijack.test.js new file mode 100644 index 00000000..3b0cc2e2 --- /dev/null +++ b/tests/jest/linkHijack.test.js @@ -0,0 +1,158 @@ +const linkHijack = require( '../../resources/skins.vector.es6/linkHijack.js' ); + +describe( 'linkHijack.js', () => { + let /** @type {jest.Mock} */ getHrefSpy; + let /** @type {Function | undefined} */ cleanup; + + beforeEach( () => { + cleanup = undefined; + getHrefSpy = jest.fn( () => 'http://localhost/wiki/Tree' ); + + // @ts-ignore + delete window.location; + // @ts-ignore + window.location = { + get href() { + return getHrefSpy(); + } + }; + + document.body.innerHTML = ` + Barack Obama + Barack Obama + Jump + Different Origin + Query Param + Query Param + Anchor without href + Anchor with invalid href +
+ + `; + } ); + + afterEach( () => { + if ( cleanup ) { + cleanup(); + } + } ); + + describe( 'when link origin is same and pathname is different', () => { + it( 'appends a query param to anchor element when user clicks anchor element', () => { + const anchor = /** @type {HTMLAnchorElement} */ ( document.querySelector( '.anchor' ) ); + cleanup = linkHijack( 'tableofcontents', '1' ); + anchor.click(); + expect( anchor.href ).toBe( 'http://localhost/wiki/Barack_Obama?tableofcontents=1' ); + } ); + + it( 'appends a query param to anchor element when user clicks inner span element', () => { + const anchorWithSpan = /** @type {HTMLAnchorElement} */ ( document.querySelector( '.anchor-with-span' ) ); + const innerSpan = /** @type {HTMLSpanElement} */ ( document.querySelector( '.inner-span' ) ); + cleanup = linkHijack( 'tableofcontents', '1' ); + innerSpan.click(); + expect( anchorWithSpan.href ).toBe( 'http://localhost/wiki/Barack_Obama?tableofcontents=1' ); + } ); + } ); + + describe( 'when link origin is different and pathname is different', () => { + it( 'does not append a query param to the anchor element', () => { + const anchorWithDifferentOrigin = /** @type {HTMLAnchorElement} */ ( document.querySelector( '.anchor-with-different-origin' ) ); + cleanup = linkHijack( 'tableofcontents', '1' ); + anchorWithDifferentOrigin.click(); + expect( anchorWithDifferentOrigin.href ).toBe( 'http://www.google.com/' ); + } ); + } ); + + describe( 'when link origin is same and pathname is same and hash fragment is present', () => { + it( 'does not append a query param to the anchor element', () => { + const anchorWithHash = /** @type {HTMLAnchorElement} */ ( document.querySelector( '.anchor-with-hash' ) ); + cleanup = linkHijack( 'tableofcontents', '1' ); + anchorWithHash.click(); + expect( anchorWithHash.href ).toBe( 'http://localhost/wiki/Tree#jump' ); + } ); + } ); + + describe( 'when link already has the same query param', () => { + it( 'does not duplicate query params', () => { + const anchorWithQueryParam = /** @type {HTMLAnchorElement} */ ( document.querySelector( '.anchor-with-query-param' ) ); + cleanup = linkHijack( 'tableofcontents', '1' ); + anchorWithQueryParam.click(); + expect( anchorWithQueryParam.href ).toBe( 'http://localhost/wiki/Barack_Obama?tableofcontents=1' ); + } ); + } ); + + describe( 'when link already has different query param', () => { + it( 'appends query param', () => { + const anchorWithDifferentQueryParam = /** @type {HTMLAnchorElement} */ ( document.querySelector( '.anchor-with-different-query-param' ) ); + cleanup = linkHijack( 'tableofcontents', '1' ); + anchorWithDifferentQueryParam.click(); + expect( anchorWithDifferentQueryParam.href ).toBe( 'http://localhost/wiki/Barack_Obama?foo=1&tableofcontents=1' ); + } ); + } ); + + describe( 'when clicking on an element that is not an anchor element or a child of an anchor', () => { + it( 'does nothing (no errors)', () => { + const div = /** @type {HTMLDivElement}} */ ( document.querySelector( 'div' ) ); + cleanup = linkHijack( 'tableofcontents', '1' ); + div.click(); + } ); + } ); + + describe( 'when clicking on an element that is not an HTMLElement', () => { + it( 'does nothing (no errors)', () => { + const svg = /** @type {SVGElement}} */ ( document.querySelector( 'svg' ) ); + cleanup = linkHijack( 'tableofcontents', '1' ); + svg.dispatchEvent( new Event( 'click', { bubbles: true } ) ); + } ); + } ); + + describe( 'when clicking on an Anchor without an `href', () => { + it( 'handles it gracefully (no errors)', () => { + const anchor = /** @type {HTMLAnchorElement}} */ ( document.querySelector( '.anchor-without-href' ) ); + cleanup = linkHijack( 'tableofcontents', '1' ); + anchor.click(); + } ); + } ); + + describe( 'when clicking on an Anchor with an invalid `href', () => { + it( 'handles it gracefully (no errors)', () => { + const anchor = /** @type {HTMLAnchorElement}} */ ( document.querySelector( '.anchor-with-invalid-href' ) ); + cleanup = linkHijack( 'tableofcontents', '1' ); + anchor.click(); + } ); + } ); + + describe( 'when cleanup function is called', () => { + let /** @type {any} */ events; + + beforeEach( () => { + events = {}; + + jest.spyOn( document.body, 'addEventListener' ).mockImplementation( ( event ) => { + if ( !( event in events ) ) { + events[ event ] = 1; + } else { + events[ event ] += 1; + } + } ); + + jest.spyOn( document.body, 'removeEventListener' ).mockImplementation( ( event ) => { + events[ event ] -= 1; + if ( events[ event ] === 0 ) { + delete events[ event ]; + } + } ); + } ); + + afterEach( () => { + jest.restoreAllMocks(); + } ); + + it( 'removes added event listeners', () => { + linkHijack( 'tableofcontents', '1' )(); + linkHijack( 'tableofcontents', '1' )(); + + expect( Object.keys( events ).length ).toBe( 0 ); + } ); + } ); +} );