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

[westonruter]

Description

I have an Image block with light box enabled. When I have enabled client-side navigation, and I try navigating from the blog archive page to the post with the image block on it, I get errors in the console:

TypeError: Cannot read properties of undefined (reading '66a0412c9bdb8')
    at setButtonStyles (view.js:362:19)
    at store.ts:153:13
    at hooks.tsx:305:48
    at directives.tsx:359:7

This is the line in question:

gutenberg/packages/block-library/src/image/view.js

Line 362 in db4bad4

state.metadata[ imageId ].imageRef = ref;

It's not just that, however. If I short-circuit that setButtonStyles function from running entirely, then when I try to open a lightbox I get another error:

view.js:90 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading '66a0417c36565')
    at showLightbox (view.js:90:26)
    at store.ts:153:13
    at hooks.tsx:305:48
    at directives.tsx:359:7

This is the line in question:

gutenberg/packages/block-library/src/image/view.js

Line 90 in db4bad4

if ( ! state.metadata[ imageId ].imageRef?.complete ) {

I'm not seeing this issue in the latest stable Gutenberg (v18.8), except when I try closing as lightbox I am seeing another error:

TypeError: Cannot read properties of undefined (reading 'focus')
    at view.js:101:17

This error is coming from:

gutenberg/packages/block-library/src/image/view.js

Line 122 in db4bad4

state.currentImage.buttonRef.focus( {

I see this in both a local dev build in wp-env as well as in the stable build from dotorg installed on a vanilla WP install.

Step-by-step reproduction instructions

1. Create a post that has an Image block in it and enable the Lightbox.
2. Enable client-side navigation experiment.
3. Navigate to the blog index page.
4. Click on the post that has the Image block in it.
5. See the error in the console.

(The other error from closing the lightbox occurs on Gutenberg 18.8.)

Screenshots, screen recording, code snippet

Screen.recording.2024-07-23.16.53.36.webm

Environment info

I'm testing in Chrome 126.0.6478.132 on ChromeOS. Gutenberg commit db4bad4. Twenty Twenty-Three theme.

Please confirm that you have searched existing issues in the repo.

• Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

• Yes

[luisherranz]

This seems to me like the view.js file of the image block is not loading correctly after the navigation.

I am not familiar with the assets management of the full-page client-side navigation yet, but @michalczaplinski should be able to provide more details.

[michalczaplinski]

👋 I'm taking a look right now.

It seems that this is a problem with full-page navigation and blocks that have state defined on the server side (with wp_interactivity_state()).

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.

---

As an aside, there IS something strange with how the view.js script of the Image block appears in the DOM which might be another (but different) bug:

[michalczaplinski]

I've attempted a fix in #64067 where I just call populateInitialData() after a navigation.

However, even with that fix present, there is another issue. E.g.: callbacks.initTriggerButton:

gutenberg/packages/block-library/src/image/index.php

Line 249 in 0ed6351

data-wp-init="callbacks.initTriggerButton"

is not being called after the Image block is added to the page through a full-page navigation.

I think this should be fixed once #62734 merged, I'm not 100% sure.

[luisherranz]

callbacks.initTriggerButton won't be called after the Image block is added to the page through a full-page navigation

In the case of full-page navigation, we need to load the view.js files before making the HTML change, but this is an interesting pattern!

@DAreRodz: now that we are working on making the Interactivity API resilient to asynchronous loads, what should happen with a data-wp-init or data-wp-watch when the initial callback is undefined because the store is loading after hydration?

My first instinct tells me that once the store is loaded, the callbacks should be called, but that means making more parts of the store reactive.

[DAreRodz]

What should happen with a data-wp-init or data-wp-watch when the initial callback is undefined because the store is loading after hydration?

My first instinct tells me that once the store is loaded, the callbacks should be called, but that means making more parts of the store reactive.

Oh, I see. 😮

Yes, we need to execute those directives again once the callbacks are defined. This could mean that we need to make the whole store reactive...

[michalczaplinski]

I've edited my previous comment because it was somewhat unclear. I hope you managed to understand it regardless 🙂

Yes, we need to execute those directives again once the callbacks are defined. This could mean that we need to make the whole store reactive

Yes, exactly. I was hoping that as part of your work in #62734, the callbacks in data-wp-init or data-wp-watch are run once a block that contains them is added to the webpage "asynchronously" (via full-page navigation or other means).

[luisherranz]

the callbacks in data-wp-init or data-wp-watch are run once a block that contains them is added to the webpage "asynchronously" (via full-page navigation or other means).

As I mentioned in my previous comment, fixing that is independent of full-page client-side navigation. In full-page client-side navigation, since we are controlling the loading of all assets, we should wait for the JavaScript to execute before updating the DOM, just like we are doing with the CSS.

[michalczaplinski]

we need to load the view.js files before making the HTML change, but this is an interesting pattern!

In full-page client-side navigation, since we are controlling the loading of all assets, we should wait for the JavaScript to execute before updating the DOM, just like we are doing with the CSS.

I'm not following, could you explain it differently?

In the case discussed in this issue, the view.js file of the Image does load before any HTML is being updated.

[luisherranz]

We have been investigating, and indeed, the view.js file of the image block is being loaded after the HTML was updated.

The issue is that when copying the attributes of the element, the src attribute of the script was also being copied and when a script has a src, it ignores the code that is within the tag.

gutenberg/packages/interactivity-router/src/head.ts

Lines 91 to 93 in a47a25d

	for ( const attr of headElement.tag.attributes ) {
	element.setAttribute( attr.name, attr.value );
	}

[michalczaplinski]

Sooo, there are in fact several issues here 😅 :

1. Remove src attributes from prefetched scripts

This is the issue that @luisherranz has just mentioned above. We should remove the src attributes of the prefetched scripts because otherwise the contents are ignored. This should be sufficient:

diff --git a/packages/interactivity-router/src/head.ts b/packages/interactivity-router/src/head.ts
index 2bde7cea520..4d2c8142140 100644
--- a/packages/interactivity-router/src/head.ts
+++ b/packages/interactivity-router/src/head.ts
@@ -89,7 +89,10 @@ export const fetchHeadAssets = async (
 				const element = doc.createElement( tagName );
 				element.innerText = headElement.text;
 				for ( const attr of headElement.tag.attributes ) {
-					element.setAttribute( attr.name, attr.value );
+					// don't copy the src or href attribute
+					if ( attr.name !== 'src' && attr.name !== 'href' ) {
+						element.setAttribute( attr.name, attr.value );
+					}
 				}
 				headTags.push( element );
 			} )

2. use textContent instead of innerText

Once 1. is fixed, there is another problem which I have already mentioned in this comment. The script that is inserted in the the <head> appears to 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

With those two issues out of the way, 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.

This should be sufficient to achieve it:

diff --git a/packages/interactivity-router/src/index.ts b/packages/interactivity-router/src/index.ts
index 79b67eeb98e..c2dfb6ef23f 100644
--- a/packages/interactivity-router/src/index.ts
+++ b/packages/interactivity-router/src/index.ts
@@ -111,6 +111,7 @@ const regionsToVdom: RegionsToVdom = async ( dom, { vdom } = {} ) => {
 const renderRegions = ( page: Page ) => {
 	batch( () => {
 		if ( globalThis.IS_GUTENBERG_PLUGIN ) {
+			populateInitialData( page.initialData );
 			if ( navigationMode === 'fullPage' ) {
 				// Once this code is tested and more mature, the head should be updated for region based navigation as well.
 				updateHead( page.head );

4. We should 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

We should fix this by waiting for the load event on the script and only then updating the DOM. This had already been partly attempted by @DAreRodz in 9baff87 so I'll try to bring this into #64067

Bonus

I've also just noticed that we accidentally prefetch scripts that belong to browser extensions (which we should definitely not do). I will file a separate issue for this.

[luisherranz]

I've also just noticed that we accidentally prefetch scripts that belong to browser extensions (which we should definitely not do). I will file a separate issue for this.

The only scripts we should load are those belonging to the new interactive blocks on the page. We shouldn't load any other scripts, as no other scripts are guaranteed to work. We need to find a way to identify them.

[michalczaplinski]

The only scripts we should load are those belonging to the new interactive blocks on the page. We shouldn't load any other scripts, as no other scripts are guaranteed to work. We need to find a way to identify them.

Agreed, I just don't want to fix everything in one huge PR! 😅