Improve theme support for navigation menu toggle

[milindmore22]

Bug Description

We are adding theme support for the navigation menu, but there is a wide range of elements are being used for toggle button elements eg: div, a, span

After adding theme support to them creates an AMP validation error of missing tabindex and role

which can be avoided by adding those attributes in case the element is not a button in AMP_Nav_Menu_Toggle_Sanitizer

if ( 'button' !== $button_el->tagName ) {
	$button_el->setAttribute( 'role', 'button' );
	$button_el->setAttribute( 'tabindex', '0' ); 
}

The theme I was trying to add support: Lightning

add_theme_support(
	'amp',
	array(
		'nav_menu_toggle' => array(
			'nav_container_id'           => 'vk-mobile-nav',
			'nav_container_toggle_class' => 'vk-mobile-nav-open',
			'menu_button_id'             => 'vk-mobile-nav-menu-btn',
			'menu_button_toggle_class'   => 'menu-open',
		),
	)
);

Expected Behaviour

Should not report AMP validation error

Screenshots

PHP Version

7.4

Plugin Version

2.1.4

AMP plugin template mode

Transitional

WordPress Version

5.8

Site Health

No response

Gutenberg Version

No response

OS(s) Affected

No response

Browser(s) Affected

No response

Device(s) Affected

No response

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

[westonruter]

Humm. This should have been handled automatically by the AMP_Accessibility_Sanitizer.

[milindmore22]

Yes, that should have been handled by AMP_Accessibility_Sanitizer
in that case, the AMP_Accessibility_Sanitizer must be added at the very end of the sanitizers ie. post theme support sanitizers

	if ( ! empty( $theme_support_args['nav_menu_toggle'] ) ) {
		$sanitizers[ AMP_Nav_Menu_Toggle_Sanitizer::class ] = $theme_support_args['nav_menu_toggle'];
	}

	if ( ! empty( $theme_support_args['nav_menu_dropdown'] ) ) {
		$sanitizers[ AMP_Nav_Menu_Dropdown_Sanitizer::class ] = $theme_support_args['nav_menu_dropdown'];
	}

	// Note: This validating sanitizer must come at the end to clean up any remaining issues the other sanitizers didn't catch.
	$sanitizers['AMP_Accessibility_Sanitizer'] = [];

[westonruter]

Ah yes. This will actually be fixed by #6546 because now the key sanitizers get a fixed order at the end.

[maitreyie-chavan]

@milindmore22 with #6546 being released, please could you verify if this issue is still valid? Will accordingly have it either closed or added to the next release milestone.

[milindmore22]

Verified Fixed, Closing this as fixed in #6546