-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Insert block template skip link via HTML API, minify CSS, remove JS. #10676
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
base: trunk
Are you sure you want to change the base?
Insert block template skip link via HTML API, minify CSS, remove JS. #10676
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
westonruter
left a comment
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 to see this!
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
…into refact/insert-skip-link-via-html
westonruter
left a comment
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.
This is looking great! I'll await a second review from @dmsnell before committing.
src/wp-includes/block-template.php
Outdated
| * Inserts the block template skip link into the template HTML. | ||
| * | ||
| * Uses the HTML API to ensure that the main content element has an ID and to | ||
| * inject the skip-link anchor before the block template wrapper. |
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.
as much as I love this, I’m not sure we need to describe in the docblock how the function operates. we can probably clarify the goal, which is well-stated here, and give an illustrative example.
/**
* ...
*
* When a `MAIN` element exists in the template, this function will ensure
* that the element contains a `id` attribute and will insert a link to
* that main element at the top of the first `DIV.wp-site-blocks` match.
*
* Example:
*
* // Input.
* <main>
* <nav>...</nav>
* <div class="wp-site-blocks">
* <h2>...
*
* // Output.
* <main id="wp--skip-link--target">
* <nav>...</nav>
* <div class="wp-site-blocks">
* <a href="#wp--skip-link--target" id="wp-skip-link" class="...">
* <h2>...
*
* When the `MAIN` element already contains a non-empty `id` value it will be
* used instead of the default skip-link id.
src/wp-includes/block-template.php
Outdated
| // Back-compat for plugins that disable functionality by unhooking this action. | ||
| if ( ! has_action( 'wp_footer', 'the_block_template_skip_link' ) ) { | ||
| return $template_html; | ||
| } |
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.
It seems like we could move this check up into get_the_block_template_html() which would leave this function considerably more declarative and stable. with the check here it’s leading us to create the awkward Closure-passing for test setup because the function is stateful with the system.
src/wp-includes/block-template.php
Outdated
| $skip_link_target_id = 'wp--skip-link--target'; | ||
| $processor->set_attribute( 'id', $skip_link_target_id ); | ||
| } | ||
| } |
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.
if we’re going to default to a given value, we could set it first and only change if we find one already exists.
// Only add skip-links to templates with a MAIN element.
if ( ! $processor->next_tag( 'MAIN' ) ) {
return $template_html;
}
$target_id = $processor->get_attribute( 'id' );
if ( ! is_string( $target_id ) && '' === $target_id ) {
$target_id = 'wp--skip-link--target'
$processor->set_attribute( 'id', $target_id );
}
...Note too that I did not use trim() in my example. This is a secondary mention, but it’s not appropriate for us to trim() an id attribute’s value, as “Identifiers are opaque strings.” (HTML spec).
we can see differentiation between id values with different whitespace.
<p id="me">one</p>
<p id=" me">two</p>
<p id="me ">three</p>
<p id=" me ">four</p>
<style>
#me {
border: 1px solid blue;
}
#\20me {
border: 1px solid green;
}
#me\20 {
border: 1px solid red;
}
#\20me\20 {
border: 1px solid orange;
}
</style>In other words, the trim() will cause WordPress to misidentify the id and break the skip-link any time it would ever make a difference in the code.
|
@rutviksavsani Could you address that great feedback from Dennis? |
| 'action_removed' => array( | ||
| 'set_up' => static function () { | ||
| remove_action( 'wp_footer', 'the_block_template_skip_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.
GitHub appears to have lost my comment again so I’ll try and resubmit it here, but I might be missing things this second time around.
I find this Closure-passing to be a bit confusing and opaque, particularly because the test code doesn’t give any clue as to what’s happening. it’s also interesting that only a single test case has setup code and only does a single thing.
one way to clear this up would be to pass the setup intention as a name, like with-filter or without-filter. but I think another step would be even clearer, since it appears that the goal is to ensure this doesn’t add a skip link under certain circumstances: split the test into separate functions.
public function test_block_template_properly_adds_skip_link( string $template_html, string $html_with_skip_link ) {
}
public function test_block_template_only_adds_skip_link_when_main_element_present() {
$template_html = "<main>...</main>";
...
$this->assertEqualMarkup(
"<main id='...'>...<div class='...'><a href='...'>",
_block_template_skip_link_markup( $template_html )
);
$template_html = "..."; // without MAIN
...
$this->assertSame(
$template_html,
_block_template_skip_link_markup( $template_html )
);
}
public function test_block_template_only_adds_skip_link_when_filter_present() {
$template_html = "<main>...</main>";
$this->assertEqualMarkup( ... );
remove_action( 'wp_footer', 'the_block_template_skip_link' );
$this->assertSame( $template_html, get_the_block_template_html() );
}I’ve omitted important details in this code snippet because I only wanted to show the idea of creating separate named tests to assert specific behaviors. in this way, the tests that fail will more closely match the behaviors that change and the test setup will/should be clearer.
obviously if the has_action() check moves up into get_the_block_template_html() function there’s an extra bit of setup code to make sure we can inject the $template_html into it, but that leaves _block_template_skip_link_markup() easier to test in isolation. this is a tradeoff that’s less significant than splitting the test.
|
@westonruter @dmsnell I have implemented the suggestions with best of my understandings, Let me know if I missed anything. |
dmsnell
left a comment
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.
Coming along quite well. I don‘t mean to delay this any more, but I left a couple of comments for your consideration.
Combining the processors into one, using the subclass, could have meaningful performance implications given that we are currently parsing the part of the post twice, up until the MAIN element.
| $inserter->insert_before( $skip_link ); | ||
| break; | ||
| } | ||
| } |
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.
it’s now more clear to me that this entire loop can be collapsed into another single if because we’re looking unconditionally for the first DIV.wp-site-blocks. That means the built-in query fully supports our goal.
if ( $inserter->next_tag( array( 'tag_name' => 'DIV', 'class_name' => 'wp-site-blocks' ) ) ) {
$skip_link = ...
$inserter->insert_before( $skip_link );
break;
}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.
yes, this makes sense, implementing it in the next commit.
| $this->set_bookmark( 'here' ); | ||
| $this->lexical_updates[] = new WP_HTML_Text_Replacement( $this->bookmarks['here']->start, 0, $text ); | ||
| } | ||
| }; |
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.
One fun thing about subclassing is that you can use all the existing mechanisms of the underlying HTML API. If you wanted to, instead of creating a separate processor for finding the MAIN id and then inserting a skip link, we could simply create $processor itself as the subclass instance.
There are two added benefits I see:
- Don’t need to create
$template_html = $processor->get_updated_html(). - Don’t need to double-traverse the entire post to find the
DIVagain.
Of course, if we expect the DIV.wp-site-blocks to appear before the MAIN then we have to start over our traversal (though we could bookmark it on the way to finding the MAIN element), but I think that’s moot because we probably expect it to follow the MAIN don’t we?
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.
We do actually expect DIV.wp-site-blocks to appear before the MAIN. This is the expected structure:
The get_the_block_template_html() function adds DIV.wp-site-blocks as a wrapper around everything:
wordpress-develop/src/wp-includes/block-template.php
Lines 302 to 304 in 5a53b94
| // Wrap block template in .wp-site-blocks to allow for specific descendant styles | |
| // (e.g. `.wp-site-blocks > *`). | |
| return '<div class="wp-site-blocks">' . $content . '</div>'; |
wordpress-develop/src/wp-includes/template-canvas.php
Lines 12 to 23 in 5a53b94
| $template_html = get_the_block_template_html(); | |
| ?><!DOCTYPE html> | |
| <html <?php language_attributes(); ?>> | |
| <head> | |
| <meta charset="<?php bloginfo( 'charset' ); ?>" /> | |
| <?php wp_head(); ?> | |
| </head> | |
| <body <?php body_class(); ?>> | |
| <?php wp_body_open(); ?> | |
| <?php echo $template_html; ?> |
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, thanks for clarifying. I did not know.
in that case, we can still reap the same benefit just by setting a bookmark on the DIV, then seeking back to it after finding the MAIN
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.
yes, this makes sense, implementing it in the next commit.
|
@dmsnell I have added the changes and looks good to me with what we wanted to achieve. Let me know if it looks good to you. |
| // Ensure the MAIN element has an ID. | ||
| if ( ! $processor->next_tag( 'MAIN' ) ) { | ||
| return $template_html; | ||
| } |
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 think this is covered by what @westonruter wrote, but one final check on this is whether we certain of the ordering of the DIV and MAIN elements. If we are, then no work is required. If, on the other hand, there could be cases where they are backwards, we would rather want to combine these two scan operations into a single loop.
Given the amount of feedback on this already, I hope this sounds more helpful than tedious. I’m seeing these changes in new ways each time you update them; thank you for this valuable contribution and your perseverance in iterating on it.
$found_div = false;
$target_id = null;
while ( ! $found_div && ! isset( $target_id ) && $processor->next_tag() ) {
switch ( $processor->get_tag() ) {
case 'DIV':
if ( ! $found_div && $processor->has_class( 'wp-site-blocks' ) ) {
$found_div = $processor->set_bookmark( '...' );
}
break;
case 'MAIN':
if ( ! isset( $target_id ) ) {
$target_id = $processor->get_attribute( 'id' );
if ( ! is_string( ... ) || '' === $target_id ) {
$target_id = '...';
$processor->set_attribute( 'id', $target_id );
}
}
break;
}
}
if ( ! ( $found_div && isset( $target_id ) ) ) {
return $template_html;
}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.
Yes, I thought of that initially but with my experience of sites I have looked into and worked on the block template, it seems to be always consistent. but will differ to opinion from you both as you would have more experience into different kind of site variations we may have as WordPress is not limited to a few.
If we feel we may need to check both ways, I can implement your suggestion as well. But will wait on @westonruter opinion as well if that is required.
Really grateful for the thorough review as well as it has been long time, I worked on Core issues 🙌
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.
The DIV.site-blocks should always be the wrapper of the MAIN given what I shared in #10676 (comment).
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.
Perfect, Thanks.
Then I assume we are good to go ahead with this PR right, with what we have.
|
I'm confused why the I'm also seeing a problem on the frontend with how the /*# sourceURL=http://wp-includes/css/wp-block-template-skip-link.css */ |
I'm not seeing test failures for the latest commit in No test failures in this PR either: #10694 Something is wrong with that |
I fixed this at least in 350b57c. This now matches the existing code better: wordpress-develop/src/wp-includes/script-loader.php Lines 1639 to 1645 in 76ca772
I now see: /*# sourceURL=/wp-includes/css/wp-block-template-skip-link.css */ |
| * <nav>...</nav> | ||
| * <div class="wp-site-blocks"> | ||
| * <a href="#wp--skip-link--target" id="wp-skip-link" class="..."> | ||
| * <h2>... |
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.
this example should be updated to reflect the proper HTML markup we expect, including but limited to showing that the DIV comes first

Move the block template skip link from client-side injection to server-side HTML processing using the HTML API, while keeping the existing accessibility behaviour and minifying the CSS.
What this changes
Adds
_block_template_skip_link_markup()to process the block template HTML:<main>has an id (addswp--skip-link--targetif missing).<a id="wp-skip-link" class="skip-link screen-reader-text">before.wp-site-blocks.<main>or when a skip link already exists.Updates
get_the_block_template_html()to run the rendered template through the new helper.Refactors
wp_enqueue_block_template_skip_link()to:Preserves backward compatibility gating via
the_block_template_skip_linkand block-templates theme support.Add tests for the new function as well.
Ticket
Trac ticket: https://core.trac.wordpress.org/ticket/64361
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.