Automattic / _s

Hi. I'm a starter theme called _s, or underscores, if you like. I'm a theme meant for hacking so don't use me as a Parent Theme. Instead try turning me into the next, most awesome, WordPress theme out there. That's what I'm here for.
http://underscores.me/
GNU General Public License v2.0
10.96k stars 3.12k forks source link

Remove aria-expanded on menu UL element? #1289

Closed samikeijonen closed 4 years ago

samikeijonen commented 6 years ago

Just leaving note here before I forget. We should check do we really need aria-expanded on menu (<ul>) itself. I'm thinking we don't need it.

Here is the JS line for it.

mrwweb commented 6 years ago

Unless this was included due to some type of AT implementing the spec wrong, it's clear that aria-expanded only goes on the control, not the container.

There are also two other lines that would need to be changed.

jaredcaraway commented 6 years ago

I did some research on this today, and I agree with @samikeijonen and @mrwweb - unless I'm overlooking something, I don't see anything supporting the need for the aria-expanded attribute on the menu <ul> itself.

From WAI-ARIA Authoring Practices 1.1:

  • The element that opens the menu has role button.
  • The element with role button has aria-haspopup set to either menu or true.
  • When the menu is displayed, the element with role button has aria-expanded set to true. When the menu is hidden, it is recommended that aria-expanded is not present. If aria-expanded is specified when the menu is hidden, it is set to false.
  • The element that contains the menu items displayed by activating the button has role menu.
  • Optionally, the element with role button has a value specified for aria-controls that refers to the element with role menu.
  • Additional roles, states, and properties needed for the menu element are described in 3.14 Menu or Menu bar.

There is also a Navigation Menu Button Example which shows this in action - the aria-expanded attribute is only applied to the button element. screenshot from 2018-06-27 16-18-45

It's worth noting some differences from the WAI-ARIA doc:

I'll go ahead and submit a pull request with the aria-expanded attributes removed from the menu element. I'd love to get some input on the other points I addressed above - are they worth implementing or is there a specific reason they're omitted/should not be added?

grappler commented 6 years ago

Thank you @jaredcaraway for the PR.

It is recommended to remove the aria-expanded attribute when the element is not expanded rather than setting its value to false, but that approach is not explicitly incorrect

I am sure the JS code could be updated to support this. This was previously added with the issue https://github.com/Automattic/_s/issues/545

For thoroughness, it looks like the button should have an attribute - aria-haspopup - set to "true" or "menu"

I don't have an opinion or more information on this.

WAI-ARIA states that the menu element should have role="menu", but it seems since it's wrapped in a

I am not sure if the theme is the place for this. I did find two core tickets sort of discussing this.

https://core.trac.wordpress.org/ticket/29475 https://core.trac.wordpress.org/ticket/35127

samikeijonen commented 6 years ago

There is no need for aria-haspopup in button, nor role="menu". <nav> is already a native landmark element. More info in article don't use aria menu roles in site nav.