Twenty Seventeen: consider to remove the onResizeARIA() function in navigation.js

[afercia]

I know some of the functionalities in Twenty Seventeen are ported and adjusted from Twenty Sixteen, but that doesn't mean they shouldn't be improved or completely reconsidered :)

The onResizeARIA() function is meant to update some aria attributes on the navigation menu and the menu toggle. By the way, there are significant differences in the way it works on Twenty Sixteen and on Twenty Seventeen and probably it should be completely removed. You can see its current revision here: https://core.trac.wordpress.org/browser/trunk/src/wp-content/themes/twentyseventeen/assets/js/navigation.js?rev=39419#L107

As far as I see, it also doesn't work correctly under some circumstances, for example when expanding the menu and then resizing a few times, the aria-expanded attributes may be not synced and have a wrong value, see screenshot below:

Starting from the aria attributes set on the navigation menu: on Twenty Sixteen they were set on the <nav> element while on Twenty Seventeen they're set on the unordered list <ul>. On Twenty Sixteen this makes a bit more sense, since the <nav> element has an aria role so it is supposed to be available to assistive technologies:

<nav id="site-navigation" class="main-navigation" role="navigation" aria-label="Primary Menu" aria-expanded="true">

Anyway, this aria-expanded attribute is not announced (see screenshots below, using VoiceOver and NVDA) so it has no practical effect.

On Twenty Seventeen, it makes even less sense because the <ul> element is not available to assistive technologies in any way: it's not focusable and doesn't have any aria role so any aria attribute set on it doesn't have any effect.

To start with, I'd propose to remove anything that refers to siteNavigation from this function. After doing that, here's what's left:

function onResizeARIA() {
	if ( 'block' === $( '.menu-toggle' ).css( 'display' ) ) {

		if ( menuToggle.hasClass( 'toggled-on' ) ) {
			menuToggle.attr( 'aria-expanded', 'true' );
		} else {
			menuToggle.attr( 'aria-expanded', 'false' );
		}
	} else {
		menuToggle.removeAttr( 'aria-expanded' );
		menuToggle.removeAttr( 'aria-controls' );
	}
}

$( document ).ready( function() {
	$( window ).on( 'load.twentyseventeen', onResizeARIA );
	$( window ).on( 'resize.twentyseventeen', onResizeARIA );
});

On load and resize events, the menu toggle is displayed when the viewport width is under '48em' and hidden otherwise. By the way, I don't see the point in updating its aria-expanded and aria-controls attributes here.

Trying to explain my point in the simplest possible way, please correct me if I'm wrong or I'm missing something. Let's read that if/else like: "when the menu toggle is displayed or hidden":

• the aria-controls attribute shouldn't be removed (also notice that currently it gets removed but never restored): when the menu toggle is hidden, it's not available to assistive technologies so there's no need to remove aria-controls
• all that's left now is the aria-expanded attribute:
  • on load: whether visible or hidden, the navigation menu is always collapsed and the menu toggle already has a default aria-expanded="false" so why all this should run on load?
  • on resize: the menu toggle aria-expanded attribute should be updated just when clicking on it; when it gets hidden or visible again, for example when changing a tablet orientation from landscape to portrait or vice-versa, its previous state should persist: if it was expanded, then it should still be expanded and vice-versa so why all this should run?

If I'm not missing something, I'd propose to completely remove this function and also any other handling of aria attributes on the siteNavigation element, as they don't bring in any accessibility benefit.
Any thoughts welcome.

[afercia]

39029.diff​ removes onResizeARIA() and other handling of aria attributes on the navigation menu. Fixes also #39026.

[sami.keijonen]

Makes perfect sense to me.

[laurelfulford]

I've tested, and 39029.diff​ looks good to me! :) The fix for #39026 is also more concise than the patch I included in that ticket - I think it would be better to go with the fix here.

[davidakennedy]

I tested this too, and it works well. I also like it over the patch(es) in #3026. This should be good to go.

[helen]

In 39451:

Twenty Seventeen: Improve ARIA for the nav menu.

The onResizeARIA() function was not very useful and buggy.

props afercia.
fixes #39029, #39026.

[helen]

In 39452:

Twenty Seventeen: Improve ARIA for the nav menu.

The onResizeARIA() function was not very useful and buggy.

props afercia.
fixes #39029, #39026 for the 4.7 branch.