From cfc454f615982521802b4c0d765dabdeea042c73 Mon Sep 17 00:00:00 2001 From: Jan Drewniak Date: Tue, 28 Feb 2023 14:48:31 -0500 Subject: [PATCH] Increase scroll-padding-top for page sections This introduces a general scroll-padding-top value of 50px for Vector 2022's HTML element. This changes makes it so that when navigating sections via the TOC, the section headings are 50px lower on the screen. This change has been synced with the ToC active sections as well, so that active sections become active at the same offset. Bug: T314419 Change-Id: I99e5fe2047f5ad1e61ef51b6ba7e76a6cac36fc5 --- resources/common/variables.less | 4 ++++ resources/skins.vector.es6/main.js | 19 +++++++++++++++---- .../components/StickyHeader.less | 2 +- .../skins.vector.styles/layouts/screen.less | 4 ++++ tests/jest/skins.vector.es6/main.test.js | 6 ++---- 5 files changed, 26 insertions(+), 9 deletions(-) diff --git a/resources/common/variables.less b/resources/common/variables.less index 0db14c8fc..72ddaccd1 100644 --- a/resources/common/variables.less +++ b/resources/common/variables.less @@ -220,6 +220,10 @@ @padding-horizontal-page-container: unit( 24px / @font-size-browser, em ); @padding-horizontal-page-container-desktop: unit( 44px / @font-size-browser, em ); @padding-horizontal-page-container-desktop-wide: unit( 52px / @font-size-browser, em ); +// T314419 - Scroll padding is modified primarily to bring section headings into a position that is +// easier to read, which is slightly lower than the very top of the page. The primary use-case is when +// navigating sections via the table of contents. +@scroll-padding-top: 50px; // Grid @grid-row-gap: 24px; diff --git a/resources/skins.vector.es6/main.js b/resources/skins.vector.es6/main.js index 28605970c..f4b91e8ef 100644 --- a/resources/skins.vector.es6/main.js +++ b/resources/skins.vector.es6/main.js @@ -140,13 +140,24 @@ const updateTocLocation = () => { }; /** - * @param {HTMLElement|null} header + * Return the computed value of the + * `scroll-margin-top` CSS property, of the document element, as a number. + * + * @return {number} Value of scroll-margin-top OR zero if falsy. + */ +function getDocumentScrollPaddingTop() { + const documentStyles = getComputedStyle( document.documentElement ), + scrollPaddingTopString = documentStyles.getPropertyValue( 'scroll-padding-top' ); + return parseInt( scrollPaddingTopString, 10 ) || 0; +} + +/** * @param {HTMLElement|null} tocElement * @param {HTMLElement|null} bodyContent * @param {initSectionObserver} initSectionObserverFn * @return {tableOfContents|null} */ -const setupTableOfContents = ( header, tocElement, bodyContent, initSectionObserverFn ) => { +const setupTableOfContents = ( tocElement, bodyContent, initSectionObserverFn ) => { if ( !( tocElement && bodyContent @@ -202,7 +213,7 @@ const setupTableOfContents = ( header, tocElement, bodyContent, initSectionObser const sectionObserver = initSectionObserverFn( { elements: elements(), - topMargin: header ? header.getBoundingClientRect().height : 0, + topMargin: getDocumentScrollPaddingTop(), onIntersection: getHeadingIntersectionHandler( tableOfContents.changeActiveSection ) } ); const updateElements = () => { @@ -252,7 +263,7 @@ const main = () => { const isToCUpdatingAllowed = isIntersectionObserverSupported && window.requestAnimationFrame; const tableOfContents = isToCUpdatingAllowed ? - setupTableOfContents( header, tocElement, bodyContent, initSectionObserver ) : null; + setupTableOfContents( tocElement, bodyContent, initSectionObserver ) : null; // // Sticky header diff --git a/resources/skins.vector.styles/components/StickyHeader.less b/resources/skins.vector.styles/components/StickyHeader.less index f10816772..d2da09a24 100644 --- a/resources/skins.vector.styles/components/StickyHeader.less +++ b/resources/skins.vector.styles/components/StickyHeader.less @@ -142,7 +142,7 @@ // .vector-sticky-header-visible class to correctly handle situations where the // sticky header isn't visible yet but we still need scroll padding applied // (e.g. when the user navigates to a page with a hash fragment in the URI). - scroll-padding-top: @height-sticky-header; + scroll-padding-top: ~'calc( @{height-sticky-header} + @{scroll-padding-top} )'; .vector-sticky-header { // Sticky header is only enabled for js users and when feature flag is enabled diff --git a/resources/skins.vector.styles/layouts/screen.less b/resources/skins.vector.styles/layouts/screen.less index 79b38d25f..20f1ccbd5 100644 --- a/resources/skins.vector.styles/layouts/screen.less +++ b/resources/skins.vector.styles/layouts/screen.less @@ -22,6 +22,10 @@ // smaller and will start horizontal scrolling instead. @min-width-supported: unit( 500px / @font-size-browser, em ); +html { + scroll-padding-top: @scroll-padding-top; +} + body { background-color: @background-color-secondary--modern; color: @color-base; diff --git a/tests/jest/skins.vector.es6/main.test.js b/tests/jest/skins.vector.es6/main.test.js index 939c0bc67..2eb7ff799 100644 --- a/tests/jest/skins.vector.es6/main.test.js +++ b/tests/jest/skins.vector.es6/main.test.js @@ -191,10 +191,9 @@ describe( 'Table of contents re-rendering', () => { it( 'listens to wikipage.tableOfContents hook when mounted', () => { mockMwHook(); const spy = jest.spyOn( mw, 'hook' ); - const header = document.createElement( 'header' ); const tocElement = document.createElement( 'div' ); const bodyContent = document.createElement( 'article' ); - const toc = test.setupTableOfContents( header, tocElement, bodyContent, sectionObserverFn ); + const toc = test.setupTableOfContents( tocElement, bodyContent, sectionObserverFn ); expect( toc ).not.toBe( null ); expect( spy ).toHaveBeenCalledWith( 'wikipage.tableOfContents' ); expect( spy ).not.toHaveBeenCalledWith( 'wikipage.tableOfContents.vector' ); @@ -202,10 +201,9 @@ describe( 'Table of contents re-rendering', () => { it( 'Firing wikipage.tableOfContents triggers reloadTableOfContents', async () => { mockMwHook(); - const header = document.createElement( 'header' ); const tocElement = document.createElement( 'div' ); const bodyContent = document.createElement( 'article' ); - const toc = test.setupTableOfContents( header, tocElement, bodyContent, sectionObserverFn ); + const toc = test.setupTableOfContents( tocElement, bodyContent, sectionObserverFn ); if ( !toc ) { // something went wrong expect( true ).toBe( false );