-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Conversation
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
This pull request changed or added PHP files in previous commits, but none have been detected in the latest commit. Thank you! ❤️ |
Size Change: +22.6 kB (+1%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
const isValidLink = ( ref ) => | ||
ref && | ||
ref instanceof window.HTMLAnchorElement && | ||
ref.href && | ||
( ! ref.target || ref.target === '_self' ) && | ||
ref.origin === window.location.origin; |
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.
Also need to exclude wp-admin
links. See similar logic in Customizer preview: https://github.com/WordPress/wordpress-develop/blob/f525e665b6ea6e88289d82cad5fc1dfac57db109/src/js/_enqueues/wp/customize/preview.js#L283-L323
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.
Also wp-login.php
. See similar Speculative Loading plugin exclusions.
In fact, we're even providing a filter (though that could be added later here).
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.
Also important to check for whether the link is for an anchor link for another part of the page: https://github.com/WordPress/wordpress-develop/blob/f525e665b6ea6e88289d82cad5fc1dfac57db109/src/js/_enqueues/wp/customize/preview.js#L156-L160
So this here should also check if ! ref.getAttribute('href').startsWith('#')
.
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.
Thanks for the feedback and the links! I added the checks for wp-admin
, wp-login
, _wpnonce
, and anchors as part of this conditional. Let me know if that's not what you had in mind.
Co-authored-by: Michal <mmczaplinski@gmail.com>
Thanks a lot for all the feedback. I've been reviewing all the remaining comments and I believe this is how I will proceed: To do in this pull request
To improve after that
|
Flaky tests detected in 745e675. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8719955449
|
I believe this experiment is ready to be merged. I addressed all the feedback listed here. We should probably create a tracking issue for client-side navigation and add the follow-up tasks there, and potentially create specific issues for some of them. |
! ref.pathname.startsWith( '/wp-admin' ) && | ||
! ref.pathname.startsWith( '/wp-login.php' ) && |
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.
Sometimes WordPress sites install core in a subdirectory, like /wp/
. In such cases home_url()
and site_url()
are different, e.g. http://example.com/
vs https://example.com/wp/
.
Additionally, in multisite subdirectory installs, there will also be path segment(s) before /wp-admin
.
Easiest way to handle that right now is to just check if the path includes the strings:
! ref.pathname.startsWith( '/wp-admin' ) && | |
! ref.pathname.startsWith( '/wp-login.php' ) && | |
! ref.pathname.includes( '/wp-admin' ) && | |
! ref.pathname.includes( '/wp-login.php' ) && |
This is not as robust as possible. But better to err on the side of not doing client-side navigation than doing it with an invalid target.
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.
I’m simply reading the discussion, but seeing this solution would not work for my sites, because for security I always change the default wp-admin url. Maybe check for a class name or other attribute in the dom instead?
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.
Actually, these paths should be passed from PHP so they don't have to be hard coded in JS.
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.
You're right. Maybe we can use wp_interactivity_config to create a new isAdmin
property that is passed to the client reading is_admin in PHP.
cc: @DAreRodz Worth adding this to the tracking issue as a follow-up task?
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.
I don't think it would be is_admin()
since this would always just be false
in the frontend context. I think we'd need to instead get an array of the special paths, including get_admin_url()
, wp_login_url()
, etc.
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.
I've added this as a task on the tracking issue.
); | ||
// Prefetch on hover. | ||
document.addEventListener( | ||
'mouseenter', |
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.
On touch interfaces, I understand that the mouseenter
event does fire but after a 300 ms delay. To ensure that prefetch happens immediately on a touch device, I suggest adding the same handler for the touchstart
event. Or apparently pointerenter
will handle both of these and it is supported by all browsers: https://caniuse.com/mdn-api_element_pointerenter_event
// Once this code is tested and more mature, the head should be updated for region based navigation as well. | ||
if ( process.env.IS_GUTENBERG_PLUGIN ) { | ||
if ( navigationMode === 'fullPage' ) { | ||
// Cache the scripts. Has to be called before fetching the assets. |
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.
Why is the caching required? Is this to guarantee execution order of the scripts when the page is loaded? What if a script is not cacheable?
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.
Or is this rather to ensure that there is no flash of unrendered content?
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.
I believe that the original idea was simply that when doing a full-page navigation to another page, we don't want to re-download the scripts that were already present on the initial page.
If a script is not cacheable (e.g. if some of the script's contents are generated dynamically) that's indeed currently a problem.
One solution could be examine the HTTP headers and e.g. skip the ones with Cache-Control: no-store
, Cache-Control: no-cache
or Expires: <Date in the past>
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.
Thanks, @michalczaplinski, for chiming in. 😄
We want to revisit the whole assets system, and examining HTTP headers is something we could try.
BTW, I've created a tracking issue to continue with the development. #60951
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.
Great! Indeed, we should take a holistic view of this. Thanks for creating the issue! 🙏
Thanks everyone for your work and feedback. 🙌 I've created this tracking issue to keep track of the progress and continue working on this feature. The list of tasks is based on all the feedback gathered from this PR. |
@michalczaplinski We shouldn't really load unknown scripts. We just did it to get it working with the WP movies demo. We should only load the module scripts that belong to the new blocks on the page or other module scripts that have declared their compatibility with full-page client-side navigation. |
I arrive a bit late to the party, but I simply want to say incredible work here. Thanks to Mario for leading this and to the rest for all the valuable feedback.This is very exciting!! 👏😄 |
What?
This pull request adds an experiment in Gutenberg -> Experiments to enable full page client-side navigation in your site using the Interactivity API.
It enables the user experience showcased in the State Of The Word and in reflected in the Movies demo.
Client-side navigation refers to the process of navigating between different pages without the need for a full page refresh. The Interactivity API takes care of fetching the new content from the server and replacing the relevant parts in the DOM, while maintaining the UI state. This approach provides a SPA experience, which is smoother and opens up new possibilities.
One of the limitations of this approach is that all the scripts must be compatible with the Interactivity API, and that's why it can only be enabled in controlled environments until the API is a bit more mature. Additionally, we need to explore deeper how to handle new scripts and styles.
We can differentiate between two types of client-side navigation:
Region based client-side navigation (existing router implementation)
This type of navigation was included as a first step and it just replaces specific regions of the DOM. This is safer because just the selected regions must be compatible with the Interactivity API. This is used, for example, to enhance the pagination of the Query Loop block.
Full page client-side navigation (enabled by this experiment)
This type of navigation does replace the whole DOM and not just specific regions. As mentioned above, there might be incompatibilities with other scripts, so it should only be used in controlled environments. This experiment should help to clarify what is needed and how it could be implemented.
Why?
The idea is to include it as experimental to start testing it and explore what needs to be done to offer it as a site option without the experimental flag.
How?
There are a few changes in this pull request:
data-wp-interactive
to all the root blocks: link.interactivity-router
module namedfull-csn
to handle the whole site and not only regions: link. It is based on the one there was in the initial experiments: link.Testing Instructions