-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use defer loading strategy for frontend view scripts #52536
Conversation
For the Navigation block and the Search block, it would actually be ideal to load them in the |
This comment was marked as off-topic.
This comment was marked as off-topic.
…dd/defer-script-loading-strategy * 'trunk' of https://github.com/WordPress/gutenberg: Update Changelog for 16.2.0 Adding support for defined IDs in `TextControl` component (#52028) Bump plugin version to 16.2.0 Revert "Bump plugin version to 16.2.0" Bump plugin version to 16.2.0 Add maxLength to LinkControl search items (#52523) [RNMobile] Update Editor block inserter button styles and default text input placeholder/selection styles (#52269) Site Editor: Reset device preview type when exiting the editing mode (#52566) Trim footnote anchors from excerpts (#52518) Add back old Navigation and File blocks JavaScript implementation when Gutenberg is not installed (#52553) Block Editor: Ensure synced patterns are accounted for in 'getAllowedBlocks' (#52546) Fix md5 class messed up with new block key (#52557) Fix entity cache misses for single posts due to string as recordKey (#52338) Make "My patterns" permanently visible (#52531) Hide site hub when resizing frame upwards to avoid overlap (#52180) Fix "Manage all patterns" link appearance (#52532) Update navigation menu title size & weight in detail panels (#52477) Site Editor Patterns: Ensure sidebar does not shrink when long pattern titles are used (#52547) Site Editor: Restore quick inserter 'Browse all' button (#52529) Patterns: update the title of Pattern block in the block inspector card (#52010)
Switching back to draft, because as of #52553 the old non-Interactivity API implementations of the File block and Navigation block are available again. I'll ensure they are deferred (or made |
Size Change: +196 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
…dd/defer-script-loading-strategy * 'trunk' of https://github.com/WordPress/gutenberg: (24 commits) Add filter to turn off Interactivity API for a block (#52579) Search: Remove unnecessary useEffect (#52604) Navigation: Simplify the useSelect for useNavigationMenus (#51977) Item: Unify focus style and add default font styles (#52495) Update Changelog for 16.2.1 Bump plugin version to 16.2.1 Avoid passing undefined `selectedBlockClientId` in `BlockActionsMenu` (#52595) Cover Block: Fix block deprecation when fixed background is enabled (#51612) Nav block: link text color inheritance fixes and tests (#51710) Stabilize defaultBlock, directInsert API's and getDirectInsertBlock selector (#52083) Fix console warning by improving error handling in Nav block classic menu conversion (#52591) Fix: Remove link action of Link UI for draft pages created from Nav block does not correctly remove link. (#52415) Lodash: Remove remaining `_.get()` from block editor and deprecate (#52561) Fix importing classic menus (#52573) ResizableFrame: Make keyboard accessible (#52443) Site Editor: Fix navigation menu sidebar actions order and label (#52592) correct a typo: sapce -> space (#52578) Avoid errors in Dimension visualizers when switching between iframed and non-iframed editors (#52588) Patterns: Add client side pagination to patterns list (#52538) Site Editor: Make sidebar back button go *back* instead of *up* if possible (#52456) ...
* Fully leverage event delegation to allow async loading strategy. * Keep track of whether a submenu is open to short-circuit event handlers. * Use passive event listeners.
* Fully leverage event delegation to allow async loading strategy. * Remove event listener for anchor clicks in modal when modal is closed. * Use passive event listeners.
…e File block does
…and Navigation block
* Adopt asynchronous loading strategy. * Use event delegation. * Collapse expanded blocks when tabbing out of expanded Search block. * Only attach keydown/keyup event handlers while Search block is expanded. * Ensure search button's aria-label is translated. * Ensure Search button's type is restored to 'button' instead of deleting type (since no type is same as 'submit'). * Use passive event listeners.
I've commented on Core-54018 to inquire if there are any cases where a block's view script has to be executed in the |
Actually, I think I know what happened. When using a classic theme, the view scripts for blocks actually get printed in the footer. This is because blocks are parsed in the middle of template rendering, which means that view scripts are enqueued too later for printing at Given that classic themes all print in the footer, it seems there is absolutely no reason to not add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree head
and defer
is always the best option for block scripts. Thanks for taking care of this Weston :)
Could you share your merging plan?
- Will you wait for WP 6.3 release and then set WP 6.3 as the lowest required version for Gutenberg?
- Are you planning to integrate the new strategy implementation into Gutenberg's compatibility mechanism? (Is it already present?)
- Is the plan that the strategy applies when users are on WP 6.3, but doesn't apply to those using older WP versions?
Please notify me if you're opting for the final approach because merging this as it is will break the Interactivity API scripts.
wp_script_add_data( 'wp-interactivity', 'strategy', 'defer' ); | ||
|
||
// Move all the view scripts of the interactive blocks to the footer. | ||
$registered_blocks = \WP_Block_Type_Registry::get_instance()->get_all_registered(); | ||
foreach ( array_values( $registered_blocks ) as $block ) { | ||
if ( isset( $block->supports['interactivity'] ) && $block->supports['interactivity'] ) { | ||
foreach ( $block->view_script_handles as $handle ) { | ||
wp_script_add_data( $handle, 'group', 1 ); | ||
wp_script_add_data( $handle, 'strategy', 'defer' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that if we are making all frontend block scripts deferred by default, we don't need to do it for the supports.interactivity
ones again. We still need to add defer
to wp-interactivity
, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Done in aab0b4b
Thank you for taking care of this change so quickly, @westonruter! 🙌 |
@luisherranz I've added a version check to continue loading in the footer if on WP<6.3. In 6.3+, the scripts will load in the
I'm not sure I understand. |
Perfect, thank you! 🙂
Oh, sorry. I'm not versed in this aspect of Gutenberg, but I was wondering if you had any intention of adding it to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me 🎉
My only pushback would be on the comment-reply
script changes, as they feel unrelated, and potentially even for core not beneficial. I think we should decouple that conversation and remove the change from here.
@luisherranz Yeah, I'm not so well versed in it either 😊 I don't think it's feasible to extract the implementation of the script loading strategies into |
@felixarntz Sounds good to me. However, in core there may still be a benefit in that as I understand |
Thanks @westonruter, I suggest we open a Trac ticket to discuss this further. |
Co-authored-by: Felix Arntz <[email protected]>
Filed: Delay loading comment-reply script with async loading strategy (Core-58870) cc @sgomes as I suggest the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, LGTM code-wise!
There's a failure in https://github.com/WordPress/gutenberg/actions/runs/5614408652/job/15212506354?pr=52536, but seems unrelated?
…ding-strategy * origin/trunk: (59 commits) Promisify action creator return type for WP data dispatch (#52530) [RNMobile] Add WP hook for registering non-core blocks (#52791) removes check for active preview device type to enable the fixed toolbar preference (#52770) Enforce checks against redeclaration for functions and classes (#52696) update appearance tools, (#52785) Behaviors: Extend Global Styles API to read/write behaviors config. (#52370) HeaderToolbar - Update inserterMethod meta data (#52735) add options for debugging php unit tests (#52778) Docs: Interactivity API > Getting Started Guide - minor adjustments (#52786) Footnotes: Use static closures when not using '' (#52781) Improve slug generation & matching in request utils (#52414) Open "docs" folder for the Interactivity API package and Getting Started Guide (#52462) Global Styles: Don't use named arguments for 'sprintf' (#52782) E2E utils - Update locator to hide the keyboard on iOS to pick the first element, on iPad two buttons are available and the second one makes the floating keyboard to show up (#52771) Patterns: Reinstate template parts mode spec (#52780) chore(release): publish Update changelog files Patterns: Fix empty general template parts category (#52747) Add id to pattern inserted notice to stop multiple notices stacking (#52746) Site Editor: Fix site link accessibility issues (#52744) ...
Seems so. E2E is also failing in |
I agree 🙂 |
Thank you, @westonruter! 👍 I left a more elaborate comment there, but TL;DR: both |
* $script_dependencies, | ||
* - isset( $script_asset['version'] ) ? $script_asset['version'] : false | ||
* + isset( $script_asset['version'] ) ? $script_asset['version'] : false, | ||
* + array( 'strategy' => 'defer' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@westonruter, do we have a patch against WordPress core that sets the default strategy for viewScripts
to defer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm catching up after an extended leave, so I might find the answer myself in the meantime as I'm reading the communication related to the defer
strategy. I'm mostly willing to ensure we have better defaults in WP core, so people have performant configuration by design.
At the moment, I don't see any changes applied in core:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch hasn't been backported to core yet. I wasn't sure when that should be done, if it should sit in Gutenberg a bit first to test and then propose for core, or to add it to core at the same time. I'm happy to open a backport ticket and open a PR to apply this patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened Core-59115 and WordPress/wordpress-develop#5019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committed to core: r56398
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's enable it for all blocks in both the Gutenberg plugin, and in WordPress core to get as much testing as necessary with 3rd party blocks that use block.json
. I see that you discussed it in the context of view scripts in #52536 (comment) and ruled out as a potential issue.
Committed to core: r56398
Noting that with this patch, defer
is going to be used with editorScript
, script
, and viewScript
. In the case of editorScript
, the dependencies aren't using defer
like wp-block
or wp-block-editor
so it will probably fallback to the legacy handling. I'll comment on Trac, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's enable it for all blocks in both the Gutenberg plugin, and in WordPress core to get as much testing as necessary with 3rd party blocks that use
block.json
. I see that you discussed it in the context of view scripts in #52536 (comment) and ruled out as a potential issue.
Yes, it should be enabled for all blocks, whether core or 3rd party.
Noting that with this patch,
defer
is going to be used witheditorScript
,script
, andviewScript
. In the case ofeditorScript
, the dependencies aren't usingdefer
likewp-block
orwp-block-editor
so it will probably fallback to the legacy handling. I'll comment on Trac, too.
Oh, good catch!
What?
Use the
defer
andloading strategies for frontend view scripts, which WordPress 6.3 is introducing in Core-12009. See Dev Note.async
If the
defer
loading strategy were used exclusively, then none of the changes to the JS files would have been necessary.However, to further improve user experience with some blocks, the use of theasync
loading strategy was used for the Navigation and Search blocks so that their view scripts can load in thehead
and execute as soon as possible so that the user can click on these blocks which are normally in the header. This required a refactor to employ event delegation instead of theDOMContentLoaded
orload
events.Why?
Fixes #51930.
These can now be leveraged to improve loading performance for frontend scripts by not blocking the rendering of the page. It also improves UX by allowing behaviors for UI elements in the Navigation block and Search block to be attached before the DOM has fully loaded.
How?
Add the
strategy
script data with the value ofasync
ordefer
for frontend scripts.Forcomment-reply
, it sets thedefer
loading strategy which we'll likely eventually do in core with v6.4. This change is done for the Comments block and the Post Comments Form block. This provides a way to test deferred loading in the meantime. In practice, this change will have minimal effect since it is already printed in the footer.defer
. This addresses a todo to leverage script deferring when it is in core. This change also will have minimal impact because these scripts are also printed in the footer. (The experimental Interactivity API is enabled by default when using the Gutenberg plugin, but can be disabled via filter in Add filter to turn off Interactivity API for a block #52579.)async
anddefer
loading strategies are added manually viawp_script_add_data()
as outlined in the Dev Note. Ideally the loading strategy and whether it can load in the footer could be indicated in theblock.json
. See Blocks: Allow customizations for assets inblock.json
#46954 and Core-54018.Navigation Block
Fixes #42394.
Loading strategy:
defer
async
view.js
script andview-modal.js
script if submenu-on-click and overlay menu (responsive mobile menu button) options are enabled, respectively. Previously both of these scripts would be enqueued if either options were enabled. This ensures either script is only enqueued if it is actually needed.click
events anywhere in thedocument
rather than waiting to attachclick
events on all elements at thewindow
'sload
event. Theload
event is particularly problematic because it only fires after all page resources have loaded, including images, fonts, styles, and scripts. PresumablyDOMContentLoaded
would have been a better fit.In any case, event delegation is even better than using either of these events because it allows for the view scripts to be loaded asynchronously in thehead
(theasync
loading strategy) and not wait for the page to finish loading. This means that even when the page HTML has half-loaded, if the async view scripts loaded from the head have executed, the user can much sooner start interacting with the navigation block and not rage click on buttons without anything happening.Behavior of the improved implementations of the Navigation block's view scripts are likely more performant and resilient than the current Interactivity API implementation due to the asynchronous loading.Search Block
Loading strategy:
defer
async
Register view script inThis was done independently as of Search block: Enqueue view script through block.json #52552.block.json
and conditionally include/exclude based on whether block options require it. (This follows the same pattern from the Navigation block and File block.)DOMContentLoaded
event is used,so when the script is asynchronously loaded from the.head
the user can tap on a collapsed Search block as before the DOM fully loadsaria-label
.File Block
Loading strategy:
defer
The view script is just responsible for hiding PDF embeds for browsers that don't support them, so the
defer
loading strategy is applied since this query for File blocks is done atDOMContentLoaded
. Since Gutenberg prints block scripts in thehead
, thedefer
strategy prevents it from blocking rendering. It is less likely that a File block would appear in the initial viewport compared with the Navigation and Search blocks, so this is another reason why it is deferred to run once the DOM loads.Testing Instructions
I've configured the header in a block theme as follows, with two row variations:
In Twenty Twenty-Three with WordPress 6.3-beta4:
add_filter( 'gutenberg_should_block_use_interactivity_api', '__return_false' )
.a. Populate a Navigation with nested submenus, ensure the overlay menu option (responsive menu) is enabled, as well as the open on click submenus. This causes both view scripts to be enqueued. Make sure that one of the links in the Navigation is for an anchor link, such as to
#respond
, which is important to test clicks when the responsive menu is open. (In my test, I have two copies of the Navigation block, one in which the overlay menu is off and another in which it is always.)b. Configure a Search block so that it is "button only". This causes the view script to get enqueued. (I found this a bit tricky to do, as I had to re-select the same option a couple times.)
c. Optionally duplicate a row containing the two previous blocks and add more variations.
The
head
should include:Note how the 4 view scripts for the Search block, Navigation block, and File block are all
defer
.Note that this is the case I'm most interested in because it is what is currently reflected in core, since the Interactivity API is experimental.
Expected markup when Interactivity API is enabled
When the Interactivity API is enabled, the
head
should contain:Note that the Search block has not yet been updated to use the Interactivity API.
Performance Analysis
Before and after the branch for this PR was checked out, I ran the following on the Gutenberg development environment (wp-env):
npm run research -- benchmark-web-vitals -u http://localhost:8888/?p=8 -n 20 -p
And I combined the two sets of results into a single table showing the difference for the FCP, LCP, and LCP-TTFB metrics:
Of particular note, the median of requests experienced a 19.3% reduction in LCP-TTFB. This metric discounts TTFB which would be the case when good page caching is in place.
I tested on an HP Dragonfly Chromebook running in the Linux environment. Testing on a more powerful Macbook Pro would surely yield less impressive results. Conversely, testing on a low-powered Android phone should be even more impressive.