Experiment: Add full page client-side navigation experiment setting #59707
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SantosGuillamot
added
[Type] Experimental
SantosGuillamot
requested review from
spacedmonkey,
luisherranz,
DAreRodz and
ajitbohra
as code owners
|
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
|
westonruter
reviewed
Apr 10, 2024
Comment on lines
155
to
160
| 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.
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.
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.
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.
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. |
westonruter
reviewed
Apr 17, 2024
Comment on lines
+164
to
+165
| ! ref.pathname.startsWith( '/wp-admin' ) && | ||
| ! ref.pathname.startsWith( '/wp-login.php' ) && |
There was a problem hiding this comment.
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:
Suggested change
| ! 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.
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.
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.
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.
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.
I've added this as a task on the tracking issue.
westonruter
reviewed
Apr 17, 2024
| ); | ||
| // Prefetch on hover. | ||
| document.addEventListener( | ||
| 'mouseenter', |
There was a problem hiding this comment.
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
westonruter
reviewed
Apr 17, 2024
| // 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.
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.
Or is this rather to ensure that there is no flash of unrendered content?
There was a problem hiding this comment.
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.
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.
Great! Indeed, we should take a holistic view of this. Thanks for creating the issue! 🙏
14 tasks
|
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!! 👏😄 |
20 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-interactiveto all the root blocks: link.interactivity-routermodule namedfull-csnto handle the whole site and not only regions: link. It is based on the one there was in the initial experiments: link.Testing Instructions