WordPress.org

Make WordPress Core

Opened 7 weeks ago

Last modified 4 days ago

#47067 new defect (bug)

Twenty Nineteen: fix markup errors in `twentynineteen_add_ellipses_to_nav()`

Reported by: afercia Owned by:
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch commit
Focuses: Cc:

Description

See https://core.trac.wordpress.org/browser/trunk/src/wp-content/themes/twentynineteen/inc/template-functions.php?rev=44376&marks=201-228#L194

function twentynineteen_add_ellipses_to_nav( $nav_menu, $args ) {

	if ( 'menu-1' === $args->theme_location ) :

		$nav_menu .= '<div class="main-menu-more">';
		$nav_menu .= '<ul class="main-menu">';
		$nav_menu .= '<li class="menu-item menu-item-has-children">';
		$nav_menu .= '<button class="submenu-expand main-menu-more-toggle is-empty" tabindex="-1" aria-label="More" aria-haspopup="true" aria-expanded="false">';
		$nav_menu .= '<span class="screen-reader-text">' . esc_html__( 'More', 'twentynineteen' ) . '</span>';
		$nav_menu .= twentynineteen_get_icon_svg( 'arrow_drop_down_ellipsis' );
		$nav_menu .= '</button>';
		$nav_menu .= '<ul class="sub-menu hidden-links">';
		$nav_menu .= '<li id="menu-item--1" class="mobile-parent-nav-menu-item menu-item--1">';
		$nav_menu .= '<button class="menu-item-link-return">';
		$nav_menu .= twentynineteen_get_icon_svg( 'chevron_left' );
		$nav_menu .= esc_html__( 'Back', 'twentynineteen' );
		$nav_menu .= '</button>';
		$nav_menu .= '</li>';
		$nav_menu .= '</ul>';
		$nav_menu .= '</li>';
		$nav_menu .= '</ul>';
		$nav_menu .= '</div>';

	endif;

	return $nav_menu;
}
  • aria-label="More" is not translatable
  • regardless, the button has also a visually hidden text <span class="screen-reader-text">' . esc_html__( 'More', 'twentynineteen' ) . '</span>: worth noting the non-translatable aria-label overrides any content of the button so: either use the hidden text or the aria-label (after it's made translatable), not both
  • <li id="menu-item--1": not sure why there's this hardcoded ID: it produces duplicate IDs in a page, which is invalid HTML and I don't see a good reason to use it in the first place, unless I'm missing something

As these are formal errors related to valid HTML / coding standards, I'd like to propose to fix them as soon as possible in the next minor release.

Attachments (2)

47067.diff (1.2 KB) - added by chetan200891 7 weeks ago.
Created initial patch.
47067.1.diff (1.4 KB) - added by ianbelanger 4 days ago.
Updated patch to remove unnecessary id and class

Download all attachments as: .zip

Change History (6)

@chetan200891
7 weeks ago

Created initial patch.

#1 @chetan200891
7 weeks ago

@afercia I have created initial patch 47067.diff which fixed first two issued. Let me know your suggestions if anything need to change.

#2 @desrosj
7 weeks ago

  • Keywords has-patch needs-testing added; needs-patch removed

#3 @ianbelanger
5 weeks ago

  • Milestone changed from 5.2.1 to 5.3

Changing milestone as Bundled Themes are only updated during major releases.

#4 @ianbelanger
4 days ago

  • Keywords commit added; needs-testing removed

Just tested 47067.diff and it still applies cleanly. It does take care of issues 1 & 2. However, I am not sure that issue 3, is really an issue at all.

<li id="menu-item--1": not sure why there's this hardcoded ID: it produces duplicate IDs in a page, which is invalid HTML and I don't see a good reason to use it in the first place, unless I'm missing something

I would agree that hardcoding an id into a menu-item is the wrong approach. However, this menu item is for the "Back" button, when the ellipsis has been clicked and the sub-menu is showing. As far as I can tell, it is not used anywhere else on the page, even when all menu locations are used. Also it uses 2 hyphens, where other menu items use just 1. menu-item--1 vs menu-item-1 Therefor, there shouldn't be any duplicate IDs on the page.

All that being said, the id for this menu-item is unnecessary. I could not find any js or css in the theme that targets this particular id or the corresponding class, so I will be uploading a patch that removes both of them. I will leave it up to the committer to choose which approach to use.

@ianbelanger
4 days ago

Updated patch to remove unnecessary id and class

Note: See TracTickets for help on using tickets.