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

Jump to bottom

Ensure JS tracker unload event is triggered in edge cases to make sure tracking requests are sent #18810

Closed
tsteur opened this issue Feb 17, 2022 · 9 comments · Fixed by #19041
Closed

Ensure JS tracker unload event is triggered in edge cases to make sure tracking requests are sent #18810

tsteur opened this issue Feb 17, 2022 · 9 comments · Fixed by #19041
Assignees
@peterhashair
Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Milestone
4.10.0

Comments

@tsteur
Copy link
Member

tsteur commented Feb 17, 2022

While testing #18135 I noticed that the beforeunload event isn't triggered when forcing a page reload. Was then reading https://developer.mozilla.org/en-US/docs/Web/API/Navigator/sendBeacon .

Web sites often want to send analytics or diagnostics to the server when the user has finished with the page. The most reliable way to do this is to send the data on the visibilitychange event

It also explains why you should use this event instead of unload. It basically may not be triggered in various cases (not sure if it's also the case for beforeunload, to be investigated).

This could result in few queued requests not being sent and therefore some data may not be tracked. Especially premium features leverage the unload event and queued tracking to send a tracking request (eg for Media Analytics to track how much of a media has been viewed when leaving).

Something like:

        addEventListener(windowAlias, 'visibilitychange', function () {
  if (document.visibilityState === 'hidden') {
          executePluginMethod('unload');
}
        }, false);

Seems this is supported on most browsers: https://caniuse.com/?search=visibilitychange Maybe as a fallback we would still also listen to beforeunload in case this event isn't triggered so it still works on older browsers too maybe? Might not be needed depending on browser compatibility.

We would also need to make sure that this won't cause any regressions by listening to this new event. Also if someone switches a lot between tabs and the page becomes visible and hidden a lot, then we wouldn't want to start sending heaps of tracking requests.

Be good if someone could look into this. The goal of the issue would be to make sending tracking requests more reliable when a user leaves a site.
@tsteur tsteur added the Bug For errors / faults / flaws / inconsistencies etc. label Feb 17, 2022
@tsteur tsteur added this to the 4.9.0 milestone Feb 17, 2022
@tsteur tsteur added the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Mar 2, 2022
@peterhashair peterhashair self-assigned this Mar 31, 2022
@peterhashair
Copy link
Contributor

tried visibilitychange, but I notice it will trigger when the user switching between tabs (the tab page is not visible)

@peterhashair peterhashair mentioned this issue Apr 3, 2022
Merged
11 tasks
@justinvelluppillai justinvelluppillai modified the milestones: 4.9.0, 4.10.0 Apr 12, 2022
@peterhashair
Copy link
Contributor

@tsteur, @bx80, and I found the visibilitychange will cause a problem when you switch between browser tabs since there is no way to talk user is switching between tabs or left page. I think we can change beforeunload to onbeforeunload that will trigger unload on refresh, but that won't work for a mobile use case. The user switches to a different app, Later, the user closes the browser from the app manager.

Do lots of research, there is not really a good way to detect a user left browser other than beforeunload or onbeforeunload events. Any suggestion?

@tsteur
Copy link
Member Author

tsteur commented Apr 29, 2022

I'm not sure I fully understand it but I'd listen to both visibilitychange and beforeunload. Mentioned this also in the issue description

Maybe as a fallback we would still also listen to beforeunload in case this event isn't triggered so it still works on older browsers too maybe

Would that solve the issue?

@peterhashair
Copy link
Contributor

@tsteur ah I think I confused you, but the problem is visibilitychange triggers on browser tab change, there is no way to tell the difference between tab change and completely leaving the browser, I think that causes a problem when people open a page ->switch tab-> plugin performance unload->come back to the page.

@tsteur
Copy link
Member Author

tsteur commented Apr 29, 2022

Yeah sorry I might not be getting it yet. It would be a good thing if they switch to a different tab and then we trigger the unload event I think, or could there be an issue? The biggest thing I could think of is that if people switch between tabs a lot then it could cause maybe a lot of requests which we would need to prevent maybe. I can see though it changes the meaning of "unload". Maybe we'd have to do that trade-off and just make sure we're not triggering the unload on visibility change too often but only about every 30seconds or so. Not sure if it makes sense.

@peterhashair
Copy link
Contributor

@tsteur right, that makes sense, I think a debounce maybe helps on tab change, but not perfect, will keep playing around.

@bx80
Copy link
Contributor

bx80 commented Apr 29, 2022

@tsteur I was thinking that if we trigger the unload event on a tab switch then plugin code would need to be aware that the user might not have left the page and could switch back to it at any point. There might be an assumption that the plugin unload event means that the page has been closed (which up until now would have been true).

I haven't looked at it in detail, but HSR for example hooks the unload / beforeunload events and appears to stopSendingData() and disables the videoPlayerInterval which probably shouldn't happen for a tab switch?

@sgiehl
Copy link
Member

sgiehl commented Apr 29, 2022

Just a random thought: Isn't the main problem that some tracking requests might not be sent if the unload event isn't triggered and the page is closed?
Wouldn't it be enough to simply send all requests in the queue when the visibility changes?
That for sure might not solve the problem that the unload event isn't triggered, but might already prevent losing requests.

@tsteur
Copy link
Member Author

tsteur commented May 1, 2022

@sgiehl that would be already some good step towards the goal 👍 . The tracking plugins will still need to also register the tracking requests they have "queued" when visibility is changing to hidden so these will be sent as well. Otherwise they might not be tracked on mobile for example.

@bx80 maybe in heatmaps ending the recording is not actually needed. I'm not actually sure why it was done since if the page unloads then we wouldn't need to end the recording anyway. And if the page is not unloaded then we still want to have the recording active. So in either case it might be not needed (unless I miss something which is possible)
image
Heatmaps could potentially startSendingData again if the window gets visibility again (tracking.startSendingData();).

@justinvelluppillai justinvelluppillai changed the title JS tracker: Unload event may not be triggered in some cases meaning some tracking requests may not be sent. Ensure JS tracker unload event is triggered in edge cases to make sure tracking requests are sent May 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@peterhashair peterhashair

Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Milestone
4.10.0
Development

Successfully merging a pull request may close this issue.

[JS]update beforeunload to visibilitychange matomo-org/matomo
5 participants
@tsteur @justinvelluppillai @sgiehl @peterhashair @bx80