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

[afercia]

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.

[chetan200891]

Created initial patch.

[chetan200891]

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

[ianbelanger]

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

[ianbelanger]

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]

Updated patch to remove unnecessary id and class

[SergeyBiryukov]

In 45595:

Twenty Nineteen: Adjust markup in twentynineteen_add_ellipses_to_nav() for better readability.

See #47067.

[SergeyBiryukov]

In 45596:

Twenty Nineteen: Fix markup errors in twentynineteen_add_ellipses_to_nav():

• Add missing i18n for aria-label attribute.
• Remove redundant screen reader text superseded by aria-label.
• Remove unnecessary id and class attributes.

Props afercia, chetan200891, ianbelanger.
Fixes #47067.