@peterhashair opened this Pull Request on April 3rd 2022 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

@peterhashair commented on April 4th 2022 Contributor

build js

@peterhashair commented on April 12th 2022 Contributor

build js

@peterhashair commented on April 12th 2022 Contributor

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

@justinvelluppillai commented on April 13th 2022 Contributor

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

@bx80 commented on April 25th 2022 Contributor

@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 commented on April 25th 2022 Contributor

@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 commented on April 26th 2022 Contributor

@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 commented on April 26th 2022 Contributor

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

@peterhashair commented on April 26th 2022 Contributor

@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 commented on April 26th 2022 Contributor

@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 commented on April 26th 2022 Contributor

@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 commented on May 2nd 2022 Contributor

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

This Pull Request was closed on May 2nd 2022
Powered by GitHub Issue Mirror