Interactivity API: Improvements to the experimental full-page navigation #64067

michalczaplinski · 2024-07-29T18:03:01Z

This PR brings several improvements to experimental full-page navigation.

1. Remove `src` attributes from prefetched scripts

We have to remove the src attributes of the prefetched scripts because otherwise the contents are ignored. Reported by @luisherranz in #63880 (comment).

2. Use `.textContent` instead of `.innerText` to set `<script>` contents

Prefetched scripts that are inserted in the the <head> have <br> in the DOM:

The article from MDN about innerText explains why we see this problem:

As a setter this will replace the element's children with the given value, converting any line breaks into
elements.

3. `populateInitialData()` with state from the server

We have to populate the client-side state with the state coming from the server (as mentioned in this comment:

We populate the client-side state with the state from the server on the initial load.
We also re-populate the state when doing region-based navigation. But we don't do the equivalent when doing full page navigations.

4. Wait for `load` event of the `script` element

Finally, there is another issue:

Scripts that are added to the DOM with type="module" are deferred by default! This means that if we add a script to the <head> the way we currently do it is not guaranteed to have executed before the HTML of the page is updated.

Notice how the DOM had been updated before the onload handler fires. The error at the end that is caused by it because the JS store that contains the callbacks.initTriggerButton has not yet loaded on the page.

output_87404e.mp4

The solution proposed in this PR follows this comment:

For prefetching create <link> elements with modulepreload instead of fetching the modules ourselves.
Upon navigation, use import() to evaluate the scripts.

Testing instructions

You can test this PR manually by using blocks that use the Interactivity API and enabling the client-side navigation in GB settings.

Some things to watch out for:

Watch out for any client-side console errors.
Ensure the JS assets are preloaded correctly (watch the Network tab of your browser's devtools). They should be preloaded on hovering over a link.
Ensure that the cache-busting works correctly (as referred to in this comment)
Ensure that upon hovering over a link, we only pre-load modules, NOT all scripts.

Example test

Make sure to enable the "full page client-side navigation" experiment in GB settings
Create a post/page with an Image block using the "Lightbox" feature. Let's call it an Image test post.
Navigate to your Blog Archive page.
Hover over the link to the Image test post.
TEST: Ensure that the assets are preloaded correctly.
Click on the link to the Image test post.
TEST: Ensure that the post is navigated to using full-page client-side navigation.
TEST: Click on the image to open the Lightbox. Ensure that it can be opened and closed without issue.
Navigate back to the Blog Archive page.
TEST: Ensure that the navigation was done using full-page client-side navigation.

Several follow-ups should be done:

Figure out the way to only load the scripts for the new & compatible interactive blocks on the page. Previously mentioned in Image block view script causes errors when client-side navigation is enabled #63880 (comment) & Experiment: Add full page client-side navigation experiment setting #59707 (comment)
Refactor the fetchHeadAssets() function so that it's not called inside of regionsToVdom.
Handle CSS asset loading (not done in this PR):
1. Handle properly the cache busting hash in the URLs of the block styles. See this comment
2. Moving the styles from a file to <style> tags messes up with how the URLs are interpreted in CSS's url() function: Interactivity API: Improvements to the experimental full-page navigation #64067 (comment)
Occasionally the CSS for the page after a navigation is messed up (see video). I'm not sure if this is due to missing CSS or or file order or something else.

output_a052aa.mp4

github-actions · 2024-08-01T18:38:07Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: michalczaplinski <czapla@git.wordpress.org>
Co-authored-by: DAreRodz <darerodz@git.wordpress.org>
Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: luisherranz <luisherranz@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

luisherranz · 2024-08-02T07:32:10Z

I believe that this fix should work after #62734 is merged.

What functionality or change from the other pull request does this one need to work?

michalczaplinski · 2024-08-02T12:35:12Z

What functionality or change from the other pull request does this one need to work?

Sorry, this comment is outdated and I've removed it now. I made it before updating this PR . We don't need any specific functionality from that PR.

luisherranz · 2024-08-06T11:02:06Z

I added a comment here about the general strategy of preloading and waiting for resources to load.

Tracking Issue: Full-page client-side navigation #60951 (comment)

michalczaplinski · 2024-08-07T12:31:18Z

I've just noticed that this:

// wait for the `load` event to fire before appending the element
return new Promise( ( resolve, reject ) => {
  element.onload = resolve;
  element.onerror = reject;
} );

actually breaks the full-page navigation. We can't rely on the onload event I guess. I'll take a look shortly.

michalczaplinski · 2024-08-20T14:04:19Z

I've just noticed that this:
// wait for the `load` event to fire before appending the element
return new Promise( ( resolve, reject ) => {
  element.onload = resolve;
  element.onerror = reject;
} );
actually breaks the full-page navigation. We can't rely on the onload event I guess. I'll take a look shortly.

Just coming back to this. The reason I was seeing breakage is because the load event never fires on inline <script> elements. So something like this does not work:

     const script = document.createElement('script');
     script.textContent = 'console.log("hi");'    
     script.onload = () => { console.log("loaded")}; // This will log
     document.body.append(script);

I'm trying a different approach now:

Instead of loading the content with fetch() and inlining it into <script> and <style> elements, fetch it with <link rel="modulepreload">. For styles, we can use rel="preload" as="style".
In the updateHead() return a Promise waiting for the load of the script.

luisherranz · 2024-08-20T14:08:49Z

In the updateHead() return a Promise waiting for the load of the script.

As I mentioned here, you don't need to add an actual script and use the load event for the scripts; you can use dynamic imports since all the scripts should be modules.

michalczaplinski · 2024-08-21T10:44:38Z

packages/interactivity-router/src/index.ts

+export const headElements = new Map<
+	string,
+	{ tag: HTMLElement; text?: string }
+>();


I've exported the headElements so that we don't have to pass it as an argument to fetchHeadAssets().

michalczaplinski · 2024-08-21T10:55:19Z

packages/interactivity-router/src/head.ts

+		[ ...headElements.entries() ]
+			.filter( ( [ , { tag } ] ) => tag.nodeName === 'SCRIPT' )
+			.map( async ( [ url ] ) => {
+				await import( /* webpackIgnore: true */ url );


We need the webpack comment here. Otherwise, webpack will try to code-split here into a new chunk and it does not permit using fully dynamic paths.

michalczaplinski · 2024-08-21T15:35:03Z

As I mentioned #60951 (comment), you don't need to add an actual script and use the load event for the scripts; you can use dynamic imports since all the scripts should be modules.

Yup, this is what I ended up doing. I think there's nothing wrong with adding the scripts and waiting for the load approach, though.

I think this is ready for review now.

There are 2 issues that I've noticed that should be fixed in follow-ups:

Do not load the scripts that have already been preloaded if they only differ by ?ver=<some-number> query param. I initially thought that this should be fixed when using the production build (npm run build) but it's not the case.

Fixed in 9aef408

output_a20672.mp4
Occasionally the CSS for the page after a navigation is messed up (see video). I'm not sure if this is due to missing CSS or or file order or something else.

output_a052aa.mp4

gziolo · 2024-08-21T15:46:16Z

I initially thought that this should be fixed when using the production build (npm run build) but it's not the case.

You should test it with SCRIPT_DEBUG set to false in your wp-config.php file.

michalczaplinski · 2024-08-21T16:14:26Z

I initially thought that this should be fixed when using the production build (npm run build) but it's not the case.

You should test it with SCRIPT_DEBUG set to false in your wp-config.php file.

Nice, I didn't think about it, thanks! 🙂

michalczaplinski · 2024-08-21T19:09:50Z

ok, I've just tried it and it looks like setting SCRIPT_DEBUG to false does not make any difference, unfortunately.

gziolo · 2024-08-21T20:37:04Z

ok, I've just tried it and it looks like setting SCRIPT_DEBUG to false does not make any difference, unfortunately.

This is the reason it uses time() for interactivity router script:

gutenberg/lib/interactivity-api.php

Line 13 in 0e88cda

    
           $default_version = defined( 'GUTENBERG_VERSION' ) && ! SCRIPT_DEBUG ? GUTENBERG_VERSION : time();

I don't know where the style.css comes from, but there must be something similar present in the code.

michalczaplinski · 2024-08-27T20:30:26Z

Thanks @gziolo

I figured out that GUTENBERG_VERSION is only available in production. In dev, it's unset.

gutenberg/bin/generate-gutenberg-php.php

Lines 22 to 23 in fb5fa24

	echo "define( 'GUTENBERG_VERSION', '$plugin_version' );\n";

For production, there is no problem in that case. ver === GUTENBERG_VERSION

In development, we should check if the full-page navigation experiment is enabled. And use the time() only if the experiment is disabled.

michalczaplinski · 2024-08-29T11:23:00Z

In development, we should check if the full-page navigation experiment is enabled. And use the time() only if the full-page navigation experiment is disabled.

I've now added this.

We should look into doing the same for indivudual block's stylesheets:

But this can be done in a follow-up

luisherranz · 2024-08-29T12:53:42Z

In development, we should check if the full-page navigation experiment is enabled. And use the time() only if the experiment is disabled.

Won't that cause problems for people developing in Gutenberg with that option enabled?

michalczaplinski · 2024-08-29T18:08:36Z

Won't that cause problems for people developing in Gutenberg with that option enabled?

I think that if you are developing with the full-page navigation enabled, then you DO want to disable the cache busting. Otherwise, full-page navigation does not work properly. This is how I did it:

gutenberg/lib/interactivity-api.php

Lines 17 to 25 in bcb3ff8

    
           $experiments                  = get_option( 'gutenberg-experiments' ); 
        
           $full_page_navigation_enabled = isset( $experiments['gutenberg-full-page-client-side-navigation'] ); 
        
           wp_register_script_module( 
        
           	'@wordpress/interactivity', 
        
           	gutenberg_url( '/build/interactivity/' . ( SCRIPT_DEBUG ? 'debug.min.js' : 'index.min.js' ) ), 
        
           	array(), 
        
           	$full_page_navigation_enabled ? null : $default_version 
        
           );

Am I missing something?

michalczaplinski · 2024-10-09T13:53:50Z

Thank you @sirreal ! 🙏

sirreal · 2024-10-23T10:18:40Z

There are several follow-up items from this PR that I don't want to slip through the cracks. Are they tracked anywhere?

michalczaplinski · 2024-10-23T14:16:19Z

Thanks for the vigilance @sirreal :)

Yes, they're tracked in #60951. In particular, see this latest comment.

michalczaplinski mentioned this pull request Jul 29, 2024

Image block view script causes errors when client-side navigation is enabled #63880

Closed

2 tasks

michalczaplinski added [Type] Bug An existing feature does not function as intended [Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity labels Jul 29, 2024

michalczaplinski changed the title ~~Fix: Full-page navigation for blocks with server-side defined state~~ Interactivity API: Improvements to the experimental full-page navigation Aug 1, 2024

michalczaplinski marked this pull request as ready for review August 1, 2024 18:37

michalczaplinski requested review from luisherranz and DAreRodz August 1, 2024 18:37

luisherranz mentioned this pull request Aug 6, 2024

Tracking Issue: Full-page client-side navigation #60951

Open

15 tasks

michalczaplinski marked this pull request as draft August 7, 2024 12:31

michalczaplinski commented Aug 21, 2024

View reviewed changes

michalczaplinski marked this pull request as ready for review August 21, 2024 15:35

michalczaplinski requested a review from spacedmonkey as a code owner August 28, 2024 14:04

michalczaplinski requested a review from cbravobernal August 28, 2024 14:06

michalczaplinski removed request for atimmer, talldan, swissspidy, ntwb, ajitbohra, mmtr, ryanwelcher, nerrad, adamsilverstein, ellatrix, ndiego, artemiomorales, dmsnell, ObliviousHarmony, kevin940726, tellthemachines, fabiankaegy, cbravobernal and t-hamano October 8, 2024 14:01

Merge branch 'trunk' into fix/full-page-navigation-server-state

ba856cf

michalczaplinski enabled auto-merge (squash) October 8, 2024 14:12

michalczaplinski merged commit db22c1b into trunk Oct 8, 2024
62 checks passed

michalczaplinski deleted the fix/full-page-navigation-server-state branch October 8, 2024 14:46

github-actions bot added this to the Gutenberg 19.5 milestone Oct 8, 2024

felixarntz mentioned this pull request Oct 8, 2024

Interactivity API: Defer hydration until node is scrolled near the viewport #58284

Draft

michalczaplinski mentioned this pull request Oct 15, 2024

Add missing CHANGELOG entries for #64067 #66120

Merged

WontonSam mentioned this pull request Oct 27, 2024

[Snyk] Upgrade @wordpress/e2e-test-utils from 7.2.1 to 11.9.0 WontonSam/chicommerce#963

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interactivity API: Improvements to the experimental full-page navigation #64067

Interactivity API: Improvements to the experimental full-page navigation #64067

michalczaplinski commented Jul 29, 2024 •

edited

Loading

github-actions bot commented Aug 1, 2024 •

edited

Loading

luisherranz commented Aug 2, 2024

michalczaplinski commented Aug 2, 2024

luisherranz commented Aug 6, 2024 •

edited

Loading

michalczaplinski commented Aug 7, 2024

michalczaplinski commented Aug 20, 2024

luisherranz commented Aug 20, 2024

michalczaplinski Aug 21, 2024

michalczaplinski Aug 21, 2024

michalczaplinski commented Aug 21, 2024 •

edited

Loading

gziolo commented Aug 21, 2024

michalczaplinski commented Aug 21, 2024

michalczaplinski commented Aug 21, 2024

gziolo commented Aug 21, 2024

michalczaplinski commented Aug 27, 2024 •

edited

Loading

michalczaplinski commented Aug 29, 2024

luisherranz commented Aug 29, 2024

michalczaplinski commented Aug 29, 2024 •

edited by gziolo

Loading

michalczaplinski commented Oct 9, 2024

sirreal commented Oct 23, 2024

michalczaplinski commented Oct 23, 2024

Interactivity API: Improvements to the experimental full-page navigation #64067

Interactivity API: Improvements to the experimental full-page navigation #64067

Conversation

michalczaplinski commented Jul 29, 2024 • edited Loading

1. Remove src attributes from prefetched scripts

2. Use .textContent instead of .innerText to set <script> contents

3. populateInitialData() with state from the server

4. Wait for load event of the script element

Testing instructions

Example test

github-actions bot commented Aug 1, 2024 • edited Loading

luisherranz commented Aug 2, 2024

michalczaplinski commented Aug 2, 2024

luisherranz commented Aug 6, 2024 • edited Loading

michalczaplinski commented Aug 7, 2024

michalczaplinski commented Aug 20, 2024

luisherranz commented Aug 20, 2024

michalczaplinski Aug 21, 2024

Choose a reason for hiding this comment

michalczaplinski Aug 21, 2024

Choose a reason for hiding this comment

michalczaplinski commented Aug 21, 2024 • edited Loading

gziolo commented Aug 21, 2024

michalczaplinski commented Aug 21, 2024

michalczaplinski commented Aug 21, 2024

gziolo commented Aug 21, 2024

michalczaplinski commented Aug 27, 2024 • edited Loading

michalczaplinski commented Aug 29, 2024

luisherranz commented Aug 29, 2024

michalczaplinski commented Aug 29, 2024 • edited by gziolo Loading

michalczaplinski commented Oct 9, 2024

sirreal commented Oct 23, 2024

michalczaplinski commented Oct 23, 2024

michalczaplinski commented Jul 29, 2024 •

edited

Loading

1. Remove `src` attributes from prefetched scripts

2. Use `.textContent` instead of `.innerText` to set `<script>` contents

3. `populateInitialData()` with state from the server

4. Wait for `load` event of the `script` element

github-actions bot commented Aug 1, 2024 •

edited

Loading

luisherranz commented Aug 6, 2024 •

edited

Loading

michalczaplinski commented Aug 21, 2024 •

edited

Loading

michalczaplinski commented Aug 27, 2024 •

edited

Loading

michalczaplinski commented Aug 29, 2024 •

edited by gziolo

Loading