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

Report scroll metrics when page is resized #2399

Merged
merged 23 commits into from Sep 19, 2023

Conversation

hamza-kadiri
Copy link
Contributor

@hamza-kadiri hamza-kadiri commented Aug 30, 2023

Motivation

We currently have an issue in scrollmaps, as you can see in this widget, many users reach a scroll depth of 100% when the page scroll height is way lower than what it should be.

This is because when the user doesn't scroll, the scroll metrics are only reported once: at loading time.

Since loading time can be reached before the page is fully loaded, it means that we collect wrong scroll metrics

This PR aims to fix this issue by decorrelating scroll height reporting from scroll depth reporting.

Changes

  • Use a ResizeObserver to report scroll height increase

Testing

Visit this page with the dev version of the sdk enabled and don't scroll. Clear the search, you should notice that the scroll height has increased

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2023

Codecov Report

Merging #2399 (293a046) into main (a639866) will decrease coverage by 0.04%.
Report is 2 commits behind head on main.
The diff coverage is 94.28%.

@@            Coverage Diff             @@
##             main    #2399      +/-   ##
==========================================
- Coverage   94.01%   93.98%   -0.04%     
==========================================
  Files         219      219              
  Lines        6299     6317      +18     
  Branches     1402     1408       +6     
==========================================
+ Hits         5922     5937      +15     
- Misses        377      380       +3     
Files Changed Coverage Δ
...lection/view/viewMetrics/trackCommonViewMetrics.ts 100.00% <ø> (ø)
...sCollection/view/viewMetrics/trackScrollMetrics.ts 95.23% <94.28%> (-4.77%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hamza-kadiri hamza-kadiri force-pushed the hamza/report-scroll-metrics-periodically branch from 5da6559 to a1479d4 Compare September 1, 2023 15:50
@hamza-kadiri hamza-kadiri changed the title before first scroll, report scroll metrics periodically report scroll metrics periodically when page is resized Sep 7, 2023
@hamza-kadiri hamza-kadiri changed the title report scroll metrics periodically when page is resized report scroll metrics when page is resized Sep 7, 2023
@hamza-kadiri hamza-kadiri changed the title report scroll metrics when page is resized Report scroll metrics when page is resized Sep 13, 2023
@hamza-kadiri hamza-kadiri marked this pull request as ready for review September 13, 2023 13:31
@hamza-kadiri hamza-kadiri requested a review from a team as a code owner September 13, 2023 13:31
observable.notify(computeScrollValues())
}

if (window.ResizeObserver) {
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ question: ‏what about IE11? ResizeObserver is not supported there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For IE11, the fallback solution is to collect data on the scrolling at regular intervals:

const id = setInterval(notify, throttleDuration)
return () => clearInterval(id)

Copy link
Contributor

Choose a reason for hiding this comment

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

oh right, so fallback to setInterval for IE 11 🤔
Do you see any pros / cons to have those two implementations rather than just always using setInterval?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • cons:
    • we have two different implementations, which can be troublesome
  • pros:
    • even if we have two implementations, the two still produce the same values, the only difference lies in the frequency with which these values are produced. For IE we're going to produce new values every second, for other browsers, we're going to produce them only when the page is scrolled or resized
    • on modern browser, since we produce values less often, we have less changes of causing layout shifts/reflows caused by scroll/viewport calculation

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, the resize observer approach seems more "polite" than recomputing the values every second, I am wondering if it is not even too agressive for IE 😕
It could worth to double check that it does not degrade performances there.

Did you look into approaches taken by some resize observer polyfills?
In case there is some interesting alternate approach that could be used everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have a way to test the SDK on IE?

I can definitely try an approach with a resize observer polyfill.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a way to test the SDK on IE?

Yes, with browserstack

@hamza-kadiri hamza-kadiri merged commit 5d5f9e1 into main Sep 19, 2023
17 checks passed
@hamza-kadiri hamza-kadiri deleted the hamza/report-scroll-metrics-periodically branch September 19, 2023 08:31
@BenoitZugmeyer BenoitZugmeyer mentioned this pull request Sep 25, 2023
4 tasks
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.

None yet

5 participants