- Issue created by @finnsky
- Status changed to Active
9 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
9 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
9 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.