Remove Table of Contents feature flag

- Update related selectors, styles.
- Remove unneeded styles.
- Remove link hijack js.
- Simplify hook to only add experiment name to body.

Bug: T310527
Change-Id: I25527261d529a16e28f1b90f2f5af234d26fd40f
This commit is contained in:
Clare Ming 2022-07-01 14:16:36 -06:00
parent 22969cb39d
commit 28732cf4f7
11 changed files with 69 additions and 440 deletions

View File

@ -206,26 +206,6 @@ final class Constants {
*/
public const FEATURE_LANGUAGE_ALERT_IN_SIDEBAR = 'LanguageAlertInSidebar';
/**
* @var string
*/
public const REQUIREMENT_TABLE_OF_CONTENTS = 'TableOfContents';
/**
* @var string
*/
public const CONFIG_TABLE_OF_CONTENTS = 'VectorTableOfContents';
/**
* @var string
*/
public const QUERY_PARAM_TABLE_OF_CONTENTS = 'tableofcontents';
/**
* @var string
*/
public const FEATURE_TABLE_OF_CONTENTS = 'TableOfContents';
/**
* @var string
*/

View File

@ -543,41 +543,14 @@ class Hooks implements
}
$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";
}
$classes[] = $experimentConfig[ 'name' ];
}
return $classes;

View File

@ -145,29 +145,6 @@ return [
]
);
// Feature: T297610: Table of Contents
// ================================
$featureManager->registerRequirement(
new OverridableConfigRequirement(
$services->getMainConfig(),
$context->getUser(),
$context->getRequest(),
null,
Constants::CONFIG_TABLE_OF_CONTENTS,
Constants::REQUIREMENT_TABLE_OF_CONTENTS,
Constants::QUERY_PARAM_TABLE_OF_CONTENTS,
null
)
);
$featureManager->registerFeature(
Constants::FEATURE_TABLE_OF_CONTENTS,
[
Constants::REQUIREMENT_FULLY_INITIALISED,
Constants::REQUIREMENT_TABLE_OF_CONTENTS
]
);
// Feature: Sticky header
// ================================
$featureManager->registerRequirement(
@ -222,15 +199,13 @@ return [
)
);
// Requires both table of contents and title above tabs
// to be enabled to simplify the number of variants it needs
// to consider.
// Requires title above tabs to be enabled to simplify the
// number of variants it needs to consider.
$featureManager->registerFeature(
Constants::FEATURE_GRID,
[
Constants::REQUIREMENT_FULLY_INITIALISED,
Constants::REQUIREMENT_GRID,
Constants::REQUIREMENT_TABLE_OF_CONTENTS,
]
);

View File

@ -22,6 +22,8 @@ class SkinVector22 extends SkinVector {
public function __construct( array $options ) {
if ( !$this->isTOCABTestEnabled() ) {
$options['toc'] = !$this->isTableOfContentsVisibleInSidebar();
} else {
$options['styles'][] = 'skins.vector.AB.styles';
}
parent::__construct( $options );
@ -39,23 +41,9 @@ class SkinVector22 extends SkinVector {
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.
* TOC is visible on main namespaces except for the Main Page.
*
* @internal
* @return bool
@ -74,7 +62,7 @@ class SkinVector22 extends SkinVector {
return $title->getArticleID() !== 0;
}
return $this->isTOCEnabled();
return true;
}
/**

View File

@ -0,0 +1,7 @@
.skin-vector-toc-experiment-control .mw-table-of-contents-container,
.skin-vector-toc-experiment-treatment #toc,
.skin-vector-toc-experiment-unsampled .mw-table-of-contents-container {
// This trumps any layout rules e.g. vector-layout-grid
/* stylelint-disable-next-line declaration-no-important */
display: none !important;
}

View File

@ -1,71 +0,0 @@
/**
* 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;

View File

@ -7,7 +7,6 @@ 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',
@ -18,7 +17,6 @@ const
TOC_SCROLL_HOOK = 'table_of_contents',
PAGE_TITLE_SCROLL_HOOK = 'page_title',
PAGE_TITLE_INTERSECTION_CLASS = 'vector-below-page-title',
TOC_QUERY_PARAM = 'tableofcontents',
TOC_EXPERIMENT_NAME = 'skin-vector-toc-experiment';
/**
@ -214,10 +212,6 @@ const main = () => {
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;

View File

@ -18,68 +18,54 @@
#p-navigation .vector-menu-heading {
display: none;
}
// Temporary magic number, will be calculated after TOC specs are finalized
padding: 12px 19px 12px 9px;
background-image: none;
background-color: @background-color-secondary--modern;
}
// FIXME: Delete this selector when .vector-toc-enabled is removed (T310527)
body:not( .vector-toc-enabled ) .mw-sidebar {
width: @width-grid-column-one;
// To avoid the white part of the gradient colliding with the sidebar links
// we apply top and bottom padding.
padding: 8px 0 40px @padding-left-sidebar;
background-image: linear-gradient( to bottom, @background-color-base 0%, @background-color-secondary--modern 10%, @background-color-secondary--modern 90%, @background-color-base 100% );
}
// Update styling to account for TOC.
.mw-sidebar,
.sidebar-toc,
.sidebar-toc:after {
// Match styles between TOC and fade element to ensure the fade covers the correct area
width: @width-sidebar;
margin-left: 0;
// Update styling when TOC is enabled
.vector-toc-enabled {
.mw-sidebar,
.sidebar-toc,
.sidebar-toc:after {
// Match styles between TOC and fade element to ensure the fade covers the correct area
width: @width-sidebar;
margin-left: 0;
@media ( min-width: @min-width-desktop-wide ) {
width: @width-sidebar-wide;
margin-left: @margin-start-sidebar-content;
}
}
.mw-sidebar {
// Temporary magic number, will be calculated after TOC specs are finalized
padding: 12px 19px 12px 9px;
background-image: none;
background-color: @background-color-secondary--modern;
}
& .vector-layout-grid {
@media ( min-width: @min-width-desktop ) {
.mw-sidebar {
// Prevent grid row gap from affecting TOC spacing when main menu is open
margin-bottom: -@grid-row-gap;
}
}
.sidebar-toc {
// Use margin-top to align TOC rather than grid row gap
// Applies when the TOC sticky and when the main menu is both open and closed.
margin-top: @margin-top-sidebar-toc;
}
@media ( min-width: @min-width-desktop-wide ) {
width: @width-sidebar-wide;
margin-left: @margin-start-sidebar-content;
}
}
// FIXME: Merge margin-top styles with above when .vector-toc-enabled is removed (T310527)
.sidebar-toc {
.vector-toc-enabled .vector-layout-legacy & {
.vector-layout-grid {
@media ( min-width: @min-width-desktop ) {
.mw-sidebar {
// Prevent grid row gap from affecting TOC spacing when main menu is open
margin-bottom: -@grid-row-gap;
}
}
.sidebar-toc {
// Use margin-top to align TOC rather than grid row gap
// Applies when the TOC sticky and when the main menu is both open and closed.
margin-top: @margin-top-sidebar-toc;
}
}
.vector-layout-legacy {
.sidebar-toc {
// Main menu is closed
margin-top: @margin-top-sidebar-toc_title_inline;
}
.vector-toc-enabled .vector-layout-legacy @{selector-workspace-container-sidebar-open} & {
@{selector-workspace-container-sidebar-open} .sidebar-toc {
// Main menu is open
margin-top: @margin-top-sidebar-toc;
}
.vector-toc-enabled.vector-sticky-header-visible .vector-layout-legacy & {
.vector-sticky-header-visible & .sidebar-toc {
// Sticky header is visible
margin-top: @margin-top-sidebar-toc;
}
@ -152,7 +138,6 @@ body:not( .vector-toc-enabled ) .mw-sidebar {
// instead to avoid hidden rendering.
visibility: hidden;
opacity: 0;
transform: translateX( -100% );
}
@media ( min-width: ( @max-width-workspace-container + ( 2 * @padding-horizontal-page-container ) ) ) {
@ -161,27 +146,3 @@ body:not( .vector-toc-enabled ) .mw-sidebar {
border-right: @border-width-base @border-style-base @border-color-sidebar;
}
}
// Disable transitions on page load. No-JS users will unfortunately miss out. A similar pattern is
// used in Minerva's DropDownList. See enableCssAnimations() in skin.vector.js/index.js for context
// and additional details on how this class is added.
.vector-animations-ready {
// Enable transition on all widths by default.
.mw-sidebar {
transition-property: transform, opacity, visibility;
transition-duration: @transition-duration-base;
transition-timing-function: ease-out;
}
@media ( max-width: @max-width-mobile ) {
.mw-sidebar {
transition: none;
}
}
// Enable sidebar button transitions.
#mw-sidebar-button {
transition-property: background-color, border-color, box-shadow;
transition-duration: @transition-duration-base;
}
}

View File

@ -170,8 +170,9 @@ body {
}
.vector-layout-legacy #mw-panel {
position: absolute;
position: static;
top: 0;
float: none;
// Sidebar is displaced from the workspace container so that the
// sidebar is flush with the edge of the screen at small widths.
left: -@padding-horizontal-page-container;
@ -182,15 +183,9 @@ body {
}
}
// Update positioning when TOC is enabled
.vector-toc-enabled .vector-layout-legacy #mw-panel {
position: static;
float: none;
}
// Add float at higher resolutions
@media ( min-width: @min-width-desktop ) {
.vector-toc-enabled .vector-layout-legacy #mw-panel {
.vector-layout-legacy #mw-panel {
float: left;
}
}
@ -315,18 +310,8 @@ body {
padding-bottom: 82px;
}
// FIXME: Delete when .vector-toc-enabled is removed (T310527)
// We want it to appear like the sidebar is going into/coming out of
// `.mw-page-container`, but we can't use `overflow: hidden` on
// `.mw-page-container` because that will cut off the sidebar. Therefore, we
// calculate the maximum distance from the start of `mw-page-container` to the
// start of the sidebar.
@{selector-workspace-container-sidebar-closed} .mw-sidebar {
transform: translateX( -( @max-width-page-container - @max-width-workspace-container ) / 2 );
}
// Hide sidebar entirely when the checkbox is disabled and the TOC is enabled
.vector-toc-enabled @{selector-workspace-container-sidebar-closed} .mw-sidebar {
@{selector-workspace-container-sidebar-closed} .mw-sidebar {
display: none;
}
@ -347,13 +332,8 @@ body {
}
@media ( max-width: @max-width-margin-start-content ) {
// Adjusts the content and mw-article-toolbar-container.
// Increase margin to account for TOC
.vector-layout-legacy @{selector-workspace-container-sidebar-open} .mw-content-container {
margin-left: @margin-start-content;
}
// Increase margin when TOC is enabled
.vector-toc-enabled .vector-layout-legacy @{selector-workspace-container-sidebar-open} .mw-content-container {
margin-left: @margin-toc-start-content;
}
@ -364,11 +344,6 @@ body {
}
}
.skin-vector-toc-experiment-control .mw-table-of-contents-container,
.skin-vector-toc-experiment-unsampled .mw-table-of-contents-container {
display: none;
}
// Cannot use display: none on legacy TOC because it needs to be accessible
// to scrollObserver for the TOC A/B test (T303297)
// Instead we hide the contents of the legacy TOC and reset it's styles
@ -389,9 +364,9 @@ body {
// HTML but only one is actually visible. Prevent the left margin from undesirably
// applying if bucketed into the control or unsampled groups which won't show
// the new TOC.
.skin-vector-disable-max-width .vector-toc-enabled .vector-layout-legacy @{selector-workspace-container-sidebar-open} .mw-content-container,
.skin-vector-disable-max-width .vector-layout-legacy @{selector-workspace-container-sidebar-open} .mw-content-container,
body:not( .skin-vector-toc-experiment-control ):not( .skin-vector-toc-experiment-unsampled ) .vector-layout-legacy .vector-toc-visible .mw-workspace-container .mw-content-container,
.vector-toc-enabled .vector-layout-legacy @{selector-workspace-container-sidebar-open} .mw-content-container {
.vector-layout-legacy @{selector-workspace-container-sidebar-open} .mw-content-container {
@media ( min-width: @min-width-desktop ) {
margin-left: @margin-toc-start-content;
}

View File

@ -210,6 +210,17 @@
"resources/skins.vector.styles.legacy/skin-legacy.less"
]
},
"skins.vector.AB.styles": {
"class": "ResourceLoaderSkinModule",
"features": [ "toc" ],
"styles": [
"resources/skins.vector.AB.styles.less"
],
"targets": [
"desktop",
"mobile"
]
},
"skins.vector.styles": {
"class": "ResourceLoaderSkinModule",
"features": {
@ -224,7 +235,8 @@
"interface-category": true,
"i18n-ordered-lists": true,
"i18n-all-lists-margins": true,
"i18n-headings": true
"i18n-headings": true,
"toc": false
},
"targets": [
"desktop",
@ -302,7 +314,6 @@
"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": "MediaWiki\\Skins\\Vector\\Hooks::getVectorResourceLoaderConfig"
@ -506,19 +517,13 @@
"value": false,
"description": "@var boolean Temporary feature flag that disables saving the sidebar expanded/collapsed state as a user-preference (triggered via clicking the main menu icon). This is intended as a temporary kill-switch in the event that the DB is overloaded with writes to the user_options table."
},
"VectorTableOfContents": {
"value": {
"default": true
},
"description": "@var When `VectorTableOfContents` is enabled, the sticky table of contents is shown."
},
"VectorTableOfContentsBeginning": {
"value": true,
"description": "@var boolean Temporary feature flag that controls link to beginning of article."
},
"VectorTableOfContentsCollapseAtCount": {
"value": 20,
"description": "@var When `VectorTableOfContents` is enabled, the minimum number of headings required to collapse all headings in the sticky table of contents by default."
"description": "@var The minimum number of headings required to collapse all headings in the sticky table of contents by default."
},
"VectorGrid": {
"value": false,

View File

@ -1,158 +0,0 @@
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 = `
<a class="anchor" href="/wiki/Barack_Obama">Barack Obama</a>
<a class="anchor-with-span" href="/wiki/Barack_Obama"><span class="inner-span">Barack Obama</span></a>
<a class="anchor-with-hash" href="/wiki/Tree#jump">Jump</a>
<a class="anchor-with-different-origin" href="http://www.google.com/">Different Origin</a>
<a class="anchor-with-query-param" href="/wiki/Barack_Obama?tableofcontents=1">Query Param</a>
<a class="anchor-with-different-query-param" href="/wiki/Barack_Obama?foo=1">Query Param</a>
<a class="anchor-without-href">Anchor without href</a>
<a class="anchor-with-invalid-href href="invalid">Anchor with invalid href</a>
<div></div>
<svg></svg>
`;
} );
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 );
} );
} );
} );