#55491 closed defect (bug) (fixed)
Replace `unload` event handlers from core
Reported by: | shawfactor | Owned by: | westonruter |
---|---|---|---|
Milestone: | 6.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Administration | Keywords: | has-patch |
Focuses: | javascript, performance | Cc: |
Description (last modified by )
Right now the heartbeat javascript library uses an unload handler, this means that where ever it is used the page is not elgible for the bfcache in many browsers.
Wordpress could get a significant performance improvement if this listener was replaced by using the page visibility api or a combination of pagehide handlers. Which i assume are possible in jquery
Added Aug 10. by Adam Silverstein:
Chrome has deprecated the unload
event (https://developer.chrome.com/blog/deprecating-unload/) so I expanded this ticket to include replacing/updating all uses of the unload
event in core.
Change History (48)
#1
@
3 years ago
- Component changed from General to Administration
- Focuses javascript performance added
- Version 5.9 deleted
#2
@
3 years ago
Even if 97% is not enough I'd assume that it is possible to test support and fallback to unload where appropriate. Note my javascript is not up too the task of fixing this myself unfortunately.
Ideally the whole api would be rewritten without a jquery dependency, but that should probably be kept as a separate issue.
#3
@
16 months ago
Just following this up, noting that there have been some recent and prospective developments in the Chrome browser which increase the importance of this:
https://web.dev/yahoo-japan-news-bfcache/
Namely
permission-policy: unload and a potential idea to remove the event entirely
#4
@
15 months ago
- Milestone changed from Awaiting Review to Future Release
This ticket was discussed in the recent Performance bug scrub.
Thanks @shawfactor for opening the ticket. great suggestion.
#5
@
15 months ago
The media modal and some TinyMCE code also seem to be affected by this. Some holistic bfcache testing might be worthwhile.
#8
@
15 months ago
Chrome recently announced they are deprecating the 'unload' event (https://developer.chrome.com/blog/deprecating-unload/) so this change is a bit more urgent.
Looks like we have five use instances in core that need updating. The article linked above offers suggested replacement approaches.
@shawfactor thanks for opening this ticket! I am going to expand it slightly to remove all instances of unload
from core rather than creating a new ticket.
#9
@
15 months ago
- Description modified (diff)
- Summary changed from Replace unload from heartbeat js to Replace `unload` event handlers from core
This ticket was mentioned in PR #5056 on WordPress/wordpress-develop by @spenserhale.
15 months ago
#11
- Keywords has-patch added
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.
@westonruter commented on PR #5056:
15 months ago
#12
The heartbeat instance we also need to account for:
@westonruter commented on PR #5056:
15 months ago
#13
#14
@
15 months ago
Note: Even with the elimination of unload
event handlers, this may still not enable the use of bfcache because admin pages are served with Cache-Control: no-store
. The Lighthouse audit notes this as being a "not actionable" failure type. The use of no-store
can be found in wp_get_nocache_headers()
and notably no-store, private
was just added for logged-in users as of WordPress 6.3, specifically in r55968 to fix #21938/#57627:
The intention behind this change is to prevent sensitive data in responses for logged in users being cached and available to others, for example via the browser history after the user logs out.
Therefore, it seems actually that the disabling of bfcache might be considered a new security/privacy feature. Eliminating unload
handlers unfortunately will yield no performance improvement, unfortunately. However, since unload
is being deprecated it is still good to remove its use and also to remove noise when doing audits.
Nevertheless, I think we should consider revisiting #21938 to remove no-store
by default because its presence disables bfcache for all logged-in page loads, even on the frontend. The introduction of no-store
didn't cause a performance regression in #21938 because bfcache was already disabled due to unload
event handlers being on the page. But with those removed, now bfcache is disabled due to Cache-Control: no-store
instead. It could be that for the 80% of users it is better to not disable bfcache and to instead let users opt-in to that via a plugin if they are concerned about privacy/security, such as when they are using a shared computer.
Here's plugin code to remove the no-store
directive from Cache-Control
header:
<?php /** * Plugin Name: Disable Cache-Control: no-store * Author: Weston Ruter */ add_filter( 'nocache_headers', static function ( $headers ) { if ( isset( $headers['Cache-Control'] ) && str_contains( $headers['Cache-Control'], 'no-store' ) ) { $directives = array_diff( preg_split( '/\s*,\s*/', $headers['Cache-Control'] ), array( 'no-store' ) ); $headers['Cache-Control'] = implode( ', ', $directives ); } return $headers; } );
#15
@
15 months ago
Regarding wp-includes/js/mce-view.js:
The code was for: https://core.trac.wordpress.org/ticket/38511#comment:10.
We could patch and use pagehide event, but after testing modern Firefox, the original issue from 7 years ago is not reproducible. Likely no longer an issue. The scope is WP users using the classic editor plugin and large shortcodes such as large galleries.
So, opting to remove the code. Commit: https://github.com/WordPress/wordpress-develop/pull/5056/commits/cfb32d01d58be96ffcef9fad778ed28e966713b9
#16
follow-up:
↓ 27
@
15 months ago
One of the instances of unload
is coming from the TinyMCE vendor dependency, src/js/_enqueues/vendor/tinymce/tinymce.js:
if (parent && parent !== getTop(parent)) { if (parent.addEventListener) { parent.addEventListener('unload', function () { setDocument(); }, false); } else if (parent.attachEvent) { parent.attachEvent('onunload', function () { setDocument(); }); } }
To fix this, we'd have to patch TinyMCE in wordpress-develop, as the current version 4.9.11 was released on 2020-07-13 which seems to be the last 4.x release. Not sure whether TinyMCE would do another 4.x release to fix the issue. This tinymce.js
file is loaded in both the Classic Editor and the Block Editor, although Gutenberg has an experiment to discontinue loading TinyMCE by default.
The source of this code can be found in the TinyMCE repo, specifically from the Sizzle library:
// Support: IE>8 // If iframe document is assigned to "document" variable and if iframe has been reloaded, // IE will throw "permission denied" error when accessing "document" variable, see jQuery #13936 // IE6-8 do not support the defaultView property so parent will be undefined if (parent && parent !== getTop(parent)) { // IE11 does not have attachEvent, so all must suffer if (parent.addEventListener) { parent.addEventListener('unload', function () { setDocument(); }, false); } else if (parent.attachEvent) { parent.attachEvent('onunload', function () { setDocument(); }); } }
So the code is specifically in place for Internet Explorer, which WordPress no longer supports. Therefore, it seems this code should could be removed entirely. Nevertheless, it is also present in the most recent version of Sizzle, and it specifically calls out Edge:
// Support: IE 9 - 11+, Edge 12 - 18+ // Accessing iframe documents after unload throws "permission denied" errors (jQuery #13936) // Support: IE 11+, Edge 17 - 18+ // IE/Edge sometimes throw a "Permission denied" error when strict-comparing // two documents; shallow comparisons work.
But seems that this is not actually relevant to Edge using Chromium, which it has since 2018. The highest version of Edge referenced in the last change is Edge 18+ (cf. first Edge mention commit), and Edge started using Chromium in v79. So it does seem obsolete.
#17
@
15 months ago
Regarding wp-includes/js/media-models.js
The code was https://core.trac.wordpress.org/ticket/22552.
The ticket said putting no-cache headers would solve the problem (https://core.trac.wordpress.org/ticket/22552#comment:25)
This JS fix for iOS 6 Safari should no longer be needed since WordPress >= 6.3 always sets no-cache headers, specifically in r55968 to fix #21938/#57627.
I removed the hook, tested the latest IOS Safari, and did not reproduce the bug, so it appears safe to delete.
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
14 months ago
This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.
14 months ago
This ticket was mentioned in Slack in #core by joemcgill. View the logs.
14 months ago
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
14 months ago
#22
@
14 months ago
- Keywords good-first-bug removed
- Milestone changed from 6.4 to Future Release
Punting to 6.5 because at present there is no performance benefit to doing this without revisiting the introduction of Cache-Control: no-store
from #21938.
#23
follow-up:
↓ 24
@
14 months ago
Guys,
Very frustrating seeing this looked at purely through a core perspective (and this seems to be a widespread problem).
Wordpress is more than anything a platform for plugins. Heartbeat is a standard way of communicating used by 100s on the front end and back end. It is even used by plugins for non logged in users.
There is a huge performance benefit here even without a solution for the no-store header
#24
in reply to:
↑ 23
@
14 months ago
Replying to shawfactor:
WordPress is more than anything a platform for plugins. Heartbeat is a standard way of communicating used by 100s on the front end and back end. It is even used by plugins for non logged in users.
There is a huge performance benefit here even without a solution for the no-store header
It's news to me that plugins are using this even for non-authenticated users. Do you have some examples?
#25
follow-up:
↓ 26
@
14 months ago
search github, you'll find quite a few:
https://github.com/search?q=heartbeat_nopriv_received&type=code
#26
in reply to:
↑ 25
@
14 months ago
- Milestone changed from Future Release to 6.4
Replying to shawfactor:
search github, you'll find quite a few:
https://github.com/search?q=heartbeat_nopriv_received&type=code
OK, searching for that string on WPdirectory I see there are only 28 plugins that use that filter. Of them, the most popular plugin (by two magnitudes) using the hook is BuddyPress.
So it still doesn't seem fixing this will have a huge impact on performance for unauthenticated users. Nevertheless, let's see if we can still get it into this release.
#27
in reply to:
↑ 16
;
follow-up:
↓ 28
@
14 months ago
Replying to westonruter:
One of the instances of
unload
is coming from the TinyMCE vendor dependency, src/js/_enqueues/vendor/tinymce/tinymce.js:
I filed an issue with the Sizzle library to remove unload
, but I then found that Sizzle itself is deprecated. So it is unlikely that this code will be removed. Since it is bundled with TinyMCE 4.x which is also an old release branch, and since there is an experiment to stop loading TinyMCE by default in the block editor, it seems patching TinyMCE here isn't going to be critical.
#28
in reply to:
↑ 27
@
14 months ago
Replying to westonruter:
Since it is bundled with TinyMCE 4.x which is also an old release branch, and since there is an experiment to stop loading TinyMCE by default in the block editor, it seems patching TinyMCE here isn't going to be critical.
Right, also there aren't any plans to upgrade TinyMCE to a newer version (afaik). That would break all custom integrations (in core and third parties), as the new versions are not backwards compatible. Once TinyMCE is deprecated in the block editor, the only remaining place with significant use will be the old "Edit" screens that are enabled by the Classic Editor plugin.
This ticket was mentioned in Slack in #core-performance by flixos90. View the logs.
14 months ago
#30
@
14 months ago
Something that has come to my attention is that since HTTP Archive includes results from Lighthouse, and since Lighthouse has an audit for bfcache, we should be able to do a query to determine how frequently bfcache is currently getting disabled for sites appearing in HTTP Archive. This will not only give us concrete data on the impact of the issue, but it will also give a list of sites that are experiencing the issue which we can investigate further to see if a core API (e.g. heartbeat) or a plugin is responsible.
This ticket was mentioned in Slack in #core by oglekler. View the logs.
14 months ago
#32
@
14 months ago
- Type changed from enhancement to defect (bug)
Converting to defect since I learned today that unload
already is not working on some mobile browsers.
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
13 months ago
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
13 months ago
#35
@
13 months ago
OK, I think I've done all I can with this PR. The only outstanding question is whether some decade-plus iOS Safari bugs still need special workarounds: one of them was removed in the PR, and the other remains. I can't test these as I don't have an iOS device available. It's ready for review, although it will benefit most from someone like @azaozz who has extensive experience with the related screens (legacy media uploads and the classic editor).
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
13 months ago
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
13 months ago
#38
@
13 months ago
I've been doing some HTTP Archive queries on bfcache on WordPress pages. I found that bfcache is disabled on a third of all WP pages indexed by HTTP Archive. And I found that "The page has an unload handler in the main frame" is the second most common reason for it being disabled. Nevertheless, I also found that the Heartbeat script is only present on 0.25% of pages. Therefore, it doesn't seem that core is the primary reason for bfcache being disabled.
#39
follow-up:
↓ 40
@
13 months ago
Yes isnt that a function of not many people using the heartbeat api or the internet archive not being able to scrape authenticated webpages?
Bfcache is disabled in 100% of the time where heartbeat is used and it is the standard way of doing this sort of communication in WordPress…
#40
in reply to:
↑ 39
@
13 months ago
Replying to shawfactor:
Yes isnt that a function of not many people using the heartbeat api or the internet archive not being able to scrape authenticated webpages?
True, it isn't crawling authenticated pages, but as of #21938 bfcache is disabled for all authenticated requests anyway.
#41
follow-up:
↓ 42
@
13 months ago
I’m not sure you are right that it’s disabled for all authenticated requests. Is it nocache_headers() that preventing it? AFAICT that’s only called on the back end, heartbeat is used by alot of plugins on the front? Maybe I’ve missed in the code
Secondly regardless of the answer that is be default, many people override the headers
Finally chrome are on the path to deprecate the unload event anyway:
#42
in reply to:
↑ 41
@
13 months ago
Replying to shawfactor:
I’m not sure you are right that it’s disabled for all authenticated requests. Is it nocache_headers() that preventing it? AFAICT that’s only called on the back end, heartbeat is used by alot of plugins on the front? Maybe I’ve missed in the code
As of #21938 for WordPress 6.3, all authenticated requests get sent Cache-Control: no-store
. This disables bfcache, just as an unload
event listener does.
Secondly regardless of the answer that is be default, many people override the headers
True, but in any case, my query of HTTP Archive shows it will have minimal impact on bfcache being disabled, since a tiny percentage of pages have the heartbeat script.
Finally chrome are on the path to deprecate the unload event anyway:
https://developer.chrome.com/blog/deprecating-unload/
I'm not disagreeing that it needs to be done. It's just I don't see how it'll have any meaningful impact for bfcache on the web as a whole.
13 months ago
#43
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 PC.
@westonruter commented on PR #5056:
13 months ago
#45
Committed in r56809.
#46
follow-up:
↓ 48
@
13 months ago
I found that there is some JS code in Gutenberg that has an unload
event as well, but this is a transitive dependency on lib0/indexeddb
. I've opened an upstream issue to address this: https://github.com/dmonad/lib0/issues/78
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
13 months ago
#48
in reply to:
↑ 46
@
13 months ago
Replying to westonruter:
I found that there is some JS code in Gutenberg that has an
unload
event as well, but this is a transitive dependency onlib0/indexeddb
. I've opened an upstream issue to address this: https://github.com/dmonad/lib0/issues/78
And it has now been removed from there as well.
Putting this on the performance focus to get some eyes on it.
According to the caniuse listing for the related pageshow and pagehide events, browser support is at 97% of all users.