SAP / openui5

OpenUI5 lets you build enterprise-ready web applications, responsive to all devices, running on almost any browser of your choice.
http://openui5.org
Apache License 2.0
2.96k stars 1.24k forks source link

Window unload event should never be used #3085

Open mauriciolauffer opened 3 years ago

mauriciolauffer commented 3 years ago

OpenUI5 version: all versions

Browser/version (+device/version): all browsers

URL (minimal example if possible): https://developers.google.com/web/updates/2018/07/page-lifecycle-api?utm_source=lighthouse&utm_medium=devtools#legacy-lifecycle-apis-to-avoid

What is the expected result? Window unload event should be triggered.

What happens instead? Window unload event is extremely unreliable, it doesn't fire in multiple scenarios, especially in mobiles. It also makes bfcache (Back/forward cache) impossible.

Google has a great explanation about the topic: https://developers.google.com/web/updates/2018/07/page-lifecycle-api?utm_source=lighthouse&utm_medium=devtools#legacy-lifecycle-apis-to-avoid https://web.dev/bfcache/ https://developer.mozilla.org/en-US/docs/Web/API/Window/unload_event

Also, Lighthouse complains about using unload event, rule Registers an unload listener: image

pksinsik commented 3 years ago

Hi @mauriciolauffer ,

Thank you for sharing this finding. Just to get your point: You say

What is the expected result? Window unload event should be triggered.

but you mean "UI5 should not use the window.unload event" due to its unreliability as outlined in the resources you have linked?

Regards, Patric

pksinsik commented 3 years ago

assuming "UI5 should not use the window.unload event", I've created an internal incident 2080398448. The status of the issue will be updated here in GitHub.

Regards, Patric

petermuessig commented 3 years ago

This is indeed interesting here. Thanks @mauriciolauffer!

Especially reading the following page https://web.dev/bfcache/, the section "Optimize your page for bfcache". AFAICS, the most critical part is the ResizeHandler which should better use the pagehide/pageshow events instead of the unload event. But in some cases, the usage of the beforeunload and the unload event might still be needed - especially if the application decides to use it explicitly. And the forwarding of the native API in the Component wasn't a good idea in the past - here could also have let the apps using native event api. The DebugEnv and the Support I see uncritical so the developers of the ResizeHandler may look into your suggestion which would benefit the most from it as this is definitely used when apps are running.

Thanks again and best regards, Peter

codeworrior commented 3 years ago

@petermuessig would you then suggest to deprecate the Component hooks, rather than switching the implementation? I tried to understand how compatible the pagehide and the unload events are, but didn't come to a conclusion yet.

petermuessig commented 3 years ago

Yes, exactly. That was what first came into my mind. Better leave the decision to the app than being the bad framework guy. In most cases the Components even do not use this event and then it will also not being registered.

mauriciolauffer commented 3 years ago

@petermuessig indeed, beforeunload may be needed and has a valid scenario, that's why my PR https://github.com/SAP/openui5/pull/3086 only changes unload events.

petermuessig commented 3 years ago

Thanks, @mauriciolauffer , we will follow up on your suggestion. 😃

flovogt commented 3 years ago

Hi @mauriciolauffer , thanks a lot for your suggestion. We'll cover this topic in BLI CPOUI5FRAMEWORK-203.

mauriciolauffer commented 1 year ago

@petermuessig it seems the time to get rid of it has come 😜 https://github.com/SAP/openui5/pull/3086

petermuessig commented 1 year ago

Hi @mauriciolauffer - yes indeed - we are discussing this just the last couple of days because of the Google announcement. AFAIK, in several internal places the usage of the unload event has been removed. There is only one API in the Component which allows to hook into this event - but hopefully rarely used by apps - and we will deprecate this API but most probably have to keep it functional with the "pagehide" event (once the "unload" doesn't work anymore). Anyhow, the Page Lifecycle API is IMO the way to go and the better option to hook into the browsers' lifecycle. An app put into the bfcache and then navigating back means to recreate the state of the app. Yes - we are on it... Honestly, used 3rd party is the bigger problem in this case... 😉