-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Experiment: Add full page client-side navigation experiment setting #59707
Changes from 52 commits
6a1b29a
a5b6dd0
cf8e72b
8ddc122
32cd62c
cc25473
dffa452
7412f98
f92355d
04b94a9
ca90d0a
34e6b22
4f67b70
592b842
1593483
afda3e6
195e7e7
c8348ab
69a70d6
9b2c853
8f16b30
7f458b1
1d96c64
63c81a6
652a2a2
442248c
0f3bc44
62f6948
a26bfa7
a5a7279
f984294
0f7a8db
cd7a112
dd2b73e
7eef1ce
f0fecee
7081358
1d00a3c
a318179
2138f0d
79eac03
5dcaa1d
fb5f911
afa7127
79768b6
181614e
36e0901
455b8d7
a2c1a07
f7e5212
8f09d2b
743926f
44d0873
eeb6491
46ce031
57ef83c
a5b9e80
4f9d728
d7e6ddb
d72cd02
745e675
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,60 @@ | ||||||||||||||||||
<?php | ||||||||||||||||||
/** | ||||||||||||||||||
* Registers full page client-side navigation option using the Interactivity API and adds the necessary directives. | ||||||||||||||||||
*/ | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* Enqueue the interactivity router script. | ||||||||||||||||||
*/ | ||||||||||||||||||
function _gutenberg_enqueue_interactivity_router() { | ||||||||||||||||||
// Set the navigation mode to full page client-side navigation. | ||||||||||||||||||
wp_interactivity_config( 'core/router', array( 'navigationMode' => 'fullPage' ) ); | ||||||||||||||||||
wp_enqueue_script_module( '@wordpress/interactivity-router' ); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
add_action( 'wp_enqueue_scripts', '_gutenberg_enqueue_interactivity_router' ); | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* Set enhancedPagination attribute for query loop when the experiment is enabled. | ||||||||||||||||||
* | ||||||||||||||||||
* @param array $parsed_block The parsed block. | ||||||||||||||||||
* | ||||||||||||||||||
* @return array The same parsed block with the modified attribute. | ||||||||||||||||||
*/ | ||||||||||||||||||
function _gutenberg_add_enhanced_pagination_to_query_block( $parsed_block ) { | ||||||||||||||||||
if ( 'core/query' !== $parsed_block['blockName'] ) { | ||||||||||||||||||
return $parsed_block; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
$parsed_block['attrs']['enhancedPagination'] = true; | ||||||||||||||||||
return $parsed_block; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
add_filter( 'render_block_data', '_gutenberg_add_enhanced_pagination_to_query_block' ); | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* Add directives to all links. | ||||||||||||||||||
* | ||||||||||||||||||
* Note: This should probably be done per site, not by default when this option is enabled. | ||||||||||||||||||
* | ||||||||||||||||||
* @param array $content The block content. | ||||||||||||||||||
* | ||||||||||||||||||
* @return array The same block content with the directives needed. | ||||||||||||||||||
*/ | ||||||||||||||||||
function _gutenberg_add_client_side_navigation_directives( $content ) { | ||||||||||||||||||
$p = new WP_HTML_Tag_Processor( $content ); | ||||||||||||||||||
while ( $p->next_tag( array( 'tag_name' => 'a' ) ) ) { | ||||||||||||||||||
if ( empty( $p->get_attribute( 'data-wp-on--click' ) ) ) { | ||||||||||||||||||
$p->set_attribute( 'data-wp-on--click', 'core/router::actions.navigate' ); | ||||||||||||||||||
} | ||||||||||||||||||
if ( empty( $p->get_attribute( 'data-wp-on--mouseenter' ) ) ) { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about mobile devices? Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK no, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is also a good example for where multiple actions should be able to be specified for a given event, as we've been discussing in #60574. There should not be a requirement that the $p->set_attribute( 'data-wp-on--mouseenter--core-router', 'core/router::actions.prefetch' ); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe let's wait until #60661 has landed, then we can fix these attributes to be unconditionally added, using a suffix for uniqueness. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a follow-up task to this list to explore and decide how to interact with mobile events. |
||||||||||||||||||
$p->set_attribute( 'data-wp-on--mouseenter', 'core/router::actions.prefetch' ); | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per @gziolo's comment on https://make.wordpress.org/core/2024/04/09/speculative-loading-in-wordpress/, if we eventually move forward with adopting the Speculation Rules API in WordPress core, we would need to consider whether or how this conflicts with it. FWIW the Speculation Rules API is a web technology, and if it allows us to replace this manual implementation, that would probably be a good thing. That said, I'm not sure how it would behave here given that we don't do a "real" navigation when those links are clicked but client-side navigation. It might just work out of the box, but that would need to be tested. I'm mostly commenting for visibility, not something we need to cater for at the moment, as it's going to be a while. But once this has landed in Gutenberg, we may actually want to try using it in combination with the https://wordpress.org/plugins/speculation-rules/ plugin to see what happens. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, speculation rules would most likely need to be disabled on sites with full page client-side navigation. This is because each link click would have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potentially we don't have to completely disable them. There may be ways to configure the rules so that any This wouldn't be helpful if the entire page is using them, but for pages partially using this it would give the best of both worlds. Worth trying when we actually start experimenting with the integration. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just put together a quick test of how Speculation Rules work with client-side navigation: https://speculation-rules-with-client-side-navigation.glitch.me/
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction: It seems prerendering will add the subresources to the cache, but the prerendered HTML is not served from the prefetch cache for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction 2: It seems Glitch sending There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a follow-up task to this list to explore deeper how both projects interact after this experiment. |
||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
// Hack to add the necessary directives to the body tag. | ||||||||||||||||||
// TODO: Find a proper way to add directives to the body tag. | ||||||||||||||||||
return (string) $p . '<body data-wp-interactive="core/experimental" data-wp-context="{}">'; | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could use a static variable here to add the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That'd be important to ensure it gets added only once. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this injecting an additional <!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
</head>
<body>
<h1>
Hello World!
</h1>
<body class="hey">
</body>
</html> Surprisingly, this actually works: Maybe I shouldn't be surprised given HTML's loose parsing rules. But I can imagine other plugins that try to parse the HTML to, for example, apply various optimizations would get might confused when encountering multiple Shouldn't the attributes rather get injected on the existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added the check in this commit and I added a follow-up task in this list to explore how to do this properly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opened PR to address this: #61212 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, thank you for sharing @westonruter. |
||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// TODO: Explore moving this to the server directive processing. | ||||||||||||||||||
add_filter( 'render_block', '_gutenberg_add_client_side_navigation_directives' ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
/** | ||
* Helper to update only the necessary tags in the head. | ||
* | ||
* @async | ||
* @param {Array} newHead The head elements of the new page. | ||
* | ||
*/ | ||
export const updateHead = async ( newHead ) => { | ||
// Helper to get the tag id store in the cache. | ||
const getTagId = ( tag ) => tag.id || tag.outerHTML; | ||
|
||
// Map incoming head tags by their content. | ||
const newHeadMap = new Map(); | ||
for ( const child of newHead ) { | ||
newHeadMap.set( getTagId( child ), child ); | ||
} | ||
|
||
const toRemove = []; | ||
|
||
// Detect nodes that should be added or removed. | ||
for ( const child of document.head.children ) { | ||
const id = getTagId( child ); | ||
// Always remove styles and links as they might change. | ||
if ( child.nodeName === 'LINK' || child.nodeName === 'STYLE' ) | ||
toRemove.push( child ); | ||
Comment on lines
+23
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the stylesheet remains the same, I think it should be left as-is. Otherwise, removing a stylesheet and then adding it right back in will likely cause a performance problem. Example: https://mousy-citrine-lotus.glitch.me/ Note there is a flash of unstyled content. It also causes layout and style recalculation: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to be able to reuse DOM diffing from Preact here. (Not sure if that's possible.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a follow-up task to this list to explore this after this experiment is merged. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried using Preact at first, but if the scripts and styles change order or new elements appear in the head, Preact gets confused and deletes and re-adds everything. I think it will be simpler and more stable if we control it ourselves. |
||
else if ( newHeadMap.has( id ) ) newHeadMap.delete( id ); | ||
else if ( child.nodeName !== 'SCRIPT' && child.nodeName !== 'META' ) | ||
toRemove.push( child ); | ||
} | ||
|
||
// Prepare new assets. | ||
const toAppend = [ ...newHeadMap.values() ]; | ||
|
||
// Apply the changes. | ||
toRemove.forEach( ( n ) => n.remove() ); | ||
document.head.append( ...toAppend ); | ||
}; | ||
|
||
/** | ||
* Fetches and processes head assets (stylesheets and scripts) from a specified document. | ||
* | ||
* @async | ||
* @param {Document} document The document from which to fetch head assets. It should support standard DOM querying methods. | ||
* @param {Map} headElements A map of head elements to modify tracking the URLs of already processed assets to avoid duplicates. | ||
* | ||
* @return {HTMLElement[]} Returns an array of HTML elements representing the head assets. | ||
SantosGuillamot marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
export const fetchHeadAssets = async ( document, headElements ) => { | ||
SantosGuillamot marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const headTags = []; | ||
const assets = [ | ||
{ | ||
tagName: 'style', | ||
selector: 'link[rel=stylesheet]', | ||
attribute: 'href', | ||
}, | ||
{ tagName: 'script', selector: 'script[src]', attribute: 'src' }, | ||
]; | ||
for ( const asset of assets ) { | ||
const { tagName, selector, attribute } = asset; | ||
const tags = document.querySelectorAll( selector ); | ||
|
||
// Use Promise.all to wait for fetch to complete | ||
await Promise.all( | ||
Array.from( tags ).map( async ( tag ) => { | ||
const attributeValue = tag.getAttribute( attribute ); | ||
if ( ! headElements.has( attributeValue ) ) { | ||
const response = await fetch( attributeValue ); | ||
SantosGuillamot marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const text = await response.text(); | ||
headElements.set( attributeValue, { | ||
tag, | ||
text, | ||
} ); | ||
} | ||
|
||
const headElement = headElements.get( attributeValue ); | ||
const element = document.createElement( tagName ); | ||
element.innerText = headElement.text; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For styles we should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added it as a follow-up to explore after this experiment. |
||
for ( const attr of headElement.tag.attributes ) { | ||
element.setAttribute( attr.name, attr.value ); | ||
} | ||
headTags.push( element ); | ||
} ) | ||
); | ||
} | ||
|
||
return [ | ||
document.querySelector( 'title' ), | ||
...document.querySelectorAll( 'style' ), | ||
...headTags, | ||
]; | ||
}; |
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.
For what it is worth, I think we should use event delegation for this. Something similar to this, but adding our checks (if the element already has a
data-wp-on--click
...): https://gist.github.com/devongovett/919dc0f06585bd88af053562fd7c41b7There 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.
That's a good idea. I guess it makes sense if we address that in a follow-up PR, right?
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.
@felixarntz related to the discussion in #60574 regarding async event handlers, if event delegation were employed like this above Gist does, then this is another area where synchronous event handlers can negatively impact INP. It would be likely that analytics plugins would adopt similar event delegation rather than having to use the tag processor to inject their own
click
event directives on each link.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.
For this experiment, I added this commit to use
document.addEventListener
instead of the previous directives. I also added a follow-up task to this list to explore this more deeply.