WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#31527 closed enhancement (fixed)

Bundled themes: Add ARIA attributes to menu toggle

Reported by: davidakennedy Owned by: lancewillett
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.1
Component: Bundled Theme Keywords: 4.2-beta
Focuses: accessibility Cc:

Description

This patch enhances the accessibility of the mobile menu toggle by adding ARIA attributes to it, and changes them based on clicking the toggle. The aria-exapanded and aria-conrols communicate to screen readers and assistive technology what's happening on the screen.

The onResizeARIA function adds and removes the attributes based on the screen size since the toggle is only present on smaller screen sizes.

Attachments (12)

31527.patch (1.3 KB) - added by davidakennedy 7 years ago.
Twenty Fifteen ARIA Menu Toggle
31279.diff (3.7 KB) - added by lancewillett 7 years ago.
31527.2.patch (2.6 KB) - added by lancewillett 7 years ago.
31527.twentyfourteen.patch (1.6 KB) - added by lancewillett 7 years ago.
Add ARIA attribute toggle behavior to Twenty Fourteen
31527.twentythirteen.patch (1.8 KB) - added by lancewillett 7 years ago.
Add ARIA attribute toggle behavior to Twenty Thirteen
31527.twentyfourteen.2.patch (2.8 KB) - added by lancewillett 7 years ago.
Second pass at Twenty Fourteen
31527.twentythirteen.2.patch (3.1 KB) - added by lancewillett 7 years ago.
Second pass at Twenty Thirteen
31527.twentyfourteen.searchtoggle.patch (1.7 KB) - added by lancewillett 7 years ago.
Toggle ARIA properties on Twenty Fourteen's search behavior
31527.twentyfourteen.searchtoggle.2.patch (1.7 KB) - added by lancewillett 7 years ago.
Second pass at search toggle behavior for Twenty Fourteen
31527.twentyfifteen.onResizeARIADocBlock.patch (659 bytes) - added by davidakennedy 7 years ago.
Twenty Fifteen onResizeARIA Doc Block
31527.twentyfourteen.onResizeARIADocBlock.patch (662 bytes) - added by davidakennedy 7 years ago.
Twenty Fourteen onResizeARIA Doc Block
31527.twentythirteen.onResizeARIADocBlock.patch (662 bytes) - added by davidakennedy 7 years ago.
Twenty Thirteen onResizeARIA Doc Block

Download all attachments as: .zip

Change History (54)

@davidakennedy
7 years ago

Twenty Fifteen ARIA Menu Toggle

#1 @lancewillett
7 years ago

  • Milestone changed from Awaiting Review to 4.2

#2 @lancewillett
7 years ago

  • Keywords has-patch added

Thanks for the ticket and the patch. Should we also apply to older default themes?

#3 @lancewillett
7 years ago

Patch looks mostly good — only change I'd suggest is onResizeAria = function() { to function onResizeARIA() { for declaring the function.

@lancewillett
7 years ago

#4 follow-up: @lancewillett
7 years ago

  • Keywords needs-testing added

Ignore wrong patch, sorry. ;)

.2 patch changes things a tiny bit:

  1. Move event handlers to main ready clause and add twentyfifteen namespace.
  2. Move onResizeARIA() function out of anonymous function so it can be called from elsewhere in the file.
  3. Declare secondary and button variables earlier in the file so they can be used in any function.

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


7 years ago

#6 in reply to: ↑ 4 @davidakennedy
7 years ago

Replying to lancewillett:

Ignore wrong patch, sorry. ;)

.2 patch changes things a tiny bit:

  1. Move event handlers to main ready clause and add twentyfifteen namespace.
  2. Move onResizeARIA() function out of anonymous function so it can be called from elsewhere in the file.
  3. Declare secondary and button variables earlier in the file so they can be used in any function.

Thanks for cleaning the patch up and making it better, @lancewillett! Everything works as expected. I'll work this weekend on getting this type of patch into other default themes.

#7 @lancewillett
7 years ago

  • Summary changed from Twenty Fifteen: Add ARIA attributes to menu toggle to Bundled themes: Add ARIA attributes to menu toggle

Sounds great. I'll get Twenty Fifteen in now.

#8 @lancewillett
7 years ago

In 31644:

Twenty Fifteen: add ARIA attributes to menu toggle.

See #31527, props davidakennedy, lance.

#9 @DrewAPicture
7 years ago

  • Version changed from trunk to 4.1

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


7 years ago

#11 @DrewAPicture
7 years ago

  • Keywords 4.2-beta added

@lance ... errr @lancewillett: What's left here? :-)

#12 @lancewillett
7 years ago

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

Hah. My bad. ;)

We're waiting on patches for Twenty Fourteen and Twenty Thirteen. However, they can wait for 4.3 if we need to move on — I can just open new tickets for them since Twenty Fifteen is done.

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


7 years ago

@lancewillett
7 years ago

Add ARIA attribute toggle behavior to Twenty Fourteen

@lancewillett
7 years ago

Add ARIA attribute toggle behavior to Twenty Thirteen

#14 follow-up: @lancewillett
7 years ago

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

Patches attached for Twenty Fourteen and Thirteen -- testing and code review appreciated. :)

#15 @lancewillett
7 years ago

One note on Twenty Thirteen: using tertiary for the ARIA role of secondary since it's the sidebar; the widget area in the footer uses secondary ID, though.

#16 in reply to: ↑ 14 @davidakennedy
7 years ago

Replying to lancewillett:

Patches attached for Twenty Fourteen and Thirteen -- testing and code review appreciated. :)

Thanks for getting these in! They're close, but they need a bit more work.

In Twenty Thirteen:

  • the aria-expanded attribute isn't changing to true and back when the menu is toggled.
  • the aria-controls attribute should correspond with the id of the container being expanded, do either the div with a class of nav-menu or the ul inside of that div.

Same in Twenty Fourteen:

  • the aria-expanded attribute isn't changing to true and back when the menu is toggled.
  • the aria-controls attribute should correspond with the id of the container being expanded, do either the div with a class of nav-menu or the ul inside of that div.
  • We could also add aria-expanded and aria-controls to the search toggle. The difference is that it would always be there since it's always a toggle.

#17 @lancewillett
7 years ago

We could also add aria-expanded and aria-controls to the search toggle. The difference is that it would always be there since it's always a toggle.

So that would be an HTML change? In the header template, probably.

@lancewillett
7 years ago

Second pass at Twenty Fourteen

@lancewillett
7 years ago

Second pass at Twenty Thirteen

#18 follow-up: @lancewillett
7 years ago

Second pass at Twenty Thirteen and Twenty Fourteen based on davidakennedy feedback.

The search toggle changes to be done separately.

#19 @DrewAPicture
7 years ago

  • Owner set to lancewillett
  • Status changed from new to assigned

#20 in reply to: ↑ 18 @davidakennedy
7 years ago

Replying to lancewillett:

Second pass at Twenty Thirteen and Twenty Fourteen based on davidakennedy feedback.

The search toggle changes to be done separately.

@lancewillett I retested these both and they work as expected. Great job!

I found one wrinkle that I didn't think of before. If a custom menu is not assigned, the id won't be there to match the aria-controls. In that case, it relies on wp_page_menu instead of wp_nav_menu. I don't think wp_page_menu has a parameter to add an id, so we'd have to do something like:

/**
 *  Add ID attributes to the first <ul> occurence in wp_page_menu.
 *  Helps ARIA attributes match proper elements.
 */
function twentyfourteen_add_menuid( $ulid ) {
	return preg_replace( '/<ul>/', '<ul id="primary-menu">', $ulid, 1 );
}
add_filter( 'wp_page_menu','twentyfourteen_add_menuid' );

Not the cleanest solution, and maybe there's a better way I'm not thinking of right now. Sorry I didn't think of this before.

Thoughts?

#21 @lancewillett
7 years ago

Thanks, DK.

Not the cleanest solution, and maybe there's a better way I'm not thinking of right now.

Hmm. I hadn't thought of that case, either.

For a slightly cleaner solution, what about filtering link_before and link_after arguments for wp_page_menu() and adding in a wrapper element with the ID we require. div maybe. Could do it using the wp_page_menu_args hook.

It's really a bummer that classes don't work, but I can see why -- the control wouldn't be unique on the page otherwise.

#22 @lancewillett
7 years ago

However we decide to solve it, should be applied to both Twenty Thirteen and Fourteen as they both add the menu ID.

We also need to address the search toggle in a new patch.

#23 @lancewillett
7 years ago

In 31784:

Twenty Fourteen: add ARIA attributes to menu toggle. See #31527.

#24 @lancewillett
7 years ago

In 31785:

Twenty Thirteen: add ARIA attributes to menu toggle. See #31527.

@lancewillett
7 years ago

Toggle ARIA properties on Twenty Fourteen's search behavior

#25 follow-up: @lancewillett
7 years ago

For the ID on wp_page_menu() we could also suggest a core patch to add menu_id to the argument list. Both to pass a filterable argument and to also inherit from wp_nav_menu maybe.

#26 follow-up: @DrewAPicture
7 years ago

  • Keywords needs-docs added

I think we're going to need a DocBlock on the new onResizeARIA() function. See the relevant section in the JS documentation standards.

Edit: That goes for both Twenty Thirteen and Twenty Fourteen.

Last edited 7 years ago by DrewAPicture (previous) (diff)

#27 in reply to: ↑ 26 @lancewillett
7 years ago

Replying to DrewAPicture:

I think we're going to need a DocBlock on the new onResizeARIA() function.

Good point. Anyone want to take a stab at a patch? There are other functions in Twenty Fifteen's functions.js that have just a one-line comment, also.

#28 in reply to: ↑ 25 @lancewillett
7 years ago

Replying to lancewillett:

For the ID on wp_page_menu() we could also suggest a core patch to add menu_id to the argument list. Both to pass a filterable argument and to also inherit from wp_nav_menu maybe.

Added a ticket and patch to add this in with core, avoiding a filter in the theme: #31656

#29 @davidakennedy
7 years ago

@lancewillett:

I tested the search toggle ARIA addition. Let's simplify it. We only really need to add the ARIA attributes to the a href="#search-container"> because that's the control a keyboard user or screen reader user would toggle it with. And that's what will give them the feedback if they're focused on it. The other elements are not focusable, so they would not hear the ARIA feedback. Also, currently, the ARIA attributes there do not change, it stays: aria-expanded="false".

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


7 years ago

@lancewillett
7 years ago

Second pass at search toggle behavior for Twenty Fourteen

#31 @davidakennedy
7 years ago

@lancewillett:

31527.twentyfourteen.searchtoggle.2.patch for the ARIA on the search toggle looks good. Works as expected. Great job!

#32 @lancewillett
7 years ago

In 31794:

Twenty Fourteen: add ARIA attributes to search toggle. See #31527.

#33 follow-up: @lancewillett
7 years ago

Two things to close this ticket:

1) DocBlocks
2) Decision & commit on #31656 for the page menu fallback ID for Twenty Fourteen and Thirteen

#34 @DrewAPicture
7 years ago

For those following along at home, my suggestion to @lancewillett in Slack was:

My suggestion is to just add the docs even if sparse everywhere else. Sends kind of a bad message if we don’t apply docs standards everywhere going forward.

@davidakennedy
7 years ago

Twenty Fifteen onResizeARIA Doc Block

@davidakennedy
7 years ago

Twenty Fourteen onResizeARIA Doc Block

@davidakennedy
7 years ago

Twenty Thirteen onResizeARIA Doc Block

#35 in reply to: ↑ 33 @davidakennedy
7 years ago

Replying to lancewillett:

Two things to close this ticket:

1) DocBlocks
2) Decision & commit on #31656 for the page menu fallback ID for Twenty Fourteen and Thirteen

Added the DocBlock patches.

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


7 years ago

#37 @lancewillett
7 years ago

Possible to have decision & commit on #31656 also? It's a minor change.

#38 @lancewillett
7 years ago

In 31814:

Bundled themes: add documentation for new onResizeARIA function.

See #31527. Props davidakennedy, lancewillett.

#39 @DrewAPicture
7 years ago

  • Keywords needs-docs removed

#40 @lancewillett
7 years ago

  • Keywords has-patch needs-testing removed
  • Resolution set to fixed
  • Status changed from assigned to closed

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


7 years ago

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


6 years ago

Note: See TracTickets for help on using tickets.