Fix broken list markup in navigation block when 3rd party blocks are used as decendants of navigation block

[fabiankaegy]

What?

Use the HTML_Tag_Processor to check if an inner block markup is wrapped in a li element instead of using an hardcoded list of allowed inner blocks.

Why?

The hard-coded list of blocks that it should be applied to prevented custom blocks from getting the handling which led to inaccessible markup.

How?

Using the HTML_Tag_Processor to check if the wrapper element is an li element

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

[github-actions]

Flaky tests detected in 2ceb918.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7817679815
📝 Reported issues:

• [Flaky Test] aria-disabled is correctly hydrated for different values #54370 in /test/e2e/specs/interactivity/directive-bind.spec.ts

[getdave]

Not had time to look into this in detail yet but if htis relates to handling li tags then do we need to amend $needs_list_item_wrapper as well?

Also does this open the block up to theoretically containing all types of blocks?

[fabiankaegy]

Not had time to look into this in detail yet but if htis relates to handling li tags then do we need to amend $needs_list_item_wrapper as well?

That is a very good question... I'm not sure how we can make that distinction. Regardless currently the logic is implemented in the incorrect order. We should first wrap the blocks in an LI and then check whether they have an LI.

Currently the code I have here produces the markup where the site logo is wrapped in an LI but outside of the UL

So that will need to be fixed before this can be merged...

Looking at the current setup the Search for example doesn't get wrapped but the Site Logo does. Which I can understand but I'm having difficulty generalizing it. Because of that I would generally say we shouldn't automatically wrap any external custom blocks in LI elements because those blocks can always take care of that themselves. Its much harder for a custom block to remove the wrapper if they don't want it.

because of that I would for now leave that manual hardcoded list in place

[fabiankaegy]

Just added a new commit that takes care of excluding those blocks that manually get wrapped in the LI from the LI check so that the behavior for those doesn't change.

[getdave]

@scruffian In light of your refactor in #55605 this is relevant.

@fabiankaegy you might like to also take a look at this.

[draganescu]

Hi Fabian, thanks for trying to improve this block 🙏 and it's giant pile of past decisions 😀

Although the idea of the PR is interesting (it's nice to rely on the power of the tag processor) - I have doubts about the "why" of the PR: it seems unlikely the navigation block will become "open" to other blocks at this stage.

Random blocks would become able to join a navigation either via the new block hooks API or when the block will offer "mega menu" creation abilities. For now that markup is too easy to break IMO if any block can be inserted.

What do you think?

[fabiankaegy]

@draganescu I do share your concerns about the complexity here and how fragile the navigation block seems. But I have to disagree that we should not open it up for extenders to insert other type of elements within.

Is the solution really that extenders should instead build an entire custom navigation block from scratch when they have different more complex requirements. Core will never and should never cover all edgecases for how menus work. For example looking at techctunch.com the flyout type navigation is not something I would ever say core should handle on top of megamenus and regular submenus. But that is where extending the navigation block with other child blocks becomes really really useful and powerful.

And it is already possible for anyone to add additional blocks to be used within the navigation block. The issue is just that the markup created by it becomes inaccessible because of the different lists that it creates. This would be a non-issue if the actual markup of the child blocks were checked instead of a hardcoded list of blocks that should get special treatment.

I get the sentiment that opening something up for extenders means we now have a greater responsibility for backward compatibility and therefore cannot iterate easily anymore.

But especially on navigations which are the primary way of navigating through a site that every user will have to interact with, the ability to customize the experience more by allowing other blocks to be used within (which already is possible) is really important from my point of view.

[draganescu]

I think we're both on the same page, I just don't understand what doesn't for instance the new block hooks option offer? Also what other blocks have extendable allowed innerBlocks?

The current block API is so that when a block has innerblocks and it defines what kinds of blocks are allowed it is not something that can be changed. Blocks have all kinds of limitations in terms of what they allow and what they disallow and currently the way to extend this is via what may be interpreted as a leaky API via filtering the render.

I do not disagree with you at all that the navigation block is important and that it will never cover on its own all the cases and that it should be extendable. I only want to make sure that the way in which this extendability works is not something that limits its future development.

[github-actions]

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ phpunit/blocks/class-wp-navigation-block-renderer-test.php

[getdave]

@fabiankaegy If we progress this we should probably add additional test cases to https://github.com/WordPress/gutenberg/blob/trunk/phpunit/class-wp-navigation-block-renderer-test.php to check that blocks that are not in the original constant (the one this PR no longer relies upon) are "wrapped" correctly.

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: fabiankaegy <[email protected]>
Co-authored-by: getdave <[email protected]>
Co-authored-by: scruffian <[email protected]>
Co-authored-by: draganescu <[email protected]>
Co-authored-by: ndiego <[email protected]>
Co-authored-by: justintadlock <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[fabiankaegy]

I just rebased this PR and resolved any conflicts 👍

[fabiankaegy]

Hey all 👋

I wanted to re surface this discussion here in light of the recent merge of #58262 which makes it much easier to manipulate the list of inner blocks used within the navigation. But as it stands today this will create a large accessibility issue because of the hardcoded list for which blocks get the special treatment.

I'd love to get some additional input on the approach here and see if we could land this alongside #58262 in WordPress 6.5

CC: @getdave @youknowriad

[ndiego]

Using the new method of filtering allowedBlocks, I can add icons to Navigation, which is amazing!!! 🎉

However, the front-end markup looks like this, which is not that great. Placing the icon in the middle of the navigation breaks the links into two separate lists. Would this PR fix that @fabiankaegy?

Note that I am getting a static error when testing this PR:

Fatal error: Uncaught Error: Access to undeclared static property WP_Navigation_Block_Renderer_Gutenberg::$has_submenus

[fabiankaegy]

@ndiego yes exactly that is the aim of this PR. That is the reason it exists. Tough it looks like my lates rebase created an issue. So I will take a look at that.

Thanks for testing!

[getdave]

Just want to check the JavaScript Unit test failures here.

[fabiankaegy]

Thank you all for your help landing this ❤️

[getdave]

Dev note - Introducing the block_core_navigation_listable_blocks Filter

WordPress 6.5 introduces a new filter, block_core_navigation_listable_blocks which is designed to provide control over the accessible rendering of child blocks used within the Navigation block.

Historically the Navigation block has conditionally wrapped particular blocks in <li> tags to ensure accessible markup on the front of the site.

With the introduction of the new allowedBlocks API, which technically allows any block to be added as valid children of the Navigation block, developers require a means to indicate which blocks need to be wrapped.

The block_core_navigation_listable_blocks filter allows developers to determine whether a specific block should be wrapped in an <li> tag thereby aiding in adherence to accessibility standards and the creation of valid markup.

The example below shows the mycustomblock/icon block opting into being wrapped in an <li> tag:

function add_icon_block_to_needs_list_item_wrapper( $blocks ) {
    $blocks[] = 'mycustomblock/icon';
    return $blocks;
}
add_filter( 'block_core_navigation_listable_blocks', 'add_icon_block_to_needs_list_item_wrapper' );

See also the Dev note on the allowedBlocks API.

[fabiankaegy]

@getdave Thank you for adding this ❤️. It was on my todo list for today :)

What you have writen is clear and correct 👍 I am happy with this being the dev note for the feature

[getdave]

@getdave Thank you for adding this ❤️. It was on my todo list for today :)

Oh sorry I hope I didn't step on your toes! 🤦

I had some notes written up from when I investigated the issue and felt it should be added as the dev note.

Glad it works for you.

[fabiankaegy]

Ohh no, no worries :) I'm glad you added this :)

[ndiego]

Was this included in 6.5? I am testing the code snippet above and I am struggling to get it to work.

[getdave]

Ok panic over - it was included in the final Gutenberg release 17.7.0RC.

So that means it should be in Core.

[getdave]

@ndiego I think I made a typo in the docs. The filter is:

block_core_navigation_listable_blocks

As per this line.

I've updated the docs and will ping the relevant release leads to get the docs updated.

cc @fabiankaegy for confidence check.

[ndiego]

@ndiego I think I made a typo in the docs. The filter is:

block_core_navigation_listable_blocks

Ah, that's it! I just copied the code snippet. All is working. Thanks!