Remove aria-pressed attribute from the primary menu toggle button

retagging

@MaddieC Unassigning you, as I don't want to hold up this issue if other people want to work on this. If you have work, please create a PR within the proof of concept repo on github. Also ping me in Drupal Slack (#d9-theme channel) if you need assistance!

Agree with removing the aria-pressed , but shouldn't the aria-expanded be on the menu toggle button also otherwise there is no feedback to the user on the state of header-menu and the #site-header__inner div is not a focusable element so that provides no feedback as to it's visibility state.

For progressive enhancement and resilience, the toggle button should be added, and header-nav hidden, via javascript.
Currently if js issue there is no access to header-menu just a button that does nothing.

Ideally the toggle button should be inside a nav element and be immediately followed by the menu, as currently for screen-readers when first landing on mobile page and listing landmarks there is only banner and main, so no navigation landmarks at all.
I appreciate that with current designs where search sits above menu that won't work so well, even if button was positioned in banner via css, it would break the tabbing order (search would be before toggle button in tab order).

I concur with what waako stated above regarding placing the menu's toggle button inside of the nav element in order for the navigation landmark to be detected. This resource might come in handy for whoever gets a chance to tackle https://www.a11ymatters.com/pattern/mobile-nav/

FWIW, it looks like the Umami theme encountered similar (https://www.drupal.org/project/drupal/issues/2940023 and https://www.drupal.org/project/drupal/issues/3042417), so perhaps it might also be worth looking towards what has been considered there?

Also noticed that aria attributes are not applied on load, only when menu is toggled.

No doubt a number of items like this will come out of the accessibility review, so might be worth postponing this issue for now?

Like cainaru says, we can leverage the work being done on Umami to get it just right here.

Thanks for the note. We have an accessibility review scheduled one week from today, but honestly, I want to get the known issues resolved beforehand.

I've done work towards fixing the aria-pressed, also removing the unnecessary aria-expanded on menu containers.
Also added a focus trap for mobile menu.

https://github.com/Lullabot/olivero-poc/pull/11

This looks great! Merged!

Good work!

As @waako says, the proposed solution in the issue summary isn't correct. I've updated the code sample to put aria-expanded in the correct place. If using ARIA, it's very important you use it correctly.

I'm not convinced by the keyboard focus trap:

• Could it be the cause of the problem in #3093455: Keyboard users can't go past the navigation?
• The focus trap, combined with the fact that the menu covers the entire viewport on small devices, kind of makes the menu more like a dialog. This could benefit from being marked up semantically as a dialog. Not completely sure about this.
• I noticed the escape key behaviour. That's good, but I wasn't sure where focus ended up after pressing it. It could be better to move focus back to the burger button.
• Note that the keyboard trap doesn't prevent screen reader users from navigating the rest of the page. If a sighted screen reader user does this, the menu is blocking them from seeing what the screen reader is doing. (This problem is really waiting for the HTML inert attribute support. Use of role=dialog with aria-modal might also help, but I'm not sure how well supported that is yet. )
• BTW the keyboard trap wasn't mentioned in the issue title or summary. Beware of scope creep like this, it makes it hard to follow accessibility progress. It's OK if you keep the issue summary up to date.

#5:

Ideally the toggle button should be inside a nav element and be immediately followed by the menu, as currently for screen-readers when first landing on mobile page and listing landmarks there is only banner and main, so no navigation landmarks at all.

Yes. This hasn't been addressed yet. The current arrangement of navigation landmarks is poor. At the moment, the navigation landmarks only exist when the menu button is expanded. If a user looks for a navigation landmark, they won't find one. These shouldn't disappear and re-appear, the landmark should always be there.

Next steps:
Improve the landmarks. nav button + ul is the element structure needed (and divs if you want).

The same thing needs to be done for the other disclosure button in the primary menu (products +). Currently this has very crappy keyboard behaviour; the submenu is just visually hidden, and appear when focus is within the sub-menu. Keyboard users have no choice but to tab through them. Better to hide the sub-menu links for all users, and only reveal them when the button is pressed. This problem affects both breakpoints.