Categories Block: Add iAPI directive for client-side routing

[ockham]

What?

Enable client-side navigation for the Categories block, when used with the Query Loop block.

Why?

For a better user experience when using the Categories block.

How?

By adding the Interactivity API's data-wp-on--click directive and setting it to the core/query::actions.navigate action. (The latter is provided by the Query Loop block.)

Testing Instructions

• Create a few posts and at least two categories. Assign each category to a couple of posts.
• In an archive template (e.g. Category Archives), add the Categories block inside of the Query Loop block (if it's not already there).
• Make sure that the Query Loop block has the "Enable forced reload" toggle disabled. To that end, you might need to modify the Post Template block to only contain allowed blocks (e.g. Post Title, Excerpt, Featured Image; but not Post Content).
• View on the frontend. Verify that clicking on individual categories in the Categories block triggers a client-side navigation (rather than server-side). You can use your browser's Network tab to verify.
• Go back to the Site Editor, and enable the Query Loop block's "Force Page Reload" toggle. Save the template.
• Go back to the frontend, and reload it. Verify that clicking the category links now indeed cause a full page reload.

Screenshots or screencast

[gziolo]

Wouldn't it be the responsibility of the Query Loop block to intercept all clicks to links instead when the proper setting is enabled? Otherwise, the same change would have to be applied to every possible block that contains link.

[michalczaplinski]

Indeed. We should add the data-wp-on-click directives in the Query block's render callback.

However, in order to find where to add them, we should also add some "markers" here (in the Categories block).

One solution could be something like adding data-wp-navigation-link attributes (which were removed in #57853):

// Categories block
while ( $p->next_tag( 'a' ) ) {
	$p->set_attribute( 'data-wp-navigation-link', );
}

// Query block
while ( $p->next_tag( 'a' ) ) {
	if ( $p->get_attribute('data-wp-navigation-link') ) {
	  $p->set_attribute( 'data-wp-on--click', 'core/query::actions.navigate' );
	}
}
$items_markup = $p->get_updated_html();

[ockham]

Thanks, folks, that's exactly the kind of feedback I was looking for!

@gziolo wrote:

Wouldn't it be the responsibility of the Query Loop block to intercept all clicks to links instead when the proper setting is enabled? Otherwise, the same change would have to be applied to every possible block that contains link.

@michalczaplinski wrote:

Indeed. We should add the data-wp-on-click directives in the Query Loop block's render callback.
However, in order to find where to add them, we should also add some "markers" here (in the Categories block).

I like the idea of adding the directives from within the Query block's render callback, as that gives us access to the information whether enhanced pagination is on or off.

However, we need to decide on an overall strategy:

• Doing it wholesale for all links found inside of the Query Loop block, as @gziolo suggested, or
• Applying it only to individual blocks that have opted in, e.g. via some sort of marker (another data-wp- attribute), as suggested by @michalczaplinski.

I think both approaches have their tradeoffs:

• Doing it wholesale might erroneously apply to links that point other parts of the site (that use a different template and would thus break region-based client-side navigation), or even external links. OTOH.
• Requiring opt-in adds some overhead. Furthermore, in terms of how blocks opt into this behavior, I don't really love the idea of using another data-wp attribute just to tell WP to replace it with data-wp-on--click='core/query::actions.navigate'. (To me, it raises the question what we're gaining from that, vs. having the block set the data-wp-on--click attribute itself.)

We might be able to tweak both strategies a bit:

• Wholesale: We could make sure to only add the data-wp-on--click handler to links that point to routes that are handled by the same template. (This could be a bit tricky in practice due to WP's template hierarchy; the system would need to know a lot about routes and templates.)
• Opt-in: Instead of setting a marker on each instance of a given block; can we just opt the entire block type in, i.e. set a block attribute or something of the sort?

[ockham]

I see that the Query Loop block provides enhancedPagination context, which is used by the Post Template block. This looks promising to me, as it might allow us to minimize the extra information we introduce and/or communicate. It would still leave the responsibility of setting data-wp-on--click='core/query::actions.navigate' attribute with individual blocks, but based on the above, that might be reasonable.

[ockham]

(Note that not all blocks will require the HTML API to inject the attribute; we need it for the core/categories block as that block gets its HTML from wp_list_categories() and wp_dropdown_categories, respectively. That HTML is opaque to us, so we have to resort to the HTML API in order to modify it; but in other cases, we'll be able to simply include the data-wp-on--click attribute in the HTML string we return from the block.)

[ockham]

Tried it in d9dc1b7. I kinda like this 😬

[luisherranz]

I don't have much to add here.

• It's true that click handlers cannot be added to all the clicks within the Query loop because there are links that may point to other parts of the site.
• I don't have a strong opinion on how to include those click handlers, but in my opinion, the way we are currently doing it, passing whether the enhanced pagination is active through block context and injecting the click handlers into each block, seems simple and appropriate for now.
• Sometimes, there will be more complex cases where a click-handler is not enough, and more logic needs to be added, such as with the Search block, so it's important to leave room for custom implementations.
  • Query and Search blocks: support for Instant Search #63147

[ockham]

Thank you very much for weighing in, @luisherranz!

I'll go ahead and open this for review then 😊

[gziolo]

I left more thoughts related to that topic #64907 (comment) in response to the #64907 (review) from @dmsnell.

[github-actions]

Flaky tests detected in d9dc1b7.
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/10621473315
📝 Reported issues:

• [Flaky Test] Should insert content using the global inserter #57525 in /test/e2e/specs/widgets/editing-widgets.spec.js

[github-actions]

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: ockham <[email protected]>
Co-authored-by: gziolo <[email protected]>
Co-authored-by: michalczaplinski <[email protected]>
Co-authored-by: luisherranz <[email protected]>
Co-authored-by: dmsnell <[email protected]>

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

[dmsnell]

I'm a bit confused on this and that's probably because I don't understand the issues involved, but why is this being done on the server? it doesn't appear to impact the rendered page at all other than by adding a flag that client-side JS code is going to have to find and deal with anyway.

could we not identify the category-containing HTML on page load/DOM ready and then apply the associated click handler there? the benefit to that approach is that it's much easier to detect whether a link is local or external, since the .href property of each A element will be normalized and complete.

if it still requires JS to work anyway, could we leave it all in JS? or possibly use this code to add some class to the containing element like this?

if ( ! empty( $block->context['enhancePagination'] ) ) {
	$processor = new WP_HTML_Tag_Processor( $content );
	if ( $processor->next_tag() ) {
		$processor->add_class( 'uses-enhanced-pagination' );
	}
}

In the client:

document
	.querySeletorAll( '.uses-enhanced-pagination a' )
	.forEach( node => {
		const linkHost = (new URL(node.href)).domain;
		if ( document.location.host !== linkHost ) {
			return;
		}

		node.dataset['wpOn-Click'] = 'core/query::actions.navigate';
	} );

[gziolo]

I'm a bit confused on this and that's probably because I don't understand the issues involved, but why is this being done on the server? it doesn't appear to impact the rendered page at all other than by adding a flag that client-side JS code is going to have to find and deal with anyway.

I had the same thoughts when commenting here. I didn't elaborate as much as @dmsnell, but I agree on principle that it isn't the most efficient to run 3 steps of processing to properly handle links in the block embeded in the Query loop. In the currently proposed approach the following things happen:

• WP_HTML_Tag_Processor needs to be instantiated within render_callback to iterate over HTML to inject directives
• during directives processing, it's very likely that these directives needs to be visited, although properly they don't get any special treatment
• Interactivity API runtime needs to find these directives and attach proper events

Plus, for every possible block, it's up to the author to ensure that links are correctly integrated.

I would also like to point out that the full-page client-side nagivation experiment automatically applies the proper events by detecting all applicable links:

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

Lines 366 to 395 in 9d4f918

	// Add click and prefetch to all links.
	if ( globalThis.IS_GUTENBERG_PLUGIN ) {
	if ( navigationMode === 'fullPage' ) {
	// Navigate on click.
	document.addEventListener(
	'click',
	function ( event ) {
	const ref = ( event.target as Element ).closest( 'a' );
	if ( isValidLink( ref ) && isValidEvent( event ) ) {
	event.preventDefault();
	actions.navigate( ref.href );
	}
	},
	true
	);
	// Prefetch on hover.
	document.addEventListener(
	'mouseenter',
	function ( event ) {
	if ( ( event.target as Element )?.nodeName === 'A' ) {
	const ref = ( event.target as Element ).closest( 'a' );
	if ( isValidLink( ref ) && isValidEvent( event ) ) {
	actions.prefetch( ref.href );
	}
	}
	},
	true
	);
	}
	}

In effect, all necessary logic is in place to replicate that for region-based client-side navigation within the Query Loop. The only difference in this case would be that there should be some way to configure how to detect which URL should be handled without the page reload, as that should be narrowed down to the pattern used by the Query loop block.

If necessary, there could be a way to opt out of the block or individual link from the default behavior, for example, through supports.interactivity flags.

[ockham]

Thanks @dmsnell and @gziolo! I'll answer Dennis' comment first, and will reply to Grzegorz's separately.

I'm a bit confused on this and that's probably because I don't understand the issues involved, but why is this being done on the server?

Primarily because that's how it's done by some existing blocks (Query Pagination Previous/Numbers/Next) -- so it seemed to be the agreed-upon way of using the iAPI. However, I understand your concerns!

could we not identify the category-containing HTML on page load/DOM ready and then apply the associated click handler there? the benefit to that approach is that it's much easier to detect whether a link is local or external, since the .href property of each A element will be normalized and complete.

But that distinction (local vs external) isn't quite enough, is it? If a link is local but points to a different part of the site that requires additional styling or scripts, client-side navigation would yield a visually broken result, no? (For the iAPI, folks are tackling this problem under the umbrella term of full-page -- vs region-based -- client-side navigation.)

Being limited to region-based client-side navigation (for the time being) is one of the main reasons I was opting to do it on a per-block basis (also see this discussion).

LMK if that makes sense, and/or if I'm missing something!

[ockham]

I agree on principle that it isn't the most efficient to run 3 steps of processing to properly handle links in the block embeded in the Query loop.

Yeah, that's fair enough. I guess it becomes more apparent if one looks at how the same problem would be solved with vanilla JS -- where we'd only require a simple onclick handler and no further processing 🤔

(Aside: I'm wondering if that raises some questions around using the iAPI in Core. On the one hand, I thought it would be a good way to provide developers with some example code to look at; plus given the fact that it's already used in Query Pagination blocks, I thought that it'd make sense to use it somewhat consistently. OTOH, I hear your and Dennis' performance and complexity concerns.)

I would also like to point out that the full-page client-side nagivation experiment automatically applies the proper events by detecting all applicable links: [...]

Right, I was unaware. Thank you for pointing that out!

In effect, all necessary logic is in place to replicate that for region-based client-side navigation within the Query Loop. The only difference in this case would be that there should be some way to configure how to detect which URL should be handled without the page reload, as that should be narrowed down to the pattern used by the Query loop block.

Yeah, and that's the part that looked fairly tricky to me: As long as we're confined by region-based client-side navigation, the criterion is basically if a link's target URL would be rendered by the same template that's currently being displayed. The way that WP's routing and querying works, I'm not sure it's easily possible to find that out without actually rendering that route 😕

[gziolo]

Primarily because that's how it's done by some existing blocks (Query Pagination Previous/Numbers/Next) -- so it seemed to be the agreed-upon way of using the iAPI. However, I understand your concerns!

I wasn’t aware of that implementation detail. Thank you for sharing this context. I see it’s hardcoded to core/query::actions.navigate. This works perfectly fine for Query navigation links that can’t be located outside of the Query block. One part that is tricky is that Query block uses enhancedPagination context to control the feature which isn’t namespaced. In effect, it shouldn’t be used for other containers with region-based navigation to ensure the onclick handler doesn’t accidentally gets wired with the incorrect namespace. Alternatively, enhancedPagination should explicitly pass the namespace like core/query. Unless, we keep assuming that Query is the only supported region and everything needs to be hardcoded accordingly. I’m worried that current approach is too rigid.

In case you want to iterate quickly and add the missing functionality then the approach taken in this PR is reasonable as it follows the well-established pattern. I raised all my concerns with the intent to spark the discussion to improve the entire design over time. In particular to align closer two types of client-side navigation that should morph into one solution in the long run.

[ockham]

Thank you @gziolo! I hope I didn't win your approval through my verbosity 😅
If anything, I was hoping to use existing tooling to scratch a (minor) itch that I'd encountered working on a real-life project; I'm definitely open to replacing this with a better solution once we have e.g. full-page client-side navigation available. I just didn't want to wait for that to happen before making this fix 😄

[dmsnell]

But that distinction (local vs external) isn't quite enough, is it? If a link is local but points to a different part of the site that requires additional styling or scripts, client-side navigation would yield a visually broken result, no?

This is the part that's beyond my familiarity, though it still seems like something worth double- or triple-checking. Is there not some way to classify at the group level which links should be activated? For example, how does the server know if a link is set to the same "region"? I'm guessing it's not path-related.

Just caught my eye since we're doing stuff that only works when JavaScript runs, but we're doing that processing on render in the server.

[fabiankaegy]

Hey @ockham 👋

Would you be able to help write a dev note for this for the 6.7 release? We are planning to have this as part of a larger Miscellaneous Editor Updates note.

We are hoping to get all drafts in by October 13th to leave some time for reviews before the RC1.

All Dev Notes get tracked in #65784 so feel free to leave a note there or ping me directly :)

Please let us know if you can assist with that.

Thanks in advance :)

[ockham]

@fabiankaegy Hmm, I'm not sure that this is a change that needs a Dev Note, as it's more user-facing? Essentially, it enables client-side navigation for the links in the Categories Block, provided that it's a descendent of a Query Loop block that has opted into client-side navigation by disabling the "Reload full page" toggle (previously labeled "Force page reload", see #65081).

[fabiankaegy]

@ockham okay in that case let me remove it from the board :)

Question though, how would developers / users find out about the new capabilities? There is no UI for any of this so I think it is really hard to discover. That is in part why I thought it would be good to call out. As it it something that developers no longer need to custom build but rather can use the core block for now.

[ockham]

@ockham okay in that case let me remove it from the board :)

Question though, how would developers / users find out about the new capabilities? There is no UI for any of this so I think it is really hard to discover. That is in part why I thought it would be good to call out. As it it something that developers no longer need to custom build but rather can use the core block for now.

Fair question :) I was thinking it would bubble up via a different path, e.g. via GB release notes (that are used to compile user-facing updates for a WP release)? But TBH that's speculation on my part; I don't really know what's the established process for changes like that. Can you enlighten me? (FWIW, there's also this -- somewhat major -- change that would be nice to have documented in 6.7)

In any case, if we'd like to document the changes from the present PR somehow, maybe something like the following would do:

Categories Block: Conditionally enable client-side routing

If a Categories block is used inside a Query Loop block whose "Reload full page" toggle is disabled, the Categories block's links will now use client-side routing. (This is achieved by adding the Interactivity API's data-wp-on--click directive and setting it to the core/query::actions.navigate action, which is provided by the Query Loop block.)

[fabiankaegy]

@ockham Yeah I don't really know the answer either. I don't think there is an established process...

Kind of because I don't know I was thinking to just include a few more of these types of things in the Field Guide. Just to have a space where we can talk about them