Make WordPress Core

Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#40666 closed enhancement (fixed)

allow arbitrary HTML attributes on <a> produced by Walker_Category

Reported by: pbiron's profile pbiron Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.2 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch commit has-dev-note
Focuses: accessibility Cc:

Description

Walker_Nav_Menu currently allows arbitrary HTML attributes to be added to the <a> tags it generates via the nav_menu_link_attributes filter.

It would be useful to do the same in Walker_Category (i.e., add a category_menu_link_attributes filter).

Related: #40359

Attachments (3)

40666.diff (2.1 KB) - added by pbiron 7 years ago.
40666.2.diff (2.1 KB) - added by audrasjb 5 years ago.
Patch refreshed against trunk and @since tag refreshed as well
40666.3.diff (2.4 KB) - added by afercia 5 years ago.

Download all attachments as: .zip

Change History (33)

@pbiron
7 years ago

#1 follow-up: @pbiron
7 years ago

  • Keywords dev-feedback has-patch added

To the best of my knowledge, in core Walker_Category is only used by wp_list_categories() and wp_list_categories() is only used by `WP_Widget_Categories'.

As a result, I'm not sure that the name of new filter introduced in my patch (category_menu_link_attributes) is appropriate. I named it that to mirror the new filter introduced in #40359...which was named after the existing filter in Walker_Nav_Menu.

I'd like some feedback on the naming (and inline docs) for this new filter.

#2 @pbiron
7 years ago

  • Focuses accessibility added

Another thing I noticed while putting together this patch is that there is a difference in the way wp_list_categories() and wp_list_pages() invoke Walker::walk() (via walk_category_tree() and walk_page_tree(), respectively).

Namely, wp_list_pages() passes the current page as a param to walk_page_tree(), whereas wp_list_categories() does not pass the current category as a param to walk_category_tree(), passing it instead $r['current_category'].

As a result, those hooking into the new filter proposed here need to write their funcs slightly different than the similar filters for nav_menu_link_attributes and page_menu_link_attributes, as in:

add_filter( 'category_menu_link_attributes', 'add_aria_current_category', 10, 5 );

function
add_aria_current_category( $atts, $category, $depth, $args, $id )
{
	if ( $category->term_id === $id ||
			( isset( $args['current_category'] ) &&
			$category->term_id === $args['current_category'] ) ) {
		$atts['aria-current'] = 'page' ;
		}

	return $atts ;
}

I think it might make sense to include a note about this different in the inline docs (resulting in the note appearing in DevHub. Alternatively, the note could be added a "comment" on the DevHub page for the new filter. I'd like feedback on this as well.

Last edited 7 years ago by pbiron (previous) (diff)

This ticket was mentioned in Slack in #accessibility by pbiron. View the logs.


7 years ago

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


7 years ago

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


7 years ago

#6 @afercia
7 years ago

@SergeyBiryukov since you already owned #40359 would you like to own this too? :)

#7 @SergeyBiryukov
7 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 years ago

This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.


6 years ago

#10 follow-up: @afercia
6 years ago

  • Milestone changed from Future Release to 4.9.9

Discussed during today's accessibility bug-scrub an agreed we'd like to propose this for 4.9.9 consideration. /Cc @SergeyBiryukov

#11 in reply to: ↑ 10 @pbiron
6 years ago

Replying to afercia:

Discussed during today's accessibility bug-scrub an agreed we'd like to propose this for 4.9.9 consideration. /Cc @SergeyBiryukov

Great, thanx!

Let me know if there are any changes necessary to the patch (such as the filter name, as suggested in comment:1).

#12 @pento
5 years ago

  • Milestone changed from 4.9.9 to 5.0.1

#13 @pento
5 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#14 @pento
5 years ago

  • Milestone changed from 5.0.2 to 5.0.3

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


5 years ago

#16 @desrosj
5 years ago

  • Milestone changed from 5.0.3 to 5.1

Going to punt this to 5.1. It is outside of the scope for 5.0.3 (block
editor bugs, regressions, and major bugs). The patch still applies cleanly for me. Whoever ends up reviewing and committing this will just need to change the @since tag.

#17 @pento
5 years ago

  • Milestone changed from 5.1 to 5.2

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


5 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


5 years ago

This ticket was mentioned in Slack in #accessibility by pbiron. View the logs.


5 years ago

#21 @audrasjb
5 years ago

  • Keywords needs-refresh added

@audrasjb
5 years ago

Patch refreshed against trunk and @since tag refreshed as well

#22 @audrasjb
5 years ago

  • Keywords needs-refresh removed

@afercia
5 years ago

#23 @afercia
5 years ago

  • Keywords commit added

40666.3.diff makes phpcs happy and boyscouts a missing translators comment.

#24 @pbiron
5 years ago

Thanx for the updates!

Before this gets committed, does anyone have a reaction to the question I asked in https://core.trac.wordpress.org/ticket/40666#comment:1 re: the name of the filter?

#25 @audrasjb
5 years ago

  • Keywords needs-dev-note added; dev-feedback removed

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


5 years ago

#27 in reply to: ↑ 1 @SergeyBiryukov
5 years ago

Replying to pbiron:

To the best of my knowledge, in core Walker_Category is only used by wp_list_categories() and wp_list_categories() is only used by WP_Widget_Categories.

As a result, I'm not sure that the name of new filter introduced in my patch (category_menu_link_attributes) is appropriate. I named it that to mirror the new filter introduced in #40359...which was named after the existing filter in Walker_Nav_Menu.

I'd like some feedback on the naming (and inline docs) for this new filter.

In hindsight, page_menu_link_attributes could probably have been called page_list_link_attributes, as Walker_Page is only used directly in wp_list_pages(), via walk_page_tree().

However, wp_list_pages() is also used in wp_page_menu(), so page_menu_link_attributes seemed to make sense in that regard.

For categories, I think category_list_link_attributes would make sense without deviating too much from the existing naming of page_menu_link_attributes and nav_menu_link_attributes.

#28 @SergeyBiryukov
5 years ago

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

In 44957:

Accessibility: Introduce category_list_link_attributes filter in Walker_Category::start_el() for the HTML attributes applied to a category list item's anchor element.

This complements the page_menu_link_attributes filter in Walker_Page::start_el() and the nav_menu_link_attributes filter in Walker_Nav_Menu::start_el().

Props pbiron, audrasjb, afercia.
Fixes #40666. See #40359.

#29 @SergeyBiryukov
5 years ago

In 44958:

Menus: Use esc_url() for the href value of page link attributes in Walker_Page::start_el(), for consistency with Walker_Nav_Menu and Walker_Category.

See #40666, #40359.

#30 @desrosj
5 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.