Make WordPress Core

Opened 2 years ago

Closed 5 months ago

Last modified 5 months ago

#55491 closed defect (bug) (fixed)

Replace `unload` event handlers from core

Reported by: shawfactor's profile shawfactor Owned by: westonruter's profile westonruter
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: javascript, performance Cc:

Description (last modified by adamsilverstein)

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.

https://web.dev/bfcache/

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 @peterwilsoncc
2 years ago

  • Component changed from General to Administration
  • Focuses javascript performance added
  • Version 5.9 deleted

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.

#2 @shawfactor
2 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 @shawfactor
7 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 @mukesh27
7 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 @swissspidy
7 months ago

The media modal and some TinyMCE code also seem to be affected by this. Some holistic bfcache testing might be worthwhile.

#6 @westonruter
7 months ago

  • Owner set to westonruter
  • Status changed from new to accepted

#7 @westonruter
7 months ago

  • Milestone changed from Future Release to 6.4

#8 @adamsilverstein
7 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 @adamsilverstein
7 months ago

  • Description modified (diff)
  • Summary changed from Replace unload from heartbeat js to Replace `unload` event handlers from core

#10 @adamsilverstein
7 months ago

  • Keywords good-first-bug added

This ticket was mentioned in PR #5056 on WordPress/wordpress-develop by @spenserhale.


6 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.

#14 @westonruter
6 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 @spenserhale
6 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: @westonruter
6 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.

Last edited 6 months ago by westonruter (previous) (diff)

#17 @spenserhale
6 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.

Commit: https://github.com/WordPress/wordpress-develop/pull/5056/commits/8699faabbdaba20da3a737a3b1b13ce9689d7286

Last edited 6 months ago by spenserhale (previous) (diff)

This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


6 months ago

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


6 months ago

#22 @westonruter
6 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: @shawfactor
6 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 @westonruter
5 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?

#26 in reply to: ↑ 25 @westonruter
5 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: @westonruter
5 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 @azaozz
5 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.


5 months ago

#30 @westonruter
5 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.


5 months ago

#32 @westonruter
5 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.


5 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


5 months ago

#35 @westonruter
5 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.


5 months ago

This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.


5 months ago

#38 @westonruter
5 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: @shawfactor
5 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 @westonruter
5 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.

Last edited 5 months ago by westonruter (previous) (diff)

#41 follow-up: @shawfactor
5 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:

https://developer.chrome.com/blog/deprecating-unload/

#42 in reply to: ↑ 41 @westonruter
5 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.

@azaozz commented on PR #5056:


5 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.

#44 @westonruter
5 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 56809:

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.

@westonruter commented on PR #5056:


5 months ago
#45

Committed in r56809.

#46 follow-up: @westonruter
5 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.


5 months ago

#48 in reply to: ↑ 46 @westonruter
5 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 on lib0/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.

Note: See TracTickets for help on using tickets.