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

[JS]update beforeunload to visibilitychange #19041

Merged
merged 25 commits into from May 2, 2022
Merged

[JS]update beforeunload to visibilitychange #19041

merged 25 commits into from May 2, 2022

Conversation

peterhashair
Copy link
Contributor

Description:

Fixes: #18810

update beforeunload to visibilitychange

Please include a description of this change and which issue it fixes. If no issue exists yet please include context and what problem it solves.

Review

Peter added 2 commits April 4, 2022 11:44
Update piwik.js
4ac2897 
update beforeunload to visibilitychange
Update piwik.js
a970fad 
update condition
@peterhashair peterhashair added this to the 4.9.0 milestone Apr 3, 2022
@peterhashair peterhashair added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Apr 3, 2022
peterhashair and others added 2 commits April 4, 2022 01:18
@peterhashair
rebuilt piwik.js
4ccd2ef
Update piwik.js
e3efa27 
take timer outside of check
@peterhashair
Copy link
Contributor Author

build js

sgiehl
sgiehl reviewed Apr 4, 2022
View reviewed changes
js/piwik.js Outdated Show resolved Hide resolved
@peterhashair peterhashair added the Needs Review PRs that need a code review label Apr 7, 2022
Peter and others added 5 commits April 7, 2022 13:29
justinvelluppillai
justinvelluppillai reviewed Apr 12, 2022
View reviewed changes
js/piwik.js Outdated Show resolved Hide resolved
@justinvelluppillai justinvelluppillai modified the milestones: 4.9.0, 4.10.0 Apr 12, 2022
@justinvelluppillai
Update js/piwik.js
98408f9 
Co-authored-by: Justin Velluppillai <justin@innocraft.com>
@peterhashair
Copy link
Contributor Author

build js

justinvelluppillai
justinvelluppillai suggested changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment •
edited

Choose a reason for hiding this comment

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

This looks like the right approach to me @peterhashair, but there is one failing test because the unload handler takes longer now:

Test failed in module request: 'Internal timers and setLinkTrackingTimer()'

Error: beforeUnloadHandler(): 522 was greater than 520

Also can any test be added eg beforeUnloadHandler({ type: 'visibilitychange' }) ?

@peterhashair
Copy link
Contributor Author

@justinvelluppillai will fix that one, you are 100% correct, test changed to visibilitychange

@justinvelluppillai
Copy link
Contributor

I don't see the changes yet so will remove the needs review label until they're done @peterhashair 👍

@justinvelluppillai justinvelluppillai removed the Needs Review PRs that need a code review label Apr 13, 2022
Peter added 2 commits April 14, 2022 13:21
update test to 530
9ef7e26 
update test to 530
Merge branch '4.x-dev' into m18810
7160e2b 
# Conflicts:
#	tests/javascript/index.php
@peterhashair peterhashair added the Needs Review PRs that need a code review label Apr 18, 2022
bx80
bx80 suggested changes Apr 22, 2022
View reviewed changes
Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

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

It looks like the most recent merge of 4.x into this branch has overwritten the commit that updated the test time from 520 to 530.

Peter Zhang and others added 2 commits April 26, 2022 08:06
@justinvelluppillai
Update tests/javascript/index.php
e9b90f7 
Co-authored-by: Justin Velluppillai <justin@innocraft.com>
Merge branch '4.x-dev' into m18810
b6ab9cb
@bx80
Copy link
Contributor

bx80 commented Apr 25, 2022

@peterhashair I'm seeing beforeUnloadHandler calling executePluginMethod('unload') once when initially switching tabs on a tracked page. Is this intentional?

My test was to add a console.log(event); statement to the first line of beforeUnloadHandler and console.log('executePluginMethod'); after the isPageUnloading check. Then load a tracked page and switch tabs. The first time the tab is switched executePluginMethod is called, subsequent tab switches do not result in additional calls.

@peterhashair
Copy link
Contributor Author

@bx80 yes, I found the same problem, mention on the issue itself as well,visibilitychange event is a kind of design for mobile, triggers on changing tab, etc, but I guess that's expected.

@bx80
Copy link
Contributor

bx80 commented Apr 26, 2022

@peterhashair I suppose existing plugin code would need to be aware that executePluginMethod('unload') could now be called on a tab switch and not just on page unload. This might be a breaking change if code is relying on a single unload event being fired only when a page is closed.

For example, it looks like HSR will stop sending data the first time that the plugin unload event is called, so in a scenario where the user loaded the page, quickly switched to another tab to check mail and then switched back, then I suspect none of the subsequent user interactions would be recorded.

@peterhashair
Copy link
Contributor Author

@bx80 makes sense, let me see if I can filter out the tab switch event.

disable unload if tab is switched
18c0c38 
disable unload if tab is switched
@peterhashair
Copy link
Contributor Author

@bx80 I am trying to do some research seems like that even can't escape tab change. Maybe that event shouldn't be added, any thought?

@bx80
Copy link
Contributor

bx80 commented Apr 26, 2022

@peterhashair I took a look too and it doesn't look like the visibility API offers any way to differentiate between a tab switch and leaving the page, so we probably don't want to have this event trigger the plugin unload event and potentially break existing plugins.

@peterhashair
Copy link
Contributor Author

peterhashair commented Apr 26, 2022 •
edited

@bx80 agree, but I notice, the visibiltyChange actually useful when is mobile. beforeunload won't trigger for those.

  • A mobile user visits your page.
  • The user then switches to a different app.
  • Later, the user closes the browser from the app manager.

@peterhashair peterhashair removed the Needs Review PRs that need a code review label Apr 26, 2022
@peterhashair
Copy link
Contributor Author

@bx80 just revert some changes, am I on the right track?

@peterhashair peterhashair requested a review from bx80 May 2, 2022 21:30
@peterhashair peterhashair added the Needs Review PRs that need a code review label May 2, 2022
bx80
bx80 approved these changes May 2, 2022
View reviewed changes
Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

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

Looks good to me, both events are being handled and the plugin load event is only being called once on leaving the page. The first tab switch is still triggering the unload event, but as discussed this should be ok 👍

@peterhashair peterhashair merged commit b9043de into 4.x-dev May 2, 2022
@peterhashair peterhashair deleted the m18810 branch May 2, 2022 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinvelluppillai justinvelluppillai justinvelluppillai requested changes

@sgiehl sgiehl sgiehl left review comments

@bx80 bx80 bx80 approved these changes

Assignees
No one assigned
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.10.0
Development

Successfully merging this pull request may close these issues.

Ensure JS tracker unload event is triggered in edge cases to make sure tracking requests are sent
4 participants
@peterhashair @justinvelluppillai @bx80 @sgiehl