Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Average Scroll Depth Metric: extracted tracker changes #4826

Merged
merged 17 commits into from
Nov 21, 2024

Conversation

RobertJoonas
Copy link
Contributor

@RobertJoonas RobertJoonas commented Nov 14, 2024

Changes

This PR extracts the tracker changes from #4791, and addresses tracker related feedback as well.

Tests

  • Automated tests have been added

Changelog

  • This PR does not make a user-facing change

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

@RobertJoonas RobertJoonas changed the title Extracted tracker changes from Scroll Depth PR Extracted tracker changes from #4791 Nov 15, 2024
@RobertJoonas RobertJoonas changed the title Extracted tracker changes from #4791 Average Scroll Depth Metric: extracted tracker changes Nov 15, 2024
// also accounted for.
var count = 0
var interval = setInterval(function () {
currentDocumentHeight = getDocumentHeight()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should also update maxScrollDepthPx here as well to account for any weird reflows.

Copy link
Contributor Author

@RobertJoonas RobertJoonas Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, but it's tricky to optimise our script for this - a reflow might both increase and decrease maxScrollDepthPx (meaning that we would have to keep re-initialising it), but the user can already scroll during those three seconds, so that information might get lost when we blindly reset it.

I would leave it be for now. In worst case, the maxScrollDepthPx will be smaller than it really is, resulting in a lower percentage. But even the very first viewport height after DOMContentLoaded should be a sensible initial value for maxScrollDepthPx. If it turns out to be an issue, we can come back to it later.

await page.evaluate(() => window.scrollBy(0, document.body.scrollHeight))

// Wait until documentHeight gets increased by the fixture JS
await page.waitForSelector('#more-content')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: It might have been simpler to evaluate JS on the page appending a div/img rather than rely on timers which might be slightly finnicky.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good note! The timeout in the test fixture was intended only for debugging. I've removed it now. It now actually works by simply appending a 2000px div to the document.

Copy link
Contributor

@macobo macobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small notes but 👍

@RobertJoonas RobertJoonas added this pull request to the merge queue Nov 21, 2024
Merged via the queue into master with commit 2a7d02b Nov 21, 2024
11 checks passed
@RobertJoonas RobertJoonas deleted the scroll-depth-tracking branch November 21, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants