Navigation side bar and top bar should have appropriate aria labels

marking as bug because it's an a11y regression

The <li id="navigation-link-navigationcreate"> tag id is used on the buttons as aria-controls="navigation-link-navigationcreate". This is important for screenreaders to understand the relationship between the button and the <li> it controls.

I am in favor of not removing the ID attr from <li>.

It's small change but issue is marked major and seems to be a regression (or revert). So think some test coverage would be good, just a simple assertion

Still would vote to added test coverage though

Removing the "prove the need" because it's proven already that this is necessary to fix #3438976: Implement a caching strategy for the menu links. Discussing this with @plopesc.

Adding the "Drupal CMS release target" to see if we could hopefully get to it.

The MR adds these back in the twig template, but the issue summary and linked issue mention it needs to happen via js to prevent caching issues. Also I can't yet find an explanation of what the caching issues are/were anywhere yet. Just looking at the MR I don't see why that would be a problem but could be missing something.

Those IDs can still be there, since we are changing the way to improve the Navigation performance.

The <h4> IDs were removed in https://git.drupalcode.org/project/drupal/-/merge_requests/7980/diffs?co... and they need to be brought back using JS for a11y reasons.

The <li> IDs are still in navigation-menu.html.twig. As part of this issue, it was suggested to remove them from the twig template and bring them back using JS to make the html markup cacheable. It seems that will not be necessary anymore, or at least for now.

Removing that part from the issue requirements seems safe to me.

They're not directly related to caching, but the way it was supposed to cache the navigation HTML markup in local storage to not serve it in every request was not compatible with having IDs in the navigation HTML markup.

All the discussion and the reasoning is available in #3438976: Implement a caching strategy for the menu links.

It appears to me that the changes in commit b99b9552 and commit d68ff2c9 are causing a11y issues because when there’s more than one menu on the page, each nav needs to have a unique label per WCAG 2.2 1.3.1: Info and Relationships (Level A) and 4.1.2: Name, Role, Value (Level A)

I'm also seeing a related accessibility issues in that each menu should contain a wrapping <nav> element

The unique aria-labelledby attribute should be added to the <nav> element instead of the <ul> element. Per W3C ARIA Landmark Example > Navigation Landmark, here is how a menu should be rendered when there's more than one menu on the page:

<nav aria-labelledby="nav1">
  <h2 id="nav1">Title for navigation 1<h2>
  <ul>
    <li><a href="page11.html">Link 1</a></li>
    <li><a href="page12.html">Link 2</a></li>
    .....
  </ul>
</nav>
<nav aria-labelledby="nav2">
  <h2 id="nav2">Title for navigation 2<h2>
  <ul>
    <li><a href="page11.html">Link 1</a></li>
    <li><a href="page12.html">Link 2</a></li>
    .....
  </ul>
</nav>

You can also skip the heading all-together and add an `aria-label` to the `` element itself.

<nav aria-label="Title for navigation">
  <ul>
    <li><a href="page11.html">Link 1</a></li>
    <li><a href="page12.html">Link 2</a></li>
    .....
  </ul>
</nav>

An important note is that the headings in both commits are both set to be .visually-hidden and .focusable which is an issue, because you then have visually headings that are keyboard focusable.

Other resources explaining this reasoning:

• ARIA Authoring Practices Guide (APG) > Landmark Regions
• W3C WAI > Menu Structure
• W3C WAI > Labeling Regions

Hope it helps.

Initial code in the MR looks good.

Tested locally and worked as expected.

Fixed the Kernel tests failing due to conflicts between DomDocument::loadHtml and HTML5 tags (https://www.php.net/manual/en/domdocument.loadhtml.php)

Updated the issue summary to reflect the last requirements from a11y team.

I think this is ready for final review.

Review steps

1. Install Drupal 11.x.
2. Enable Navigation module.
3. Check menus and inspect the html for them.

See evidences:

Content Menu

Administration Menu

Help Menu

Everything works as expected ✅

Moving to RTBC.

Addressed some duplication in the navigation titles for non-sighted users. See last comment and commit.

Ready for a re-review and accessibility review.

Went to review as I got some accessibility knowledge/tools. But issue summary appears incomplete in that the proposed solution just says "Implement this" but MR contains a number of changes so think solution should be properly flushed out.

Will try and pick up once complete.

Good point @smustgrave. Updated the issue description. Back to Needs Review.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Merged the latest from the 11.x branch. Back to Needs review.

Can we get a CR for the template changes? In case some theme have overridden the templates

The module is still experimental and templates are considered @internal, so I don't think we need a CR here - doesn't hurt to have one but not a blocking issue, whereas it looks like an accessibility review still is.

@finnsky @m4olivei Thank you for rebuilding the tugboat. The Accessibility team, including @rkoller and I, tested the latest work. I just now tested the latest tugboat against the Apple VoiceOver screen reader and this is what I'm seeing in its rotor output and the HeadingsMap browser extension. I'm seeing the following (with screenshots):

Headings: Toolbars have three nav menus with "Administration" heading labels

Landmarks: The toolbar's landmarks contains some generic "Navigation" titles, likely because they have empty headings

In addition, during testing with the Drupal Accessibility Team, we came up with some recommendations that @rkoller has thankfully outlined. The following steps should fix these navigation issues for users of assistive technology:

1. Complementary renamed to:

• administrative sidebar complementary

2. Administrative toolbar content navigation gets replaced by:

• shortcuts navigation (one menu item)
• content navigation (six menu items)
• administration navigation (seven menu items)

3. Administrative toolbar footer navigation:

• help navigation (one menu item)
• user navigation (one menu item)

So we'd end up with the following:

Before

• Complementary
• Administrative toolbar content navigation
• Administrative toolbar footer navigation

After

• administrative sidebar complementary
• shortcuts navigation
• content navigation
• administration navigation
• help navigation
• user navigation

Please find details on this proposal in this Google doc, specifically the action items section of the proposal.

Moving to needs work for #50.

2. Administrative toolbar content navigation gets replaced by:

shortcuts navigation (one menu item)
content navigation (six menu items)
administration navigation (seven menu items)

3. Administrative toolbar footer navigation:

help navigation (one menu item)
user navigation (one menu item)

It looks like this could probably be achieved by moving the aria markup to the individual navigation block plugins.

Oh good spot in #52, that's definitely the old toolbar, completely missed that given shortcuts, user etc. also exist in the new navigation.

I’ve taken another look at the current state of the MR and created two small videos to illustrate the difference between the current state of the navigation (check 11.x.mp4) and the state in the MR (check MR.mp4)

The point that still needs some more discussion is the level of granularity of landmarks, as outlined in the first question in the google doc by @katannshaw. In 11.x you currently have the complementary wrapping landmark and the two navigation landmarks for the content and the footer section (11.x.mp4). With the MR you now have the complementary wrapping landmark and another landmark for each of the five navigation blocks (MR.mp4) - three of those blocks even have just a single menu item.

We have discussed the matter in the accessibility office hour end of march where we havent come to a consensus which of the variants outlined in the first question of the google doc should be picked. Back then we agreed to ask for feedback over on the accessibility slack about the preferences and expectations of screenreader users in regard to landmark regions - reason is that quite a few screenreader users are part of the community there. But unfortunately we havent received any actionable feedback yet :( In addition to that i’ve posted over in the CMS Garden Rocketchat instance and asked in their a11y channel as well.

The worry and the reason why we tried to get feedback from actual screenreader users simply is, that the number of navigation landmarks might be way too granular - to reach the main landmark with the MR applied you have to pass by eight landmarks first (five navigation landmarks and two more complementary landmarks, both wrappers for the navigation sidebar and the topbar). The docs of the skipto script (https://skipto-landmarks-headings.github.io/page-script-5/config.html) state something in a similar vein:

The list of landmarks and headings should be relatively short, the more items the menu contains the more time the user will need to scan and navigate to the section they want to “skip to”.

“Strictly” speaking, landmarks should segment a page into semantically clearly distinguishable sections, so a user is able on one hand to understand the sites general structure and on the other hand is able to skip to the section in question more easily. but the five navigation blocks are semantically too close/similar, since together they build the main navigation aka navigation sidebar.

The other minor problem i see, the complementary landmark that you get with the aside element feels semantically wrong, complementary is about supporting the main content (https://www.w3.org/WAI/ARIA/apg/practices/landmark-regions/), which might be the right choice in cases like for example https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/nav, but for something like the main navigation it feels not like the best pick?

Personally I would lean towards a solution with two landmarks, one for the navigation sidebar and one for the navigation topbar. For the sidebar i would use a navigation landmark and label it with primary, that way it would be crystal clear that this is the main navigation for the admin ui. For the topbar the complementary landmark is fine. But that is only my personal opinion. Curious what Kat and others think about it, plus i still hope to get some feedback from actual screenreader users, those who actually use landmarks on a daily basis would be the most helpful answering this question. :/

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Personally I would lean towards a solution with two landmarks, one for the navigation sidebar and one for the navigation topbar. For the sidebar i would use a navigation landmark and label it with primary, that way it would be crystal clear that this is the main navigation for the admin ui. For the topbar the complementary landmark is fine.

From your description, this sounds sensible to me. Having to click past eight landmarks seems unreasonable. People could also add even more blocks on real sites.

I personally think we should go ahead with that idea here, and open a follow-up to review it with more feedback for actual screen reader users. This is the last blocker to navigation being stable but it's also something that can be tweaked without BC implications once it is stable.

+1 for the two landmarks

Seems like there is agreement on the suggested approach of having two landmarks but can someone update the IS with exactly what the proposed markup should be?

I hate to bikeshed this.

From @rkoller in #56:

For the sidebar i would use a navigation landmark and label it with primary, that way it would be crystal clear that this is the main navigation for the admin ui.

I worry that on the front-end of the site, the primary label would be confused with the primary navigation of the site. My thought is it could be named administrative toolbar Thoughts?

Looking at the 11.x, the current top bar is is an <aside> with an aria-labelledby attribute that points to its visually-hidden heading, with the text "Administrative top bar"

From my point of view, I don't see any reason to change this (please correct me if I'm wrong).

There are currently two <nav> elements in the sidebar, which we will consolidate to one.

If all this looks correct, I'll update the IS. I also feel like we might need to have a discussion in Slack to make sure we're on the same page. The requirements of this issue keep changing, and we need to lock it down to make progress.

I worry that on the front-end of the site, the primary label would be confused with the primary navigation of the site. My thought is it could be named administrative toolbar Thoughts?

This makes sense to me.

Looking at the 11.x, the current top bar is is an with an aria-labelledby attribute that points to its visually-hidden heading, with the text "Administrative top bar"

From my point of view, I don't see any reason to change this (please correct me if I'm wrong).

If this is for screenreaders, is 'top bar' meaningful? Should it be something like 'Contextual administration bar'?

I love this idea @rkoller and @mherchel's point about the primary nav is 100% valid. Once the extra landmark found by @roller (mentioned in comment #71) is resolved, this looks good to me. Thank you for your work on this.

i've applied the latest changes and the landmarks look good now imho (see landmarks.mp4). and left one comment in regard to the word sidebar over on gitlab

and in regard to the labels for the landmarks referring to #63. i completely agree to the concern @mherchel raised. in the latest iteration the landmark labels are (also see landmarks.mp4):

"administrative sidebar" navigation
"administrative top bar" complementary

that way they are clearly distinguishable from any menu in the frontend. the only worry i have, for sighted users or in the general discussion on slack, the issue queue, or forums, the navigation is addressed by either navigation or navigation sidebar / top bar or navigation top bar. having a different name/label in the aural interface for that component, screenreader users might have trouble searching for something like "administrative sidebar" in the documentation. that might be potentially confusing.

As for the region names themselves, that's something we can decide and adjust later without rushing.

I think a follow-up to review alternatives for the region names is a good idea, and I'd be happy going ahead with the not-bad-but-possibly-not-the-best-we-can-do names in this issue so that the HTML structure is stable and the noise from too many landmarks is gone.

i agree the naming of the two landmarks can be moved to a followup. uhhh and sorry there is another detail i've just noticed. if you go to admin/config/user-interface/navigation-block and enable the edit mode (and for example move one nav block there), in the rotor you see the landmark for the administrative sidebar navigation twice:

and strictly speaking it is not the actual administrative sidebar navigation but a dummy you are unable to navigate with. should that maybe moved to a follow up?

I nearly opened a new issue for #78 but instead I've explicitly referenced it from #3521155: Remove Layout Builder dependency for the UI of Navigation admin config so it gets tackled in that issue. It's likely the markup for that page will change dramatically but we'd want to make sure we don't reproduce the same issue when that's done.

Opened #3539715: Review navigation landmark naming for the naming.

I've also updated the issue summary with the proposed resolution that's actually in the MR/discussion now - it was still suggesting adding them to individual blocks.

Trying an issue re-title.

Just a heads-up that in #3093378-35: Use ARIA disclosure pattern for submenu buttons in vertical toolbar orientation @cwilcox808 cautioned against using aria-label instead of hidden text for buttons due to lack of support in translation software.

I inquired about similar concerns with other html elements for this issue in Slack.

I think we should go ahead here and review that more generally if it's an issue for non-button text, this isn't the only place with use aria labels and for me it needs a policy discussion in its own issue.

Rebasing without merge commit.

Rebased without merge commit.

Tested, and reviewed: ok

@katannshaw in #3452724-31: Navigation side bar and top bar should have appropriate aria labels #31 laid out that the title should persist very well and it looked like someone added it back as aria-label in https://git.drupalcode.org/project/drupal/-/merge_requests/11122/diffs?c... I didn't see where it got dropped again but just wanted to raise that is is gone and want to ensure that is intentional and not incidental.

If I'm reading correctly, it was removed in https://git.drupalcode.org/project/drupal/-/merge_requests/11122/diffs?c... which removes the aria-label from navigation-menu.html.twig altogether - this is because that template is for the nested individual menus (admin, user etc.) within the navigation toolbar, not for the toolbar itself - and in that case it would be 100% intentional to reduce the labels down to only the main side and top bars and not the nested items.

Fixed code review.

Thanks for the twig coding standard fixes, sorry I can't mark them as resolved in gitlab @grimreaper.

@catch Thanks for taking a stab at figuring out why the menu title/label was dropped!

I suspect that’s exactly why the aria-label/heading disappeared, but the result is that each navigation block now renders as an unlabeled list “Shortcuts”, “Content”, “Create”, etc. never reach the DOM when the block label is hidden in config. We still need an accessible name for each block even if we drop the redundant , so I’d like to restore a hidden heading or similar so screen readers can differentiate those sections again.

I would like to end in that I am not an a11y expert nor do I use a screen reader, so feel free to overrule me if someone on this thread is, I just don't want things to unintentionally get worse for those who need it. I'll try to reach out to @katannshaw in Slack to get a take as that was very well described in #31.

@joelpittet Thanks for asking. To follow accessibility standards I mentioned in my previous comment in another post titled Navigation side bar and top bar should have appropriate aria labels, I do advise that each <nav> element on a page have a unique heading that's connected to the <nav> using ARIA. In my opinion, it's best to use this pattern:

<nav aria-labelledby="heading-id">
<h2 id="heading-id">Nav Title</h2>
</nav>

This pattern:
* Allows the heading label to show up in the screen reader's list of headings

* Allows the navigation label to show up in the screen reader's list of landmarks

Both are used for navigation purposes. Here's an example of what the web rotor looks like from The A11y Project > Getting started with macOS VoiceOver:

I hope this helps.

@finnsky Right, re #94 we’re on the same page: aria-labelledby and aria-label give equivalent results, so it’s good to confirm the principle.

The snag is in #97: with the current MR the block title is gone entirely. In navigation_menu.html.twig we dropped both the <nav aria-label="{{ title }}"> and the hidden <h4>{{ title }}</h4> so nothing else outputs the name. That leaves a single sidebar landmark full of unlabeled lists. I looped @katannshaw as she mentioned this earlier up and we really do need to surface that label somewhere... either by restoring the hidden heading or wiring the title into an aria-label/aria-labelledby — so each navigation block remains discoverable. Two landmarks is fine, but we still need names for the menus inside them.

In a (hot) minute I will do a test locally with the instructions for @katannshaw mentioned above as it clarifies what I couldn't sort out of what

VO means for keyboard shortcut
so VO + F1 means Control + Option + F1

That was throwing me when I tried to test this out the first time, I couldn't figure out VO meant.

Again, I am not an expert, but I also don't want to lose valuable information for those who need it, so if you can prove it's not needed, I’d be happy to hear that advice too. Just nobody mentioned the reason behind dropping it, from my scan of the comments.

@joelpittet I think the problem is that a nav with an aria label becomes a landmark, which is what we've been specifically trying to avoid since #3452724-56: Navigation side bar and top bar should have appropriate aria labels. I'm basing this on https://www.w3.org/WAI/ARIA/apg/patterns/landmarks/examples/navigation.html

So if we add the aria labels back, we go back to 7 or more landmarks instead of two.

The options then would be:

1. Continue to use nav without a label, because the nav elements are wrapped in another landmark, which is what the MR was previously doing when RTBCed.

2. Stop using the nav element for the individual menu items.

If we can agree that the approach results in the correct number of landmarks, I'd prefer us to move the discussion of the exact markup for individual menus to a follow-up probably.

I think the problem is that a nav with an aria label becomes a landmark, which is what we've been specifically trying to avoid since #3452724-56: Navigation side bar and top bar should have appropriate aria labels. I'm basing this on https://www.w3.org/WAI/ARIA/apg/patterns/landmarks/examples/navigation.html

So if we add the aria labels back, we go back to 7 or more landmarks instead of two.

The options then would be:

1. Continue to use nav without a label, because the nav elements are wrapped in another landmark, which is what the MR was previously doing when RTBCed.

2. Stop using the nav element for the individual menu items.

My understanding is that nav elements without names are still landmarks (they're just unnamed). Here's screenshot of VoiceOver from a pared-down demo of various submenus inside a labeled parent nav.

If it's any guidance, the ARIA Authoring Practices Guide Example Disclosure Navigation Menu doesn't wrap the dropdown submenus in nav elements. It looks like this would correspond to option #2 above: "Stop using the nav element for the individual menu items".

[edited to clarify the option #2 that I was referring to]

Followup:

In my last screenshot, "Submenu 4" is not wrapped in a nav and so doesn't appear in the rotor landmarks menu.

Don’t mind me if this doesn’t help but I’m fond of ANDI toolbar it’s what DHS accessibility office makes us take their training in. It announces landmarks

@kentr thanks that's really useful.

So, to me to match the issue summary I think we should probably do #99.2, which would mean changing the nav for the individual menus into div. And the basis for this would be that we consider the navigation sidebar to be 'three (or more or less) menus in a trenchcoat' e.g. just one navigation element that happens to be subdivided due to site configuration and defaults.

I think it then might be worth opening a follow-up to see if there's a better way to represent that subdivision than divs, but that wouldn't be stable blocking then.

Ahh good point! But this means that given they're now not navs, we could introduce an aria label or hidden title per #98 without them becoming landmarks again.

Added a suggestion on the MR to bring back the hidden titles, not sure the best way to do aria labels there.

The menu titles appear to be the same as the block labels.

There's already the optional block label display in block--navigation.html.twig.

  {% if label %}
    <h2{{ title_attributes.addClass('toolbar-block__title').setAttribute('data-drupal-tooltip', label).setAttribute('data-drupal-tooltip-class', 'toolbar-block__title-tooltip') }}>{{ label }}</h2>
  {% endif %}

However, AFAICT this code not used by default because the module install configuration sets label_display: '0'.

Does it make sense to leverage that instead of adding a heading to navigation-menu.html.twig?

[edited to correct typo]

#107 shows we can't use the block titles, but I think we could add back the hidden title per #105 and it's (hopefully?) a one-line change to the template.

I don't have a strong opinion on whether that should be explored here in a follow-up, but would be very happy if we could unblock this again.

Instead of adding back the hidden menu titles per #105, what about reworking the block titles as so:

  If (label_display)
    Display the block title/label normally.
  Else
     Hide the block title/label visually.

Otherwise:

I imagine problems when both the hidden menu title is added back per #105 and the blocks have been customized to display the block title per #107.

• There will be redundant adjacent text (with the menu titles hidden visually but perceivable to screen reader users).
• If the hidden menu titles (per #105) are h* elements, both these title and the block titles will display as headings to screen reader users.

AFAIK headings would cause navigation problems. But even if the menu titles were standard text instead of headings, it seems this would be confusing verbosity. A screen reader expert should double-check me on this.

Examples:

Here's a screenshot of the VoiceOver rotor with the block titles displayed and the hidden menu titles added back as headings on line 2 of navigation-menu.html.twig (per #105):

Here's a screenshot with the block titles visually-hidden in block--navigation.html.twig and without the hidden menu titles added back in navigation-menu.html.twig. The only headings in the menu are the block titles.:

[edited for clarity]

Just discussed this with @ckrina at Drupalcon Vienna and we both think doing #110 makes a lot of sense, so would be great to go ahead with that. Moving to needs work.

@oily the proposed solution here is in #110. The suggestion you applied doesn't match that. If you'd like to try implementing #110 instead that could help get this issue done.

I added a code suggestion.

Then edited the IS. Have added the screenshots with explanatory text from #110 to the IS.

This section of the IS probably needs replacing but best done by those more knowledgeable of the history of this issue than me:

Proposed resolution

Add label "Administrative side bar" to the navigation side bar. This aligns nicely with "Administrative top bar" label that we already have for the <aside> element that wraps the Top Bar.

We decided in this issue to explicitly not add aria labels for specific navigation blocks because this resulted in too many landmarks on the page - the one each for side and top bar are sufficient and a good balance.

@oily #110 explains why unconditionally adding the block title as a heading could cause problems, and suggests only conditionally adding it when label display is off, see the start of that comment.

Changing the test and opening a follow-up seems good.

Patch applies and works. RTBC.

Opened #3557571: Define shortcut navigation block behaviour when there are no shortcuts for the shortcuts follow-up.

Don't think this needs a change record, it's a small markup change to the navigation bar which is not stable yet. If we were changing the markup for something stable and visitor-facing then a change record is useful, but this should not really be overridden.

Removing accessibility review tag because it's had extensive accessibility review at this point.

Also this was a task/bug, not a feature request - just noticed now after more than 100 comments..

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!