Skip to content

Commit

Permalink
Fix broken list markup in navigation block when 3rd party blocks are …
Browse files Browse the repository at this point in the history 
…used as decendants of navigation block (#55551)

This introduces a new filter called `block_core_navigation_listable_blocks` that allows extenders to modify the list of blocks that should get wrapped within a `li` element when nested inside the Navigation block. 

On top of that it also modifies the mechanism that controls the `ul` markup to no longer check for a hardcoded list of blocks that are list items. But instead uses the `WP_HTML_Tag_Processor` to check the actual markup to see whether something is a list item.

Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: scruffian <scruffian@git.wordpress.org>
Co-authored-by: draganescu <andraganescu@git.wordpress.org>
Co-authored-by: ndiego <ndiego@git.wordpress.org>
Co-authored-by: justintadlock <greenshady@git.wordpress.org>
  • Loading branch information
@fabiankaegy @getdave
@scruffian @draganescu @ndiego @justintadlock
7 people authored Feb 8, 2024
1 parent bee57c4 commit 0a828cf
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 16 deletions.
37 changes: 21 additions & 16 deletions packages/block-library/src/navigation/index.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,6 @@ class WP_Navigation_Block_Renderer {
*/
private static $has_submenus = false;

/**
* Used to determine which blocks are wrapped in an <li>.
*
* @var array
*/
private static $nav_blocks_wrapped_in_list_item = array(
'core/navigation-link',
'core/home-link',
'core/site-title',
'core/site-logo',
'core/navigation-submenu',
);

/**
* Used to determine which blocks need an <li> wrapper.
*
Expand Down Expand Up @@ -117,7 +104,23 @@ private static function is_interactive( $attributes, $inner_blocks ) {
* @return bool Returns whether or not a block needs a list item wrapper.
*/
private static function does_block_need_a_list_item_wrapper( $block ) {
return in_array( $block->name, static::$needs_list_item_wrapper, true );

/**
* Filter the list of blocks that need a list item wrapper.
*
* Affords the ability to customize which blocks need a list item wrapper when rendered
* within a core/navigation block.
* This is useful for blocks that are not list items but should be wrapped in a list
* item when used as a child of a navigation block.
*
* @since 6.5.0
*
* @param array $needs_list_item_wrapper The list of blocks that need a list item wrapper.
* @return array The list of blocks that need a list item wrapper.
*/
$needs_list_item_wrapper = apply_filters( 'block_core_navigation_listable_blocks', static::$needs_list_item_wrapper );

return in_array( $block->name, $needs_list_item_wrapper, true );
}

/**
Expand Down Expand Up @@ -161,7 +164,9 @@ private static function get_inner_blocks_html( $attributes, $inner_blocks ) {
$is_list_open = false;

foreach ( $inner_blocks as $inner_block ) {
$is_list_item = in_array( $inner_block->name, static::$nav_blocks_wrapped_in_list_item, true );
$inner_block_markup = static::get_markup_for_inner_block( $inner_block );
$p = new WP_HTML_Tag_Processor( $inner_block_markup );
$is_list_item = $p->next_tag( 'LI' );

if ( $is_list_item && ! $is_list_open ) {
$is_list_open = true;
Expand All @@ -176,7 +181,7 @@ private static function get_inner_blocks_html( $attributes, $inner_blocks ) {
$inner_blocks_html .= '</ul>';
}

$inner_blocks_html .= static::get_markup_for_inner_block( $inner_block );
$inner_blocks_html .= $inner_block_markup;
}

if ( $is_list_open ) {
Expand Down
96 changes: 96 additions & 0 deletions phpunit/blocks/class-wp-navigation-block-renderer-test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,102 @@ public function test_gutenberg_get_markup_for_inner_block_site_title() {
$this->assertEquals( $expected, $result );
}

/**
* Test that a given block will not be automatically wrapped in a list item by default.
*
* @group navigation-renderer
*
* @covers WP_Navigation_Block_Renderer::get_markup_for_inner_block
*/
public function test_gutenberg_block_not_automatically_wrapped_with_li_tag() {

register_block_type(
'testsuite/sample-block',
array(
'api_version' => 2,
'render_callback' => function ( $attributes ) {
return '<div class="wp-block-testsuite-sample-block">' . $attributes['content'] . '</div>';
},
)
);

// We are testing the site title block because we manually add list items around it.
$parsed_blocks = parse_blocks(
'<!-- wp:testsuite/sample-block {"content":"Hello World"} /-->'
);
$parsed_block = $parsed_blocks[0];
$context = array();
$heading_block = new WP_Block( $parsed_block, $context );

// Setup an empty testing instance of `WP_Navigation_Block_Renderer` and save the original.
$reflection = new ReflectionClass( 'WP_Navigation_Block_Renderer_Gutenberg' );
$method = $reflection->getMethod( 'get_markup_for_inner_block' );
$method->setAccessible( true );
// Invoke the private method.
$result = $method->invoke( $reflection, $heading_block );

$expected = '<div class="wp-block-testsuite-sample-block">Hello World</div>';
$this->assertEquals( $expected, $result );

unregister_block_type( 'testsuite/sample-block' );
}

/**
* Test that a block can be added to the list of blocks which require a wrapping list item.
* This allows extenders to opt in to the rendering behavior of the Navigation block
* which helps to preserve accessible markup.
*
* @group navigation-renderer
*
* @covers WP_Navigation_Block_Renderer::get_markup_for_inner_block
*/
public function test_gutenberg_block_is_automatically_wrapped_with_li_tag_when_filtered() {

register_block_type(
'testsuite/sample-block',
array(
'api_version' => 2,
'render_callback' => function ( $attributes ) {
return '<div class="wp-block-testsuite-sample-block">' . $attributes['content'] . '</div>';
},
)
);

$filter_needs_list_item_wrapper_function = static function ( $needs_list_item_wrapper ) {
$needs_list_item_wrapper[] = 'testsuite/sample-block';
return $needs_list_item_wrapper;
};

add_filter(
'block_core_navigation_listable_blocks',
$filter_needs_list_item_wrapper_function,
10,
1
);

// We are testing the site title block because we manually add list items around it.
$parsed_blocks = parse_blocks(
'<!-- wp:testsuite/sample-block {"content":"Hello World"} /-->'
);
$parsed_block = $parsed_blocks[0];
$context = array();
$heading_block = new WP_Block( $parsed_block, $context );

// Setup an empty testing instance of `WP_Navigation_Block_Renderer` and save the original.
$reflection = new ReflectionClass( 'WP_Navigation_Block_Renderer_Gutenberg' );
$method = $reflection->getMethod( 'get_markup_for_inner_block' );
$method->setAccessible( true );
// Invoke the private method.
$result = $method->invoke( $reflection, $heading_block );

$expected = '<li class="wp-block-navigation-item"><div class="wp-block-testsuite-sample-block">Hello World</div></li>';
$this->assertEquals( $expected, $result );

remove_filter( 'block_core_navigation_listable_blocks', $filter_needs_list_item_wrapper_function, 10, 1 );

unregister_block_type( 'testsuite/sample-block' );
}

/**
* Test that the `get_inner_blocks_from_navigation_post` method returns an empty block list for a non-existent post.
*
Expand Down

0 comments on commit 0a828cf

Please sign in to comment.