Make WordPress Core

Opened 16 months ago

Closed 8 months ago

Last modified 8 months ago

#57140 closed enhancement (fixed)

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

Reported by: davidwebca's profile davidwebca Owned by: audrasjb's profile audrasjb
Milestone: 6.3 Priority: normal
Severity: minor Version: 3.0
Component: Menus Keywords: has-patch needs-testing has-unit-tests add-to-field-guide
Focuses: javascript, template Cc:

Description

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

Change History (30)

This ticket was mentioned in PR #3645 on WordPress/wordpress-develop by davidwebca.


16 months ago
#1

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

#2 follow-up: @danyk4
16 months ago

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

davidwebca commented on PR #3645:


16 months ago
#3

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.

@costdev commented on PR #3645:


16 months ago
#4

@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/'
}}}

@peterwilsoncc commented on PR #3645:


16 months ago
#5

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 &amp; Pencils"
wp> $attr = esc_attr( $attr );
=> string(18) "Pens &amp; Pencils"

@costdev commented on PR #3645:


16 months ago
#6

@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?

@peterwilsoncc commented on PR #3645:


16 months ago
#7

@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.

@costdev commented on PR #3645:


16 months ago
#8

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.

davidwebca commented on PR #3645:


16 months ago
#9

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

davidwebca commented on PR #3645:


12 months ago
#10

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?

#11 @costdev
12 months ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 6.3
  • Version changed from trunk to 3.0

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.

#12 @davidwebca
12 months ago

Amazing thanks!

#13 @oglekler
9 months ago

@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.

#14 @costdev
9 months ago

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

davidwebca commented on PR #3645:


8 months ago
#15

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.

#16 @costdev
8 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

@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?

#17 @oglekler
8 months ago

@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.

#18 @costdev
8 months ago

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

#19 @oglekler
8 months ago

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

#20 @oglekler
8 months ago

  • Keywords commit added

#21 @audrasjb
8 months ago

  • Keywords commit removed

Removing commit as @peterwilsoncc requested some changes in the PR

#22 @azaozz
8 months ago

Also E2E Tests failing.

davidwebca commented on PR #3645:


8 months ago
#23

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

#24 @costdev
8 months ago

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.

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


8 months ago

#26 in reply to: ↑ 2 @hasanazizul
8 months ago

it is a good enhancement

#27 @audrasjb
8 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

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

#28 @audrasjb
8 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

@peterwilsoncc commented on PR #3645:


8 months ago
#29

Committed in r56067

#30 @stevenlinx
8 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.