Factor out a separate scroll observer for the TOC A/B test, which should be fired separately from the page title observer used by the sticky header and TOC

Bug: T307952
Bug: T307345
Change-Id: I3f247730fa1c399e6d2e4d866677703fc24e8c58
(cherry picked from commit 94be7578f5)

resources/common/variables.less

@@ -113,9 +113,12 @@
 @margin-horizontal-sidebar-button-icon: unit( 12px / @font-size-browser, em ); // 0.75em @ 16
 
 // Sidebar
-@margin-toc-start-content: 18.5em;
-@space-end-sidebar: 3.25em;
-@width-sidebar: @margin-toc-start-content - @space-end-sidebar;
+@width-sidebar-px: 220;
+@width-sidebar-px-wide: 244;
+@margin-toc-start-content: unit( ( @width-sidebar-px + 24 ) / @font-size-browser, em );
+@margin-toc-start-content-wide: unit( ( @width-sidebar-px-wide + 52 ) / @font-size-browser, em ); // 18.5em;
+@width-sidebar: unit( @width-sidebar-px / @font-size-browser, em );
+@width-sidebar-wide: unit( @width-sidebar-px-wide / @font-size-browser, em );
 
 // Search
 @min-width-search-button: 28px;
@@ -172,4 +175,5 @@
 // Layout
 //
 @max-width-page-container: unit( 1514px / @font-size-browser, em ); // 99.75em @ 16
-@padding-horizontal-page-container: unit( 40px / @font-size-browser, em ); // 1.875em @ 16
+@padding-horizontal-page-container: unit( 32px / @font-size-browser, em );
+@padding-horizontal-page-container-wide: unit( 40px / @font-size-browser, em );

resources/skins.vector.es6/main.js

@@ -18,7 +18,8 @@ const
 	TOC_SCROLL_HOOK = 'table_of_contents',
 	PAGE_TITLE_SCROLL_HOOK = 'page_title',
 	TOC_QUERY_PARAM = 'tableofcontents',
-	TOC_EXPERIMENT_NAME = 'skin-vector-toc-experiment';
+	TOC_EXPERIMENT_NAME = 'skin-vector-toc-experiment',
+	SCROLL_TOC_BELOW_CLASS = 'vector-scrolled-below-table-of-contents';
 
 /**
  * @callback OnIntersection
@@ -95,30 +96,22 @@ const main = () => {
 	const tocElement = document.getElementById( TOC_ID );
 	const tocElementLegacy = document.getElementById( TOC_ID_LEGACY );
 	const bodyContent = document.getElementById( BODY_CONTENT_ID );
-	const tocLegacyPlaceholder = document.getElementsByTagName( TOC_LEGACY_PLACEHOLDER_TAG )[ 0 ];
-	const tocLegacyTargetIntersection = tocElementLegacy || tocLegacyPlaceholder;
 
-	// Set up intersection observer for sticky header and table of contents functionality
-	// and to fire scroll event hooks for event logging if AB tests are enabled for
-	// either feature.
+	// Set up intersection observer for page title, used by sticky header
 	const observer = scrollObserver.initScrollObserver(
 		() => {
 			if ( isStickyHeaderAllowed && isStickyHeaderEnabled ) {
 				stickyHeader.show( header );
+				document.body.classList.add( SCROLL_TOC_BELOW_CLASS );
 			}
 			scrollObserver.fireScrollHook( 'down', PAGE_TITLE_SCROLL_HOOK );
-			if ( tocLegacyTargetIntersection ) {
-				scrollObserver.fireScrollHook( 'down', TOC_SCROLL_HOOK );
-			}
 		},
 		() => {
 			if ( isStickyHeaderAllowed && isStickyHeaderEnabled ) {
 				stickyHeader.hide( header );
+				document.body.classList.remove( SCROLL_TOC_BELOW_CLASS );
 			}
 			scrollObserver.fireScrollHook( 'up', PAGE_TITLE_SCROLL_HOOK );
-			if ( tocLegacyTargetIntersection ) {
-				scrollObserver.fireScrollHook( 'up', TOC_SCROLL_HOOK );
-			}
 		}
 	);
 
@@ -133,9 +126,21 @@ const main = () => {
 		observer.observe( stickyIntersection );
 	}
 
+	// Setup intersection observer for TOC scroll event tracking
+	// fire hooks for event logging if AB tests are enabled
+	const tocLegacyPlaceholder = document.getElementsByTagName( TOC_LEGACY_PLACEHOLDER_TAG )[ 0 ];
+	const tocLegacyTargetIntersection = tocElementLegacy || tocLegacyPlaceholder;
 	// Initiate observer for table of contents in main content.
 	if ( tocLegacyTargetIntersection ) {
-		observer.observe( tocLegacyTargetIntersection );
+		const tocObserver = scrollObserver.initScrollObserver(
+			() => {
+				scrollObserver.fireScrollHook( 'down', TOC_SCROLL_HOOK );
+			},
+			() => {
+				scrollObserver.fireScrollHook( 'up', TOC_SCROLL_HOOK );
+			}
+		);
+		tocObserver.observe( tocLegacyTargetIntersection );
 	}
 
 	// Add event data attributes to legacy TOC

resources/skins.vector.es6/scrollObserver.js

@@ -7,8 +7,7 @@ const
 	SCROLL_TOC_CONTEXT_ABOVE = 'scrolled-above-table-of-contents',
 	SCROLL_TOC_CONTEXT_BELOW = 'scrolled-below-table-of-contents',
 	SCROLL_TOC_ACTION = 'scroll-to-toc',
-	SCROLL_TOC_PARAMETER = 'table_of_contents',
-	SCROLL_TOC_BELOW_CLASS = 'vector-scrolled-below-table-of-contents';
+	SCROLL_TOC_PARAMETER = 'table_of_contents';
 
 /**
  * @typedef {Object} scrollVariables
@@ -55,19 +54,11 @@ function fireScrollHook( direction, hook ) {
 		mw.hook( scrollVariables.scrollHook ).fire( {
 			context: scrollVariables.scrollContextBelow
 		} );
-		// Piggy back on this function to apply needed class for shifting TOC.
-		if ( hook === SCROLL_TOC_PARAMETER ) {
-			document.body.classList.add( SCROLL_TOC_BELOW_CLASS );
-		}
 	} else {
 		mw.hook( scrollVariables.scrollHook ).fire( {
 			context: scrollVariables.scrollContextAbove,
 			action: scrollVariables.scrollAction
 		} );
-		// Piggy back on this function to apply needed class for shifting TOC.
-		if ( hook === SCROLL_TOC_PARAMETER ) {
-			document.body.classList.remove( SCROLL_TOC_BELOW_CLASS );
-		}
 	}
 }
 

resources/skins.vector.es6/tableOfContents.js

@@ -294,9 +294,13 @@ module.exports = function tableOfContents( props ) {
 				/** @type {HTMLElement | null} */ ( e.target.closest( `.${SECTION_CLASS}` ) );
 
 			if ( tocSection && tocSection.id ) {
-				if ( e.target.classList.contains( LINK_CLASS ) ) {
+				// In case section link contains HTML,
+				// test if click occurs on any child elements.
+				if ( e.target.closest( `.${LINK_CLASS}` ) ) {
 					props.onHeadingClick( tocSection.id );
 				}
+				// Toggle button does not contain child elements,
+				// so classList check will suffice.
 				if ( e.target.classList.contains( TOGGLE_CLASS ) ) {
 					props.onToggleClick( tocSection.id );
 				}

resources/skins.vector.styles/components/Sidebar.less

@@ -30,15 +30,25 @@
 		// Temporary magic number, will be calculated after TOC specs are finalized
 		padding: 12px 19px 12px 9px;
 		margin-top: @top-margin-sidebar-content;
-		margin-left: @margin-start-sidebar-content;
 		background-image: none;
 		background-color: @border-color-sidebar;
+
+		@media ( min-width: @width-breakpoint-desktop-wide ) {
+			width: @width-sidebar-wide;
+		}
 	}
 
 	.sidebar-toc {
-		position: relative;
 		margin-top: @top-margin-sidebar-toc_title_inline;
-		margin-left: @margin-start-sidebar-content;
+	}
+
+	.mw-sidebar,
+	.sidebar-toc {
+		margin-left: 0;
+
+		@media ( min-width: @width-breakpoint-desktop-wide ) {
+			margin-left: @margin-start-sidebar-content;
+		}
 	}
 
 	// T305069 Apply different top spacing to the topmost sidebar element during menu toggling when sidebar is open.

resources/skins.vector.styles/components/StickyHeader.less

@@ -13,7 +13,6 @@
 	transition: @transition-sticky-header;
 	display: flex;
 	align-items: center;
-	max-width: @max-width-page-container + @padding-horizontal-page-container + @padding-horizontal-page-container;
 	margin: 0 auto;
 	background: @background-color-base;
 	background-color: #fffffff7;
@@ -25,6 +24,12 @@
 
 	@media ( min-width: @width-breakpoint-desktop ) {
 		padding: 6px 25px;
+		max-width: @max-width-page-container + ( @padding-horizontal-page-container * 2 );
+	}
+
+	@media ( min-width: @width-breakpoint-desktop-wide ) {
+		padding: 6px 25px;
+		max-width: @max-width-page-container + ( @padding-horizontal-page-container-wide * 2 );
 	}
 
 	// T298836 Hide the sticky header at lower resolutions.

resources/skins.vector.styles/components/TableOfContents.less

@@ -1,6 +1,5 @@
 @import '../../common/variables.less';
 
-@subcategory-padding: 8px;
 @fade-height: 40px;
 @vertical-padding: 20px;
 @toggle-icon-size: 1.834em;
@@ -9,11 +8,16 @@
 	display: none;
 	width: @width-sidebar;
 	max-height: 75vh;
-	padding: @vertical-padding @subcategory-padding @vertical-padding ~'calc( @{subcategory-padding} * 4 )';
+	padding: @vertical-padding 12px @vertical-padding 32px;
 	.box-sizing( border-box );
 	overflow: auto;
 	background-color: @border-color-sidebar;
 
+	@media ( min-width: @width-breakpoint-desktop-wide ) {
+		width: @width-sidebar-wide;
+		padding-right: 8px;
+	}
+
 	.sidebar-toc-header {
 		padding-bottom: 12px;
 	}
@@ -26,26 +30,17 @@
 		border: 0;
 	}
 
-	.sidebar-toc-link {
-		word-break: break-word;
-
-		> * {
-			// Prevent click events on the link's contents so that we can use event
-			// delegation and have the target be the anchor element instead.
-			pointer-events: none;
-		}
-	}
-
 	.sidebar-toc-numb {
 		display: none;
 	}
 
 	.sidebar-toc-link {
+		word-break: break-word;
 		color: @color-link;
 	}
 
 	.sidebar-toc-text {
-		padding: ~'calc( @{subcategory-padding} / 2 )'  0;
+		padding: 4px  0;
 	}
 
 	.sidebar-toc-contents,
@@ -63,7 +58,7 @@
 		display: block;
 		position: relative;
 		list-style-type: none;
-		padding-left: @subcategory-padding;
+		padding-left: 8px;
 
 		a {
 			font-size: @font-size-base;
@@ -86,12 +81,11 @@
 	display: block;
 	position: absolute;
 	bottom: 0;
-	left: 0;
-	right: 0;
+	left: @margin-start-sidebar-content; // T306893: Ensure scrollable indicator is contained in the TOC
+	right: @border-width-base; // Ensure scrollable indicator doesn't cover border/scrollbar
 	height: @fade-height;
 	background: linear-gradient( rgba( 255, 255, 255, 0 ), @border-color-sidebar );
 	pointer-events: none; // Make the link below the fade clickable
-	margin: 0 @border-width-base;
 }
 
 // Highlight active section

resources/skins.vector.styles/layouts/screen.less

@@ -57,7 +57,7 @@
 // smaller and will start horizontal scrolling instead.
 @min-width-supported:
 	unit( 500px / @font-size-browser, em ) -
-	( 2 * @padding-horizontal-page-container );
+	( 2 * @padding-horizontal-page-container-wide );
 // 31.25em - 3.75em = 27.5em @ 16
 @border-color-sidebar: @background-color-secondary--modern;
 @background-color-secondary--modern: #f8f9fa;
@@ -155,6 +155,10 @@ body {
 	// sidebar is flush with the edge of the screen at small widths.
 	left: -@padding-horizontal-page-container;
 	z-index: @z-index-sidebar;
+
+	@media ( min-width: @width-breakpoint-desktop-wide ) {
+		left: -@padding-horizontal-page-container-wide;
+	}
 }
 
 // Update positioning when TOC is enabled
@@ -207,6 +211,16 @@ body {
 	// For devices too small, they should be more useable with horizontal scrolling.
 	// e.g. Portrait on an iPad
 	min-width: @min-width-supported;
+
+	@media ( min-width: @width-breakpoint-desktop ) {
+		padding-left: @padding-horizontal-page-container;
+		padding-right: @padding-horizontal-page-container;
+	}
+
+	@media ( min-width: @width-breakpoint-desktop-wide ) {
+		padding-left: @padding-horizontal-page-container-wide;
+		padding-right: @padding-horizontal-page-container-wide;
+	}
 }
 
 .skin--responsive .mw-page-container {
@@ -287,13 +301,6 @@ body {
 	}
 }
 
-// Increase margin when TOC is enabled
-.vector-toc-enabled .mw-checkbox-hack-checkbox:checked ~ .mw-workspace-container .mw-content-container {
-	.skin-vector-disable-max-width & {
-		margin-left: @margin-toc-start-content;
-	}
-}
-
 @media ( max-width: @max-width-margin-start-content ) {
 	// Adjusts the content and mw-article-toolbar-container.
 	.mw-checkbox-hack-checkbox:checked ~ .mw-workspace-container .mw-content-container,
@@ -315,8 +322,20 @@ body {
 }
 
 .skin-vector-toc-experiment-control .mw-table-of-contents-container,
-.skin-vector-toc-experiment-unsampled .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
+// See I3f247730fa1c399e6d2e4d866677703fc24e8c58
 .skin-vector-toc-experiment-treatment #toc {
+	padding: 0;
+	border: 0;
+}
+
+.skin-vector-toc-experiment-treatment #toc > * {
 	display: none;
 }
 
@@ -327,6 +346,7 @@ 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 .mw-checkbox-hack-checkbox:checked ~ .mw-workspace-container .mw-content-container,
 body:not( .skin-vector-toc-experiment-control ):not( .skin-vector-toc-experiment-unsampled ) .vector-toc-visible .mw-workspace-container .mw-content-container,
 body:not( .skin-vector-toc-experiment-control ):not( .skin-vector-toc-experiment-unsampled ) .vector-toc-visible .mw-workspace-container .mw-article-toolbar-container,
 .vector-toc-enabled .mw-checkbox-hack-checkbox:checked ~ .mw-workspace-container .mw-content-container,
@@ -334,12 +354,9 @@ body:not( .skin-vector-toc-experiment-control ):not( .skin-vector-toc-experiment
 	@media ( min-width: @width-breakpoint-desktop ) {
 		margin-left: @margin-toc-start-content;
 	}
-}
 
-@media ( min-width: @width-breakpoint-desktop ) {
-	.mw-page-container {
-		padding-left: @padding-horizontal-page-container;
-		padding-right: @padding-horizontal-page-container;
+	@media ( min-width: @width-breakpoint-desktop-wide ) {
+		margin-left: @margin-toc-start-content-wide;
 	}
 }