From 8a0b913cc7daa0a3d2b15000f06b307e4445eeb7 Mon Sep 17 00:00:00 2001 From: Chris Holder Date: Wed, 8 Jan 2025 10:50:13 -0600 Subject: [PATCH 1/6] Track scroll depth on unmount of full post --- client/blocks/reader-full-post/index.jsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client/blocks/reader-full-post/index.jsx b/client/blocks/reader-full-post/index.jsx index b9201197e0be5..b9373f71b9372 100644 --- a/client/blocks/reader-full-post/index.jsx +++ b/client/blocks/reader-full-post/index.jsx @@ -197,6 +197,8 @@ export class FullPostView extends Component { document.removeEventListener( 'keydown', this.handleKeydown, true ); document.removeEventListener( 'visibilitychange', this.handleVisibilityChange ); + // Track scroll depth and remove related instruments + this.trackScrollDepth( this.props.post ); if ( this.scrollableContainer ) { this.scrollableContainer.removeEventListener( 'scroll', this.setScrollDepth ); } From 2a6fded828052163c493aff688d56d1e37d00368 Mon Sep 17 00:00:00 2001 From: Chris Holder Date: Wed, 8 Jan 2025 13:19:03 -0600 Subject: [PATCH 2/6] Simplify scroll depth calculation --- client/blocks/reader-full-post/index.jsx | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/client/blocks/reader-full-post/index.jsx b/client/blocks/reader-full-post/index.jsx index b9373f71b9372..4f593fea46e06 100644 --- a/client/blocks/reader-full-post/index.jsx +++ b/client/blocks/reader-full-post/index.jsx @@ -295,13 +295,16 @@ export class FullPostView extends Component { setScrollDepth = () => { if ( this.scrollableContainer ) { - const scrollTop = this.scrollableContainer.scrollTop; - const scrollHeight = this.scrollableContainer.scrollHeight; - const clientHeight = this.scrollableContainer.clientHeight; - const scrollDepth = ( scrollTop / ( scrollHeight - clientHeight ) ) * 100; + const scrollTop = this.scrollableContainer.scrollTop ?? 0; + const scrollHeight = this.scrollableContainer.scrollHeight ?? 0; + const clientHeight = this.scrollableContainer.clientHeight ?? 0; + + const denominator = scrollHeight - clientHeight; + const scrollDepth = denominator <= 0 ? 0 : scrollTop / denominator; + this.setState( ( prevState ) => ( { - maxScrollDepth: Math.max( prevState.maxScrollDepth, scrollDepth ) || 0, - hasCompleted: prevState.hasCompleted || scrollDepth >= 90, + maxScrollDepth: Math.min( 1, Math.max( 0, prevState.maxScrollDepth || 0, scrollDepth ) ), + hasCompleted: prevState.hasCompleted || scrollDepth >= 0.9, } ) ); } }; @@ -313,10 +316,11 @@ export class FullPostView extends Component { } if ( this.scrollableContainer && post.ID ) { - const roundedDepth = Math.round( maxScrollDepth * 100 ) / 100; + const scrollDepthPercentage = Math.round( maxScrollDepth * 100 ); + recordTrackForPost( 'calypso_reader_article_scroll_depth', post, { context: 'full-post', - scroll_depth: roundedDepth, + scroll_depth: scrollDepthPercentage, } ); } }; From 8f9f5a08bcc189a0ff56a584f2599626214d2888 Mon Sep 17 00:00:00 2001 From: Chris Holder Date: Wed, 8 Jan 2025 14:46:03 -0600 Subject: [PATCH 3/6] Keep scrolling container consistent between mobile and desktop --- client/reader/recent/style.scss | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client/reader/recent/style.scss b/client/reader/recent/style.scss index 1cef2a3df7df0..d46fb95feae86 100644 --- a/client/reader/recent/style.scss +++ b/client/reader/recent/style.scss @@ -18,6 +18,7 @@ body.is-reader-full-post { ); height: $feed-content-height !important; // Using !important here to avoid having a lengthy selector. + overflow-y: hidden !important; // Keeps the scrolling container consistent between one and two column layouts. @media ( min-width: $break-wide ) { display: flex; @@ -200,6 +201,7 @@ body.is-reader-full-post { @extend %column-shared; display: none; overflow-y: auto; + height: 100%; &.overlay { display: block; From 2a2f8ded114bfba0070dce8e45bbe66aed2a323b Mon Sep 17 00:00:00 2001 From: Chris Holder Date: Wed, 8 Jan 2025 16:12:46 -0600 Subject: [PATCH 4/6] Separate tracking code from component code and compose together --- client/blocks/reader-full-post/index.jsx | 47 +++-------- .../blocks/reader-full-post/scroll-tracker.ts | 81 +++++++++++++++++++ 2 files changed, 92 insertions(+), 36 deletions(-) create mode 100644 client/blocks/reader-full-post/scroll-tracker.ts diff --git a/client/blocks/reader-full-post/index.jsx b/client/blocks/reader-full-post/index.jsx index 4f593fea46e06..3afdc199f337c 100644 --- a/client/blocks/reader-full-post/index.jsx +++ b/client/blocks/reader-full-post/index.jsx @@ -77,6 +77,7 @@ import isSiteWPForTeams from 'calypso/state/selectors/is-site-wpforteams'; import { disableAppBanner, enableAppBanner } from 'calypso/state/ui/actions'; import ReaderFullPostHeader from './header'; import ReaderFullPostContentPlaceholder from './placeholders/content'; +import ScrollTracker from './scroll-tracker'; import ReaderFullPostUnavailable from './unavailable'; import './style.scss'; @@ -101,8 +102,6 @@ export class FullPostView extends Component { state = { isSuggestedFollowsModalOpen: false, - maxScrollDepth: 0, // Track the maximum scroll depth achieved - hasCompleted: false, // Track whether the user completed the post }; openSuggestedFollowsModal = ( followClicked ) => { @@ -113,6 +112,7 @@ export class FullPostView extends Component { }; componentDidMount() { + this.scrollTracker = new ScrollTracker(); // Send page view this.hasSentPageView = false; this.hasLoaded = false; @@ -141,8 +141,8 @@ export class FullPostView extends Component { document.querySelector( '#primary > div > div.recent-feed > section' ) || // for Recent Feed in Dataview document.querySelector( '#primary > div > div' ); // for Recent Feed in Stream if ( scrollableContainer ) { - scrollableContainer.addEventListener( 'scroll', this.setScrollDepth ); - this.scrollableContainer = scrollableContainer; // Save reference for cleanup + this.scrollableContainer = scrollableContainer; + this.scrollTracker.setContainer( scrollableContainer ); this.resetScroll(); } } @@ -199,9 +199,7 @@ export class FullPostView extends Component { // Track scroll depth and remove related instruments this.trackScrollDepth( this.props.post ); - if ( this.scrollableContainer ) { - this.scrollableContainer.removeEventListener( 'scroll', this.setScrollDepth ); - } + this.scrollTracker.cleanup(); this.clearResetScrollTimeout(); } @@ -282,55 +280,32 @@ export class FullPostView extends Component { resetScroll = () => { this.clearResetScrollTimeout(); this.resetScrollTimeout = setTimeout( () => { - if ( this.scrollableContainer ) { - this.scrollableContainer.scrollTo( { - top: 0, - left: 0, - behavior: 'instant', - } ); - } - this.setState( { maxScrollDepth: 0, hasCompleted: false } ); + this.scrollTracker.resetMaxScrollDepth(); }, 0 ); // Defer until after the DOM update }; - setScrollDepth = () => { - if ( this.scrollableContainer ) { - const scrollTop = this.scrollableContainer.scrollTop ?? 0; - const scrollHeight = this.scrollableContainer.scrollHeight ?? 0; - const clientHeight = this.scrollableContainer.clientHeight ?? 0; - - const denominator = scrollHeight - clientHeight; - const scrollDepth = denominator <= 0 ? 0 : scrollTop / denominator; - - this.setState( ( prevState ) => ( { - maxScrollDepth: Math.min( 1, Math.max( 0, prevState.maxScrollDepth || 0, scrollDepth ) ), - hasCompleted: prevState.hasCompleted || scrollDepth >= 0.9, - } ) ); - } - }; - trackScrollDepth = ( post = null ) => { - const { maxScrollDepth } = this.state; if ( ! post ) { post = this.props.post; } if ( this.scrollableContainer && post.ID ) { - const scrollDepthPercentage = Math.round( maxScrollDepth * 100 ); - + const maxScrollDepth = this.scrollTracker.getMaxScrollDepthAsPercentage(); recordTrackForPost( 'calypso_reader_article_scroll_depth', post, { context: 'full-post', - scroll_depth: scrollDepthPercentage, + scroll_depth: maxScrollDepth, } ); } }; trackExitBeforeCompletion = ( post = null ) => { - const { hasCompleted, maxScrollDepth } = this.state; if ( ! post ) { post = this.props.post; } + const maxScrollDepth = this.scrollTracker.getMaxScrollDepthAsPercentage(); + const hasCompleted = maxScrollDepth >= 90; // User has read 90% of the post + if ( this.scrollableContainer && post.ID && ! hasCompleted ) { recordTrackForPost( 'calypso_reader_article_exit_before_completion', post, { context: 'full-post', diff --git a/client/blocks/reader-full-post/scroll-tracker.ts b/client/blocks/reader-full-post/scroll-tracker.ts new file mode 100644 index 0000000000000..233fac07c2737 --- /dev/null +++ b/client/blocks/reader-full-post/scroll-tracker.ts @@ -0,0 +1,81 @@ +/** + * Tracks scroll depth for a container element. + */ +export default class ScrollTracker { + private container: HTMLElement | null = null; + private maxScrollDepth: number = 0; + + private handleScroll = (): void => { + if ( ! this.container ) { + return; + } + + const scrollTop = this.container.scrollTop ?? 0; + const scrollHeight = this.container.scrollHeight ?? 0; + const clientHeight = this.container.clientHeight ?? 0; + + const denominator = scrollHeight - clientHeight; + const scrollDepth = denominator <= 0 ? 0 : scrollTop / denominator; + + this.maxScrollDepth = calcAndClampMaxValue( scrollDepth, this.maxScrollDepth ); + }; + + /** + * Sets the container element to track scrolling on. + * Removes scroll listener from previous container if it exists. + * @param container - The HTML element to track scrolling on, or null to stop tracking + */ + public setContainer( container: HTMLElement | null ): void { + if ( this.container ) { + this.container.removeEventListener( 'scroll', this.handleScroll ); + } + + this.container = container; + if ( container ) { + container.addEventListener( 'scroll', this.handleScroll ); + } + } + + /** + * Gets the maximum scroll depth reached as a decimal between 0 and 1. + * @returns A number between 0 and 1 representing the maximum scroll depth + */ + public getMaxScrollDepth(): number { + return this.maxScrollDepth; + } + + /** + * Gets the maximum scroll depth reached as a percentage between 0 and 100. + * @returns A rounded number between 0 and 100 representing the maximum scroll depth percentage + */ + public getMaxScrollDepthAsPercentage(): number { + return Math.round( this.maxScrollDepth * 100 ); + } + + /** + * Resets the maximum scroll depth back to 0. + */ + public resetMaxScrollDepth = (): void => { + this.maxScrollDepth = 0; + }; + + /** + * Removes scroll event listener from container. + * Should be called when tracking is no longer needed. + */ + public cleanup(): void { + if ( this.container ) { + this.container.removeEventListener( 'scroll', this.handleScroll ); + } + } +} + +/** + * Calculates the maximum value between two numbers and clamps the result between 0 and 1. + * @param valueA - First number to compare + * @param valueB - Second number to compare + * @returns A number between 0 and 1 representing the maximum value between valueA and valueB + */ +function calcAndClampMaxValue( valueA: number, valueB: number ): number { + return Math.min( 1, Math.max( 0, valueA, valueB ) ); +} From 8112c63dca1b8419a88ffdf48de5d8d8097b8d57 Mon Sep 17 00:00:00 2001 From: Chris Holder Date: Wed, 8 Jan 2025 16:50:43 -0600 Subject: [PATCH 5/6] Add tests --- .../blocks/reader-full-post/scroll-tracker.ts | 6 +- .../reader-full-post/test/scroll-tracker.ts | 130 ++++++++++++++++++ 2 files changed, 132 insertions(+), 4 deletions(-) create mode 100644 client/blocks/reader-full-post/test/scroll-tracker.ts diff --git a/client/blocks/reader-full-post/scroll-tracker.ts b/client/blocks/reader-full-post/scroll-tracker.ts index 233fac07c2737..3f4f453ba0acb 100644 --- a/client/blocks/reader-full-post/scroll-tracker.ts +++ b/client/blocks/reader-full-post/scroll-tracker.ts @@ -26,10 +26,8 @@ export default class ScrollTracker { * @param container - The HTML element to track scrolling on, or null to stop tracking */ public setContainer( container: HTMLElement | null ): void { - if ( this.container ) { - this.container.removeEventListener( 'scroll', this.handleScroll ); - } - + this.cleanup(); + this.resetMaxScrollDepth(); this.container = container; if ( container ) { container.addEventListener( 'scroll', this.handleScroll ); diff --git a/client/blocks/reader-full-post/test/scroll-tracker.ts b/client/blocks/reader-full-post/test/scroll-tracker.ts new file mode 100644 index 0000000000000..4b2a4b60910d4 --- /dev/null +++ b/client/blocks/reader-full-post/test/scroll-tracker.ts @@ -0,0 +1,130 @@ +/** + * @jest-environment jsdom + */ +import ScrollTracker from '../scroll-tracker'; + +describe( 'ScrollTracker', () => { + let scrollTracker; + let container; + + beforeEach( () => { + scrollTracker = new ScrollTracker(); + container = document.createElement( 'div' ); + // Setup scrollable container + Object.defineProperties( container, { + scrollTop: { value: 0, configurable: true }, + scrollHeight: { value: 1000, configurable: true }, + clientHeight: { value: 500, configurable: true }, + } ); + } ); + + afterEach( () => { + scrollTracker.cleanup(); + } ); + + describe( 'getMaxScrollDepth()', () => { + it( 'should return 0 when no scrolling has occurred', () => { + expect( scrollTracker.getMaxScrollDepth() ).toBe( 0 ); + } ); + + it( 'should return correct depth when scrolled', () => { + scrollTracker.setContainer( container ); + Object.defineProperty( container, 'scrollTop', { value: 250 } ); + container.dispatchEvent( new Event( 'scroll' ) ); + expect( scrollTracker.getMaxScrollDepth() ).toBe( 0.5 ); + } ); + + it( 'should return the highest depth reached', () => { + scrollTracker.setContainer( container ); + + Object.defineProperty( container, 'scrollTop', { value: 375 } ); + container.dispatchEvent( new Event( 'scroll' ) ); + + Object.defineProperty( container, 'scrollTop', { value: 250 } ); + container.dispatchEvent( new Event( 'scroll' ) ); + + expect( scrollTracker.getMaxScrollDepth() ).toBe( 0.75 ); + } ); + + it( 'should reset when container changes', () => { + scrollTracker.setContainer( container ); + Object.defineProperty( container, 'scrollTop', { value: 250 } ); + container.dispatchEvent( new Event( 'scroll' ) ); + + const newContainer = document.createElement( 'div' ); + Object.defineProperties( newContainer, { + scrollTop: { value: 0, configurable: true }, + scrollHeight: { value: 1000, configurable: true }, + clientHeight: { value: 500, configurable: true }, + } ); + scrollTracker.setContainer( newContainer ); + + expect( scrollTracker.getMaxScrollDepth() ).toBe( 0 ); + } ); + } ); + + describe( 'getMaxScrollDepthAsPercentage()', () => { + it( 'should return 0 when no scrolling has occurred', () => { + expect( scrollTracker.getMaxScrollDepthAsPercentage() ).toBe( 0 ); + } ); + + it( 'should return the correct percentage when scrolled', () => { + scrollTracker.setContainer( container ); + Object.defineProperty( container, 'scrollTop', { value: 250 } ); + container.dispatchEvent( new Event( 'scroll' ) ); + expect( scrollTracker.getMaxScrollDepthAsPercentage() ).toBe( 50 ); + } ); + + it( 'should return the highest percentage reached', () => { + scrollTracker.setContainer( container ); + + Object.defineProperty( container, 'scrollTop', { value: 375 } ); + container.dispatchEvent( new Event( 'scroll' ) ); + + Object.defineProperty( container, 'scrollTop', { value: 250 } ); + container.dispatchEvent( new Event( 'scroll' ) ); + + expect( scrollTracker.getMaxScrollDepthAsPercentage() ).toBe( 75 ); + } ); + } ); + + describe( 'setContainer()', () => { + it( 'should track scroll events on the new container', () => { + scrollTracker.setContainer( container ); + Object.defineProperty( container, 'scrollTop', { value: 250 } ); + container.dispatchEvent( new Event( 'scroll' ) ); + expect( scrollTracker.getMaxScrollDepthAsPercentage() ).toBe( 50 ); + } ); + + it( 'should stop tracking previous container when setting new one', () => { + const oldContainer = document.createElement( 'div' ); + Object.defineProperties( oldContainer, { + scrollTop: { value: 0, configurable: true }, + scrollHeight: { value: 1000, configurable: true }, + clientHeight: { value: 500, configurable: true }, + } ); + + scrollTracker.setContainer( oldContainer ); + Object.defineProperty( oldContainer, 'scrollTop', { value: 250 } ); + oldContainer.dispatchEvent( new Event( 'scroll' ) ); + + scrollTracker.setContainer( container ); + Object.defineProperty( oldContainer, 'scrollTop', { value: 375 } ); + oldContainer.dispatchEvent( new Event( 'scroll' ) ); + + expect( scrollTracker.getMaxScrollDepthAsPercentage() ).toBe( 0 ); + } ); + } ); + + describe( 'cleanup()', () => { + it( 'should stop tracking scroll events', () => { + scrollTracker.setContainer( container ); + scrollTracker.cleanup(); + + Object.defineProperty( container, 'scrollTop', { value: 250 } ); + container.dispatchEvent( new Event( 'scroll' ) ); + + expect( scrollTracker.getMaxScrollDepthAsPercentage() ).toBe( 0 ); + } ); + } ); +} ); From 58f2a52f83e9954c741ad31ea5bc9dc36540c188 Mon Sep 17 00:00:00 2001 From: Chris Holder Date: Thu, 9 Jan 2025 11:42:27 -0600 Subject: [PATCH 6/6] Add back scroll reset --- client/blocks/reader-full-post/index.jsx | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/client/blocks/reader-full-post/index.jsx b/client/blocks/reader-full-post/index.jsx index 3afdc199f337c..6c9753db485b1 100644 --- a/client/blocks/reader-full-post/index.jsx +++ b/client/blocks/reader-full-post/index.jsx @@ -280,6 +280,11 @@ export class FullPostView extends Component { resetScroll = () => { this.clearResetScrollTimeout(); this.resetScrollTimeout = setTimeout( () => { + this.scrollableContainer.scrollTo( { + top: 0, + left: 0, + behavior: 'instant', + } ); this.scrollTracker.resetMaxScrollDepth(); }, 0 ); // Defer until after the DOM update };