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?

…l page navigation is enabled

…sset files - Added logic to retrieve version information from `index.min.asset.php` and `router.min.asset.php` files. - Updated `wp_register_script_module` calls to use the retrieved version instead of the default version when full-page navigation is not enabled.

This reverts commit 62e527e.

github-actions · 2024-09-17T19:20:43Z

Flaky tests detected in a9469d0.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11016205655
📝 Reported issues:

[Flaky Test] Navigates the items list via UP/DOWN arrow keys #64334 in /test/e2e/specs/site-editor/dataviews-list-layout-keyboard.spec.js

michalczaplinski · 2024-09-17T20:13:27Z

I believe this should be good to be re-reviewed again @gziolo @DAreRodz @sirreal 🙇‍♂️

gziolo · 2024-09-18T16:09:30Z

What is the best way to test these changes? How much confidence can we put in the existing e2e tests coverage?

michalczaplinski · 2024-09-18T19:22:30Z

What is the best way to test these changes? How much confidence can we put in the existing e2e tests coverage?

Apologies, I've now added better testing instructions in the PR description.

The existing e2e tests do not cover the scenarios in this PR. I have started working on additional e2e tests in #63268

I'm happy to continue working on them as an immediate follow-up. We'll need them before full-page client-side navigation is moved out of GB experiment. For now, as full-page client-side navigation is still experimental, I'm not worried about the impact of this PR. The current implementation has been somewhat broken for months (which this PR attempts to fix 🙂).

lib/interactivity-api.php

gziolo · 2024-09-24T10:41:38Z

packages/interactivity-router/src/index.ts

-		} );
-		await fetchHeadAssets( document, headElements );
+		[].map.call(
+			document.querySelectorAll( 'script[type="module"][src]' ),


That's intriguing, as it skips all scripts that aren't ES Modules moving forward. It never covered inline scripts, to there is no change regarding that.

What's the story for the plugins or themes that add custom JavaScript to the site? It will load on the full-page reload but not when doing the full-page client-side navigation. It would be good to start collecting the requirements for the feature to work correctly.

sirreal · 2024-09-24T10:46:57Z

packages/interactivity-router/src/head.ts

@@ -29,6 +37,14 @@ export const updateHead = async ( newHead: HTMLHeadElement[] ) => {
 		}
 	}

+	await Promise.all(
+		[ ...headElements.entries() ]


This looks good, but I always worry about these data structures because I think of Map as an unordered collection. I was worried about dependencies and ordering not being respected:

The entries() method of Map instances returns a new map iterator object that contains the [key, value] pairs for each element in this map in insertion order.

From MDN (bold mine).

It seems like we're OK since entries returns elements in insertion order 👍

gziolo · 2024-09-24T10:49:11Z

We need to validate whether this PR fixes #63880. If not, then we should not need to backport this PR to WP 6.7, so the corresponding label will have to be removed.

sirreal · 2024-09-24T10:49:46Z

packages/interactivity-router/src/head.ts

+
+			const headElement = headElements.get( href );
+			const styleElement = doc.createElement( 'style' );
+			styleElement.textContent = headElement.text;


Is it also necessary to store the text? Couldn't headElements be Map< string, HTMLElement >, then this could become styleElement.textContent = headElement.textContent?

DAreRodz · 2024-09-24T14:41:03Z

I have found an issue (maybe introduced after merging trunk?).

Following the steps listed in the description, I see this error when I navigate to the "Image test" page:

context.ts:62 Uncaught (in promise) TypeError: Invalid value used as weak map key
    at WeakMap.set (<anonymous>)
    at proxifyContext (context.ts:62:26)
    at directives.tsx:179:49
    at F (index.js:322:25)
    [...]

After a bit of debugging, the cause of the problem seems to be inside the wp-context directive. These refs lose their initial value somehow:

gutenberg/packages/interactivity/src/directives.tsx

Lines 149 to 150 in 001241b

    
           const client = useRef( proxifyState( ns, {} ) ); 
        
           const server = useRef( proxifyState( ns, {}, { readOnly: true } ) );

This is a quick fix we could use right after initialize the refs:

if ( ! client.current ) {
    client.current = proxifyState( ns, {} );
}
if ( ! server.current ) {
    server.current = proxifyState( ns, {}, { readOnly: true } );
}

However, this is weird, and would require further investigation. It could be an issue in Preact, or the way it reconciles elements during rendering. I have no idea at this moment. 😅

DAreRodz · 2024-09-24T14:48:22Z

@michalczaplinski, I've added minor changes to the renderRegions() function in a9469d0. Just made the renderRegions() function async and wrapped only the required parts with batch(). 🙂

michalczaplinski mentioned this pull request Jul 29, 2024

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

Open

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 added 11 commits September 17, 2024 19:14

Do not load interactivity script modules in development mode when ful…

eac0deb

…l page navigation is enabled

Format interactivity-api.php

af136bc

empty commit

56a9b08

Rename populateInitialData to populateServerData

bc0c933

remove populateServerData

78d2d88

try: remove the webpack comment

a5a40cc

try: remove the await import()

3cef459

bring back the async import

2811a8c

Move headElements to head.ts

79bd1c9

Revert "try: remove the webpack comment"

06fd98d

This reverts commit 62e527e.

michalczaplinski force-pushed the fix/full-page-navigation-server-state branch from 068d62f to 06fd98d Compare September 17, 2024 18:16

default_version => router_version

9983180

michalczaplinski mentioned this pull request Sep 17, 2024

Undefined array key "paged" in comments-pagination-previous.php #65424

Closed

michalczaplinski requested a review from sirreal September 17, 2024 20:12

noisysocks added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 18, 2024

sirreal reviewed Sep 23, 2024

View reviewed changes

lib/interactivity-api.php Outdated Show resolved Hide resolved

michalczaplinski added 2 commits September 24, 2024 11:15

Remove the changes to interactivity-api.php

0186d2a

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

3e49d89

gziolo reviewed Sep 24, 2024

View reviewed changes

sirreal reviewed Sep 24, 2024

View reviewed changes

Make renderRegions async

a9469d0

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

github-actions bot commented Sep 17, 2024 •

edited

Loading

michalczaplinski commented Sep 17, 2024

gziolo commented Sep 18, 2024

michalczaplinski commented Sep 18, 2024

gziolo Sep 24, 2024

sirreal Sep 24, 2024 •

edited

Loading

gziolo commented Sep 24, 2024

sirreal Sep 24, 2024

DAreRodz commented Sep 24, 2024

DAreRodz commented Sep 24, 2024

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

Are you sure you want to change the base?

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

github-actions bot commented Sep 17, 2024 • edited Loading

michalczaplinski commented Sep 17, 2024

gziolo commented Sep 18, 2024

michalczaplinski commented Sep 18, 2024

gziolo Sep 24, 2024

Choose a reason for hiding this comment

sirreal Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

gziolo commented Sep 24, 2024

sirreal Sep 24, 2024

Choose a reason for hiding this comment

DAreRodz commented Sep 24, 2024

DAreRodz commented Sep 24, 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

github-actions bot commented Sep 17, 2024 •

edited

Loading

sirreal Sep 24, 2024 •

edited

Loading