- Issue created by @finnsky
- Status changed to Active
11 months ago 8:21pm 22 June 2024 - First commit to issue fork.
- Merge request !8509ID attr on h4 and aria-labelledby attr added on ul → (Open) created by Unnamed author
- Status changed to Needs review
11 months ago 3:56am 24 June 2024 - 🇮🇳India gauravvvv Delhi, India
The
<li id="navigation-link-navigationcreate">tag id is used on the buttons asaria-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>. - Status changed to Needs work
11 months ago 1:26pm 24 June 2024 - 🇺🇸United States smustgrave
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
- 🇷🇸Serbia finnsky
We just removed that from Twig in linked ticket. In current ticket we need to decide if that title needed or not.
And if yes we need to add this with JS because of caching reasons.Also hat titles were added initially from first days of development. And then we created others on block level. So probably they are artefacts and can be ignored.
- 🇷🇸Serbia finnsky
marking as bug because it's an a11y regression
We not sure yet.
- 🇪🇸Spain ckrina Barcelona
Removing the "prove the need" because it's proven already that this is necessary to fix 📌 Implement a caching strategy for the menu links Active . Discussing this with @plopesc.
- 🇷🇸Serbia finnsky
I think it still requires some prove label in title ;)
I see now real chaos in titles
https://gyazo.com/0f847b120acbbc46226ddfc0cf47815e
we have h2 and h4. which is real title here? and which one should be aria label? - 🇪🇸Spain ckrina Barcelona
Adding the "Drupal CMS release target" to see if we could hopefully get to it.
- 🇬🇧United Kingdom catch
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.
- 🇪🇸Spain plopesc Valladolid
Those IDs can still be there, since we are changing the way to improve the Navigation performance.
- 🇷🇸Serbia finnsky
Prove the need
was in the title not because of caching but because of a11y.
- 🇪🇸Spain plopesc Valladolid
The
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
- 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.
- IDs are still in
- 🇷🇸Serbia finnsky
I was saying that these headings were there from the very beginning of the project and I don't think anyone ever seriously looked into the semantics or accessibility of the headings. That's why we removed them.
What we really need is a quality review of accessibility and structure. And depending on the review results, add or remove headings. I'm not sure they are needed at all.
@ckrina wrote in #22
Removing the "prove the need" because it's proven already that this is necessary to fix #3438976: [PP-2] Implement a caching strategy for the menu links. Discussing this with @plopesc.
But I don't understand how caching is related to accessibility and semantics
- 🇪🇸Spain plopesc Valladolid
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 📌 Implement a caching strategy for the menu links Active .
- 🇺🇸United States katannshaw Kansas
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>elementThe unique
aria-labelledbyattribute 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>Other resources explaining this reasoning:
- ARIA Authoring Practices Guide (APG) > Landmark Regions
- W3C WAI > Menu Structure
- W3C WAI > Labeling Regions
Hope it helps.
- 🇷🇸Serbia finnsky
One backend fix need to be applied here. We can go with here.
- First commit to issue fork.
- 🇪🇸Spain plopesc Valladolid
Updated the issue summary to reflect the last requirements from a11y team.
Tested locally and worked as expected.
Fixed the Kernel tests failing due to conflicts between
DomDocument::loadHtmland HTML5 tags (https://www.php.net/manual/en/domdocument.loadhtml.php)I think this is ready for final review.
- 🇪🇸Spain gxleano Cáceres
Review steps
- Install
Drupal 11.x. - Enable Navigation module.
- Check menus and inspect the html for them.
See evidences:
Content Menu
Administration Menu
Help Menu
Everything works as expected ✅
Moving to RTBC.
- Install
- 🇨🇦Canada m4olivei Grimsby, ON
Addressed some duplication in the navigation titles for non-sighted users. See last comment and commit.
Ready for a re-review and accessibility review.
- 🇺🇸United States smustgrave
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.
- 🇨🇦Canada m4olivei Grimsby, ON
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.
- 🇨🇦Canada m4olivei Grimsby, ON
Merged the latest from the 11.x branch. Back to Needs review.
- 🇺🇸United States smustgrave
Can we get a CR for the template changes? In case some theme have overridden the templates
- 🇬🇧United Kingdom catch
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.
- Status changed to Needs review
about 2 months ago 3:06pm 14 April 2025 - 🇺🇸United States katannshaw Kansas
@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.
- 🇬🇧United Kingdom catch
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.
- 🇷🇸Serbia finnsky
Thanks for participating everyone!
I added one small change by removing the useless wrapper.
- In comment 50 I see screenshots from the old toolbar. While we need to test the new one. This may be my mistake. When I restarted Tugboat I did not enable our module in it. Sorry
- In the attached Google doc I see sentences like
Complementary renamed to: administrative sidebar complementary
while I see that it is pronounced in VoiceOver as
`Administrative sidebar, complementary` so nothing needs to be renamed?What is the desired level of granularity using the example of the navigational sidebar?
We don't know. Let's choose the best option
Other questions in the doc seem to me to be beyond the scope of the current task. At least we can definitely solve them later and not delay this last release blocker.
So I'm moving the task to review again and asking you to do the following:
Answer the question.
Does the current MR satisfy the minimum accessibility requirements? And if it doesn't, what should we do to achieve them? - 🇬🇧United Kingdom catch
Oh good spot in #52, that's definitely the old toolbar, completely missed that given shortcuts, user etc. also exist in the new navigation.
- 🇩🇪Germany rkoller Nürnberg, Germany
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
complementarywrapping landmark and the two navigation landmarks for the content and the footer section (11.x.mp4). With the MR you now have thecomplementarywrapping 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
mainlandmark 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
complementarylandmark that you get with theasideelement feels semantically wrong,complementaryis 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 thecomplementarylandmark 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.
- 🇬🇧United Kingdom catch
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.