Add filters to allow themes and plugins to pass HTML attributes to different Nav Walker output

[davidwebca]

Currently, Nav Walkers allow custom HTML attributes to be passed to the link (<a>) element only. There are many instances, like when creating a menu with a declarative JavaScript library, where it would be ideal to be able to pass down HTML attributes to any of the built element. UL and LI are currently excluded of this.

Here's a link to the pull request: ​https://github.com/WordPress/wordpress-develop/pull/3645

Currently, Nav Walkers allow custom HTML attributes to be passed to the link (<a>) element only. There are many instances, like when creating a menu with a declarative JavaScript library, where it would be ideal to be able to pass down HTML attributes to any of the built element. UL and LI are currently excluded of this.

Trac ticket: https://core.trac.wordpress.org/ticket/57140

[danyk4]

this is very useful ticket
in modern web we need to add custom attrs to the wp menu

Thanks @costdev for your pointers, very helpful.

As for the HTML attributes building function, here's my train of thought:

At first, I started using the ​code that was already there with the assumptions that it had already been approved by core and had the least risk of causing breaking changes for end users. I had replicated the code multiple times and it started feeling silly so I extracted it into its separated protected function in the Nav Walker's class. But, then, my research lead me to previous efforts regarding this:

• A general HTML attribute building function just like the one I extracted that could be added to formatting.php - https://core.trac.wordpress.org/ticket/23236. Opened 10 years ago and closed 9 years ago for lack of interest.
• A general HTML attribute name escape function with an interesting conversation about the differences in allowed characters to escape or not in attribute names versus values. https://core.trac.wordpress.org/ticket/43010

I think it's safe to assume that the path to have this included as fast as possible without introducing breaking changes in Nav Walkers would be to continue with this patch without involving too many different conversations at once, but once that's done, we could start looking at reviving those two conversations, especially since the second one already has a patch and is just sitting there without activity. It would be amazing to have a general HTML attributes builder functions that every classes could refactor to use and eventually debloat + secure a lot of code.

@davidwebca Thanks for the detailed response!

I agree with you that given the following:

1. The existing code doesn't escape attribute names.
2. Extenders may be using spec-valid attribute names that esc_attr() would break.
3. Core doesn't have a spec-valid escape for attribute names.

This PR shouldn't escape the attribute names, doesn't introduce an additional security issue here, and escaping these attribute names should be part of wider discussion about the introduction of a spec-valid escape for attribute names.

Had this been newly introduced code, the introduction of a spec-valid escape in future would be an _enhancement_, not a regression. However, using esc_attr() in this PR would be a regression.

---

Regarding test failures, note the previous output:
{{{php
$output .= $indent . '<li' . $id . $class_names . '>';
}}}

Reversing lines 192 and 193 in the PR shows all tests passing with:
{{{sh
./vendor/bin/phpunit --filter '/Tests_Menu_Walker_Nav_Menu|Tests_Menu_wpNavMenu/'
}}}

This PR shouldn't escape the attribute names, doesn't introduce an additional security issue here, and escaping these attribute names should be part of wider discussion about the introduction of a spec-valid escape for attribute names.

Just a quick note that WordPress prevents double escaping, so esc_attr() should be fine in this context. From wp shell to demonstrate:

wp> $attr = 'Pens & Pencils';
=> string(14) "Pens & Pencils"
wp> $attr = esc_attr( $attr );
=> string(18) "Pens & Pencils"
wp> $attr = esc_attr( $attr );
=> string(18) "Pens & Pencils"

@peterwilsoncc So you don't anticipate any issues causes by introducing esc_attr() for attribute names in this PR?

If so, should this change be made in this PR, or a follow-up ticket to note the change?

@costdev No, I don't. I'm happy for the change to remain in this PR, the data is currently escaped and this PR is simply moving around where that takes place. Feel free to correct me if I am misunderstanding, I've only done a quick scan of the PR.

Summary of discussion with ​@peterwilsoncc in Slack:

• ​The existing code doesn't escape attribute names, only values.
• Introducing esc_attr() to attribute names here could break CSS styling.
• Further consideration is needed before escaping attribute names in already-existing code.

In short: The PR can continue without introducing esc_attr() for attribute names.

I've made the swap suggested by @costdev and tests seem to be okay.

Hi @costdev! Just wanted to check on this. Is there anything else I can do to help get the ball rolling or is this just waiting to be pulled in?

[costdev]

Thanks for the ping @davidwebca!

Aside from some unit tests to ensure the new filters are run, and only when they should run, this looks close to being ready for commit consideration.

As this is an enhancement, and we're in Beta for 6.2, I'll milestone this ticket for 6.3. We should be able to land this next cycle.

[davidwebca]

Amazing thanks!

[oglekler]

@davidwebca can you please add Unit-tests for this enhancement?
We have 9 days before Beta 1 and if it will not be in trunk, unluckily, it should be rescheduled to 6.4.

[costdev]

@oglekler I've sent some tests to @davidwebca so the PR should have these added soon

Tests are added. I didn't read them through but since the patch was given by @costdev I have full confidence it's ok. Sorry for the multiple commits, there were some conflicts when applying the patch.

[costdev]

@oglekler Tests have now been added to the PR and there's just a minor bit of feedback to address to remove some extra () in the @covers annotations.

With the unit tests included here, do you think this still needs manual testing, or is it ready for commit consideration?

[oglekler]

@costdev,

I did manual testing with Twenty Twenty theme and WP 6.3-alpha-55505-src with the following filters:

add_filter( 'nav_menu_submenu_attributes', function( $atts, $args, $depth ) {
    
    $atts['class']     = ! empty( $atts['class'] ) ? $atts['class'] . ' 57140-submenu_attributes-0' : ' 57140-submenu_attributes-0';
    $atts['data-test'] = '57140-submenu_attributes-1';
    
    return $atts;
}, 10, 3);

add_filter( 'nav_menu_item_attributes', function( $atts, $menu_item, $args, $depth ) {
    
    $atts['class']     = ! empty( $atts['class'] ) ? $atts['class'] . ' 57140-item_attributes-0' :  '57140-item_attributes-0';
    $atts['data-test'] = '57140-item_attributes-1';
    
    return $atts;
}, 10, 4);

add_filter( 'nav_menu_link_attributes', function( $atts, $menu_item, $args, $depth ) {
    
    $atts['class']     = ! empty( $atts['class'] ) ? $atts['class'] . ' 57140-link_attributes-0' : '57140-link_attributes-0';
    $atts['data-test'] = '57140-link_attributes-1';
    
    return $atts;
}, 10, 4);

All attributes went into the correct places.

[costdev]

Awesome! Thanks for testing and confirming! This seems ready for commit consideration now then, what do you think?

[oglekler]

Yes, I think so, @costdev, just @since 6.2.0 needs to be changed.

[audrasjb]

Removing commit as @peterwilsoncc requested some changes in the PR

[azaozz]

Also E2E Tests failing.

Thanks for all the suggestions. I've added them now.

[costdev]

The feedback appears to have been addressed. @peterwilsoncc can you take another look?

@azaozz I don't think the E2E failure is related as it refers to tinymce, and this PR touches the Nav Walker - we've had almost constant E2E failures in the past few days and I think investigation is ongoing.

[hasanazizul]

it is a good enhancement

[audrasjb]

Thanks for updating the PR!
Self assigning for final review/commit.

[audrasjb]

In 56067:

Menus: Allow themes and plugins to pass HTML attributes to various Nav Walker outputs.

This introduces a new set of hooks that can be used to filter various HTML elements of the Nav Walker, in order to output the desired HTML attributes:

• List items: nav_menu_item_attributes
• Submenu <ul> element: nav_menu_submenu_attributes

Props davidwebca, danyk4, costdev, peterwilsoncc, audrasjb, oglekler.
Fixes #57140.

Committed in r56067