Replacing unload events handlers to use pagehide by spenserhale · Pull Request #5056 · WordPress/wordpress-develop

spenserhale · 2023-08-24T11:41:30Z

Chrome has deprecated the 'unload' event (https://developer.chrome.com/blog/deprecating-unload/)

Replacing event handlers to use the 'pagehide' instead based on the suggestion from Chrome's documentation.

Trac ticket: https://core.trac.wordpress.org/ticket/55491

Commit Message

Administration: Remove deprecated unload event handlers and use pagehide (and pageshow) when appropriate.

Use pagehide event instead of unload in the following cases:

For classic editor to release the post lock.
In Text widget to rebuild editor after dragging widget to new location in classic widgets interface.
To clear out the window.name when navigating away from a post preview.
To suspend heartbeat, while also using pageshow event to resume as if it had been a focused tab in case page restored from bfcache.

Also:

Remove obsolete mobile cleanup code in js/_enqueues/lib/gallery.js (introduced in [9894]). Do same for src/js/_enqueues/wp/media/models.js (introduced in [22872]). See #22552.
Remove obsolete Firefox-specific workaround in js/_enqueues/wp/mce-view.js from [39282]. See #38511.

Fixes #55491.
Props spenserhale, westonruter, adamsilverstein, azaozz, shawfactor, peterwilsoncc, swissspidy.

Chrome has deprecated the unload event (https://developer.chrome.com/blog/deprecating-unload/) Fixes #55491

westonruter · 2023-08-24T14:30:56Z

src/js/_enqueues/lib/gallery.js

 });

-jQuery(window).on( 'unload', function () { window.tinymce = window.tinyMCE = window.wpgallery = null; } ); // Cleanup.
+jQuery(window).on( 'pagehide', function () { window.tinymce = window.tinyMCE = window.wpgallery = null; } ); // Cleanup.


I think we should look into removing this instance entirely. Otherwise, if a tab is restored then it would be broken. And it wouldn't be possible to restore these variables with pageshow since they are gone.

This change was introduced in r9894

Having a hard time finding where this file is even used in core.

It is enqueued in two functions: wp_media_upload_handler() and media_upload_gallery(). This latter function refers to the screen as the "legacy media uploader form".

OK, I found out how to get there:

Activate the Classic Editor plugin

Create a new post

Upload new photos in the post and create a gallery from them

Save the post

With the post ID in hand, go to /wp-admin/media-upload.php?type=gallery&tab=type&post_id={POST_ID}

It looks like this:

I don't see any other way to get to that screen.

Given this, either we:

Replace unload with pagehide and add a pageshow that does the reload behavior below, or

We just remove the line of cleanup code altogether. It seems like it was put in place for an iOS Safari bug from over a decade ago, similar to https://github.com/WordPress/wordpress-develop/pull/5056/files#r1332289674, so it's probably obsolete.

@azaozz Since you introduced this line of code and are probably the most familiar with this codebase, what do you think we should do (also considering this is an inaccessible legacy screen)?

See also how @spenserhale removed similar code in src/js/_enqueues/wp/mce-view.js: https://github.com/WordPress/wordpress-develop/pull/5056/files#r1332287807

I'm just going to go ahead and remove this for consistency with src/js/_enqueues/wp/mce-view.js.

src/js/_enqueues/wp/mce-view.js

src/js/_enqueues/wp/media/models.js

westonruter · 2023-08-24T14:52:04Z

src/js/_enqueues/wp/widgets/text.js


 					// When a widget is moved in the DOM the dynamically-created TinyMCE iframe will be destroyed and has to be re-built.
-					$( editor.getWin() ).on( 'unload', function() {
+					$( editor.getWin() ).on( 'pagehide', function() {


The use of the unload event may not be a problem here for bfcache since it is added to the iframe and not the window as a whole. Still, if pagehide works the same we could still use it.

Using unload is indeed a problem here. It disabled bfcache:

Removing the handler altogether results in a breakage:

Screen.recording.2023-09-20.17.09.09.webm

Nevertheless, using pagehide instead of unload does work:

Screen.recording.2023-09-20.17.09.40.webm

Nevertheless, bfcache remains disabled since TinyMCE is using unload. However, this doesn't seem like something we should remove per https://core.trac.wordpress.org/ticket/55491#comment:27

Chrome has deprecated the unload event (https://developer.chrome.com/blog/deprecating-unload/) Fixes #55491

reload function was to fix #38511, but Firefox issue is no longer reproducible For Ticket #55491

was to fix #22552, but the iOS issue is no longer reproducible For Ticket #55491

…cache

…into 55491-replace-unload-events

westonruter · 2023-09-20T23:44:09Z

src/js/_enqueues/wp/mce-view.js

 					}, 3000 );
 				}

-				function reload() {


Rationale for removal: https://core.trac.wordpress.org/ticket/55491#comment:15

Introduced in r39282 to fix Core-38511 in 2016. It is to work-around a Firefox-specific bug. We should validate whether it is still an issue in Firefox. If not, we can remove this code.

I tried reproducing the issue in Firefox according to the description of Core-38511 and I was unable.

I've reported this to the 7-year old ticket: https://core.trac.wordpress.org/ticket/38511#comment:27

@azaozz also please weigh in on whether we can remove this code.

Given the age of this bug fix, it should be fine to remove. If we discover that this still creates an issue for Firefox users, we can always restore the fix.

Yep, super old browser specific fix. LGTM to remove too.

westonruter · 2023-09-20T23:48:22Z

src/js/_enqueues/wp/media/models.js

-// Clean up. Prevents mobile browsers caching.
-$(window).on('unload', function(){
-	window.wp = null;
-});


Rationale for removal: https://core.trac.wordpress.org/ticket/55491#comment:17

However, if this depends on the server sending Cache-Control: no-store, then I'd like to further verify this.

@spenserhale What happens if no-store is not sent? Is there an issue then? I'd like to find a way to not have to send no-store in the admin so that we can benefit from bfcache, but if this depends on no-store then that would block enabling bfcache.

Origin is d2d01ad to fix Core-22552: "Empty wp on unload to work around caching in iOS Safari."

This commit is 11 years old. Most likely this is no longer an issue in Safari. If so, we can most likely delete this entire event handler.

+1 - fine to delete this super old bug fix/browser specific workaround. It is unlikely that the bug still exists, if it does we can always reintroduce a fix.

Yep, same as above. Super old browser specific fix that seems okay to remove

westonruter · 2023-09-21T00:35:34Z

src/wp-includes/functions.php


 		if ( window.addEventListener ) {
-			window.addEventListener( 'unload', function() { window.name = ''; }, false );
+			window.addEventListener( 'pagehide', function() { window.name = ''; } );


I was confused why the PageTransitionEvent being passed to this event handler has its persisted property set to false, both for the pagehide and the pageshow events. In looking at DevTools, I see:

So it is explained that the fact that window.open() was used to open the preview is another way that bfcache is disabled.

I think this is all good. I'm seeing that the window.name is being set to wp-preview-... when the tab is opened when clicking preview. When I navigate the preview window to another page, the window.name is set to an empty string. When I then re-preview a post I am opened in a new tab rather than the previous preview tab (which is now no longer a preview). If in that preview tab I go back to the previewed post, and then hit preview again, then it is that tab that gets updated with the latest preview content.

So I think we're good here.

westonruter · 2023-09-21T00:42:30Z

src/js/_enqueues/wp/heartbeat.js

+			$(window).on(
+				'pageshow.wp-heartbeat',
+				/**
+				 * Handles pageshow event.
+				 *
+				 * @param {jQuery.Event} event
+				 * @param {PageTransitionEvent} event.originalEvent
+				 */
+				function ( event ) {
+					if ( event.originalEvent.persisted ) {
+
+						// Continue connecting.
+						resume();
+					}
+				}
+			);


I'm not seeing this code firing.

OK, I am seeing it fire. Apparently I have a browser extension which is disabling bfcache, but if I try in an incognito window then event.originalEvent.persisted is true.

…into 55491-replace-unload-events

westonruter · 2023-10-02T21:12:01Z

src/js/_enqueues/admin/post.js

 			return __( 'The changes you made will be lost if you navigate away from this page.' );
 		}
-	}).on( 'unload.edit-post', function( event ) {
+	}).on( 'pagehide.edit-post', function( event ) {


I don't see a single reference to the jQuery-namespaced unload.edit-post event name in WPdirectory, so I don't see any back-compat concerns here.

Note that post lock is restored upon pageshow via heartbeat. See #5056 (comment)

westonruter · 2023-10-02T21:13:13Z

src/js/_enqueues/wp/heartbeat.js

 			}

-			$(window).on( 'unload.wp-heartbeat', function() {
+			$(window).on( 'pagehide.wp-heartbeat', function() {


As with pagehide.edit-post, I don't see a single reference to the jQuery-namespaced unload.wp-heartbeat event name in WPdirectory, so I don't see any back-compat concerns here.

westonruter · 2023-10-02T22:55:34Z

src/js/_enqueues/admin/post.js

+		 */
+		function ( event ) {
+			if ( event.originalEvent.persisted ) {
+				window.location.reload();


In practice this code will never fire because this code is loaded on the Classic Editor and TinyMCE disables bfcache due to an unload event handler.

Nevertheless, if that offending TinyMCE code is commented-out and Cache-Control: no-store is disabled (e.g. via plugin), then the experience is not great when you leave with a dirty editor state: you get the "Are you sure?" dialog when you leave and then when you go back, to get it again when it attempts to reload:

Screen.recording.2023-10-02.16.03.53.webm

In the end, this code is not needed because upon pageshow, heartbeat will resume and it will start sending its wp-refresh-post-lock requests.

So this pageshow event is removed in 329ebfb, and then heartbeat then connects immediately upon pageshow if restoring from bfcache in 00772ee.

Much better experience:

Screen.recording.2023-10-02.16.19.16.webm

…en restoring from bfcache

…s both of those

westonruter · 2023-10-09T15:52:34Z

src/js/_enqueues/wp/heartbeat.js

+					if ( event.originalEvent.persisted ) {
+						/*
+						 * When page navigation is stored via bfcache (Back/Forward Cache), consider this the same as
+						 * if the user had just switched to the tab since the behavior is similar.
+						 */
+						focused();
+					}


See outdated conversation: #5056 (comment)

westonruter · 2023-10-09T15:54:08Z

src/js/_enqueues/lib/gallery.js

-jQuery(window).on( 'unload', function () { window.tinymce = window.tinyMCE = window.wpgallery = null; } ); // Cleanup.
-


Removal rationale: https://github.com/WordPress/wordpress-develop/pull/5056/files#r1332287807

See also #5056 (comment)

azaozz · 2023-10-09T20:29:22Z

The changes from unload to pagehide seem pretty straightforward. Also the removal of super old browser specific bugfixes. On the other hand this touches an area where bugs may be harder to figure out, but thinking that if something goes wrong it would be detected and fixed during RC.

westonruter · 2023-10-09T21:30:17Z

Committed in r56809.

Replacing unload events handlers to use pagehide

f1ed0a7

Chrome has deprecated the unload event (https://developer.chrome.com/blog/deprecating-unload/) Fixes #55491

westonruter reviewed Aug 24, 2023

View reviewed changes

src/js/_enqueues/wp/mce-view.js Outdated Show resolved Hide resolved

westonruter reviewed Aug 24, 2023

View reviewed changes

src/js/_enqueues/wp/media/models.js Outdated Show resolved Hide resolved

westonruter reviewed Aug 24, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

spenserhale and others added 10 commits August 24, 2023 11:17

Replacing unload events handlers to use pagehide

0062603

Chrome has deprecated the unload event (https://developer.chrome.com/blog/deprecating-unload/) Fixes #55491

Add pageshow handler for heartbeat

bf40da1

MCE-View: Removing pagehide event listener

cfb32d0

reload function was to fix #38511, but Firefox issue is no longer reproducible For Ticket #55491

Remove obsolete unload handler from Sizzle library bundled with TinyMCE

ac4548c

Media/models.js Removing pagehide event listener

8699faa

was to fix #22552, but the iOS issue is no longer reproducible For Ticket #55491

Make sure post lock is re-established when editor is restored from bf…

a2bd1ff

…cache

Merge branch 'trunk' into 55491-replace-unload-events

b15537b

Reload page upon persisted pageshow on page enqueueing gallery.js

3f78076

Merge branch 'trunk' of https://github.com/WordPress/wordpress-develop …

2e72411

…into 55491-replace-unload-events

Revert patch to tinymce

1014f9d

westonruter reviewed Sep 20, 2023

View reviewed changes

westonruter reviewed Sep 21, 2023

View reviewed changes

Merge branch 'trunk' of https://github.com/WordPress/wordpress-develop …

1783a6e

…into 55491-replace-unload-events

westonruter reviewed Oct 2, 2023

View reviewed changes

westonruter added 4 commits October 2, 2023 16:11

Remove unnecessary pageshow.edit-post event handler to reload page wh…

329ebfb

…en restoring from bfcache

Connect heartbeat immediately when restoring from bfcache

00772ee

Use focused() function instead of resume and connectNow, since it doe…

cf5fc8d

…s both of those

Remove unused jQuery reference

fd9630b

westonruter requested a review from azaozz October 3, 2023 00:22

westonruter approved these changes Oct 3, 2023

View reviewed changes

westonruter added 2 commits October 9, 2023 08:39

Remove (likely) obsolete cleanup code

e591b6c

Also remove pageshow event after removal of pagehide

e6dcef0

westonruter requested a review from adamsilverstein October 9, 2023 15:41

westonruter reviewed Oct 9, 2023

View reviewed changes

azaozz approved these changes Oct 9, 2023

View reviewed changes

westonruter closed this Oct 9, 2023

		jQuery(window).on( 'unload', function () { window.tinymce = window.tinyMCE = window.wpgallery = null; } ); // Cleanup.

Conversation

spenserhale commented Aug 24, 2023 • edited by westonruter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Commit Message

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azaozz commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

spenserhale commented Aug 24, 2023 •

edited by westonruter

Loading

westonruter Sep 21, 2023 •

edited

Loading

azaozz commented Oct 9, 2023 •

edited

Loading