Make WordPress Core

Opened 5 months ago

Closed 4 months ago

Last modified 4 months ago

#62892 closed defect (bug) (fixed)

Twenty Twelve: Add aria-expanded to menu button (mobile)

Reported by: bschneidewind's profile bschneidewind Owned by: joedolson's profile joedolson
Milestone: 6.8 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch commit
Focuses: accessibility Cc:

Description

The aria-expanded and aria-controls attributes are missing on the mobile nav for the Twenty Twelve theme: https://wp-themes.com/twentytwelve/

Change History (13)

This ticket was mentioned in PR #8235 on WordPress/wordpress-develop by @bschneidewind.


5 months ago
#1

  • Keywords has-patch added

This adds the aria-expanded and aria-controls attributes to the mobile nav of the Twenty Twelve theme - they currently are not present, ex: https://wp-themes.com/twentytwelve/

Trac ticket: https://core.trac.wordpress.org/ticket/62892

umeshie commented on PR #8235:


5 months ago
#2

@bschneidewind Please use const and let for block-scoped variables (no var).

umeshie commented on PR #8235:


5 months ago
#3

@bschneidewind Please use const and let for block-scoped variables (no var).

@umeshsinghin commented on PR #8235:


5 months ago
#4

@bschneidewind Please use const and let for block-scoped variables (no var).

#5 @sabernhardt
5 months ago

  • Component changed from Themes to Bundled Theme

#6 @joedolson
5 months ago

  • Milestone changed from Awaiting Review to 6.8
  • Owner set to joedolson
  • Status changed from new to accepted

@joedolson commented on PR #8235:


5 months ago
#7

@umeshsinghin While that's generally good advice, it's generally WordPress policy to keep things stylistically consistent within the context they're written. Since most of this file uses var to declare variables, it isn't a requirement.

Note the JavaScript coding standards, which explicitly indicate that const and let should be used in code written using ES2015 or newer; which is definitely not the case with Twenty Twelve.

It's not a problem to use more modern notations in older files, but not necessary, either.

@sabernhardt commented on PR #8235:


5 months ago
#8

I think this can be done without declaring new variables by editing the main function (and reusing its button and menu variables). I also recommend setting aria-expanded="false" in the script file in case a child theme overrides the header template.

	// Assign an ID for the default page list if no menu is set as Primary.
	if ( ! menu.id ) {
		menu.setAttribute( 'id', 'twentytwelve-page-list-menu' );
	}

	button.setAttribute( 'aria-controls', menu.id );
	button.setAttribute( 'aria-expanded', 'false' );

	button.onclick = function() {
		if ( -1 === menu.className.indexOf( 'nav-menu' ) ) {
			menu.className = 'nav-menu';
		}

		if ( -1 !== button.className.indexOf( 'toggled-on' ) ) {
			button.className = button.className.replace( ' toggled-on', '' );
			menu.className = menu.className.replace( ' toggled-on', '' );
			button.setAttribute( 'aria-expanded', 'false' );
		} else {
			button.className += ' toggled-on';
			menu.className += ' toggled-on';
			button.setAttribute( 'aria-expanded', 'true' );
		}
	};

@bschneidewind commented on PR #8235:


5 months ago
#9

Thanks @sabernhardt ~ just adjusted the approach!

#10 @joedolson
4 months ago

  • Keywords commit added

Tested and reviewed; lgtm.

#11 @joedolson
4 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 59911:

Bundled Themes: Twenty Twelve: Add ARIA attributes on menu toggle.

Add aria-expanded and aria-controls attributes to the Twenty Twelve mobile menu toggle.

Props bschneidewind, joedolson, sabernhardt, umeshsinghin.
Fixes #62892.

#12 @joedolson
4 months ago

In 59912:

Bundled Themes: Twenty Twelve: Move skip link to top of body.

Move the skip link to the top of the body content, so it is the first focusable item on the page.

Props joedolson, sabernhardt, abcd95, vgnavada, shailu25.
Fixes #62892.

#13 @joedolson
4 months ago

I accidentally cited this in two commit messages; the first one is correct.

Note: See TracTickets for help on using tickets.