Reader: fix scroll depth tracking #98093

holdercp · 2025-01-08T16:53:12Z

Fixes Automattic/loop#300

Proposed Changes

Fixes large values for scroll depth in future Tracks events, keeping them as a percentage between 0 and 100
Tracks scroll depth on mobile and desktop viewports
Tracks scroll depth on both stream and two-column views
Separates the scroll tracking logic into its own class that's composed into the React component. This removes the need to component state, reducing re-renders drastically.

Why are these changes being made?

Some of the values in Tracks were above 100, indicating there was a bug in the calculation.
The events weren't being sent to Tracks depending on how the component was rendered, specifically in a mobile viewport. Now the events are sent in every rendering of the full post component.
Separating out the scroll tracking from the component rendering meant we don't need to re-render the component whenever a scroll value changed, improving performance.

Testing Instructions

Go to /read and navigate between full post views
Make sure to test both the stream and two-column layout, as well as mobile and desktop
When you navigate away from the post, you should see the track event calypso_reader_article_scroll_depth logged
If the scroll depth doesn't go past 90% then a track event calypso_reader_article_exit_before_completion should also be logged.
You should be able to see these events on tracks after 5 mins
To see them live, I suggest adding console.log to the recordTrackForPost method.
Make sure all track values represent a percentage (0-100); they should be whole numbers.

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
- For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2025-01-08T16:53:26Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-129379

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-129379&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-129379&env=a8c-for-agencies

matticbot · 2025-01-08T17:01:37Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Async-loaded Components (~108 bytes added 📈 [gzipped])

name                                        parsed_size           gzip_size
async-load-calypso-blocks-reader-full-post       +364 B  (+0.9%)     +108 B  (+0.9%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

matticbot · 2025-01-08T17:03:25Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

notifications
wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug fix/reader-tracks-scroll-depth on your sandbox.

holdercp · 2025-01-08T23:04:42Z

client/blocks/reader-full-post/index.jsx

-		maxScrollDepth: 0, // Track the maximum scroll depth achieved
-		hasCompleted: false, // Track whether the user completed the post


No longer need this component state because we just need to know the max scroll depth at the moment when we unmount or the post/feed changes. All of that tracking logic was moved to scroll-tracker.ts and just uses event listeners.

holdercp · 2025-01-08T23:07:53Z

client/blocks/reader-full-post/index.jsx

-			this.scrollableContainer.removeEventListener( 'scroll', this.setScrollDepth );
-		}
+		// Track scroll depth and remove related instruments
+		this.trackScrollDepth( this.props.post );


We need to track the scroll depth when the component is about to unmount. We were missing events when the feed would change or a new page in the middle column was requested because the component actually unmounts/remounts in those cases.

holdercp · 2025-01-08T23:10:08Z

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.


These css changes fix an issue where the scroll container changed based on the viewport width, meaning we were no longer getting events. This ensures that the same DOM element is the scrolling container in the Recent feed.

meaning we were no longer getting events

I understand the motivation, but looks like this had a side-effect. In the reader recent tab, if you scroll down while reading a longish post and click on another post, the scroll position doesn't reset to the top of the post.

We fixed it here should you want to see the before and after videos, but I suspect this change makes it come back since it doesn't happen on trunk.

spsiddarthan

Appears to work well, I left a comment about the CSS changes.

Please let me know if it doesn't make sense.

spsiddarthan · 2025-01-09T05:38:09Z

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.


meaning we were no longer getting events

I understand the motivation, but looks like this had a side-effect. In the reader recent tab, if you scroll down while reading a longish post and click on another post, the scroll position doesn't reset to the top of the post.

We fixed it here should you want to see the before and after videos, but I suspect this change makes it come back since it doesn't happen on trunk.

holdercp · 2025-01-09T18:02:03Z

@spsiddarthan Thank you for that catch! I unintentionally removed that code. Should be fixed now.

spsiddarthan

Works well, thanks for your work on this!

Track scroll depth on unmount of full post

8a0b913

holdercp self-assigned this Jan 8, 2025

holdercp added 4 commits January 8, 2025 13:19

Simplify scroll depth calculation

2a6fded

Keep scrolling container consistent between mobile and desktop

8f9f5a0

Separate tracking code from component code and compose together

2a2f8de

Add tests

8112c63

holdercp marked this pull request as ready for review January 8, 2025 23:00

holdercp requested a review from a team as a code owner January 8, 2025 23:00

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 8, 2025

holdercp commented Jan 8, 2025

View reviewed changes

spsiddarthan reviewed Jan 9, 2025

View reviewed changes

Add back scroll reset

58f2a52

holdercp requested review from spsiddarthan and eoigal January 9, 2025 18:02

spsiddarthan approved these changes Jan 10, 2025

View reviewed changes

holdercp merged commit 64d3372 into trunk Jan 10, 2025
14 checks passed

holdercp deleted the fix/reader-tracks-scroll-depth branch January 10, 2025 14:28

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reader: fix scroll depth tracking #98093

Reader: fix scroll depth tracking #98093

holdercp commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited by matticbot

Loading

matticbot commented Jan 8, 2025 •

edited

Loading

matticbot commented Jan 8, 2025 •

edited

Loading

holdercp Jan 8, 2025 •

edited

Loading

holdercp Jan 8, 2025 •

edited

Loading

holdercp Jan 8, 2025

spsiddarthan Jan 9, 2025

spsiddarthan left a comment

spsiddarthan Jan 9, 2025

holdercp commented Jan 9, 2025

spsiddarthan left a comment

		maxScrollDepth: 0, // Track the maximum scroll depth achieved
		hasCompleted: false, // Track whether the user completed the post

Reader: fix scroll depth tracking #98093

Reader: fix scroll depth tracking #98093

Conversation

holdercp commented Jan 8, 2025 • edited Loading

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented Jan 8, 2025 • edited by matticbot Loading

matticbot commented Jan 8, 2025 • edited Loading

matticbot commented Jan 8, 2025 • edited Loading

holdercp Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

holdercp Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

holdercp Jan 8, 2025

Choose a reason for hiding this comment

spsiddarthan Jan 9, 2025

Choose a reason for hiding this comment

spsiddarthan left a comment

Choose a reason for hiding this comment

spsiddarthan Jan 9, 2025

Choose a reason for hiding this comment

holdercp commented Jan 9, 2025

spsiddarthan left a comment

Choose a reason for hiding this comment

holdercp commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited by matticbot

Loading

matticbot commented Jan 8, 2025 •

edited

Loading

matticbot commented Jan 8, 2025 •

edited

Loading

holdercp Jan 8, 2025 •

edited

Loading

holdercp Jan 8, 2025 •

edited

Loading