Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#39029 closed defect (bug) (fixed)

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

Reported by: afercia's profile afercia Owned by: helen's profile helen
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Bundled Theme Keywords: has-screenshots has-patch commit dev-reviewed
Focuses: accessibility, javascript Cc:

Description

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:

https://cldup.com/WfPNj06i5F.png

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.

https://cldup.com/I7gSRDxImX.png

https://cldup.com/FdzdRyu9Hh.png

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.

Attachments (1)

39029.diff (2.7 KB) - added by afercia 8 years ago.

Download all attachments as: .zip

Change History (10)

@afercia
8 years ago

#1 @afercia
8 years ago

  • Keywords has-patch added; needs-patch removed

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

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


8 years ago

#3 @sami.keijonen
8 years ago

Makes perfect sense to me.

#4 @laurelfulford
8 years ago

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.

#5 @davidakennedy
8 years ago

  • Keywords commit added

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

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


8 years ago

#7 @helen
8 years ago

  • Keywords dev-reviewed added
  • Milestone changed from Awaiting Review to 4.7

#8 @helen
8 years ago

  • Owner set to helen
  • Resolution set to fixed
  • Status changed from new to closed

In 39451:

Twenty Seventeen: Improve ARIA for the nav menu.

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

props afercia.
fixes #39029, #39026.

#9 @helen
8 years ago

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.

Note: See TracTickets for help on using tickets.