back the ID and aria-labelled-by with JS

Created on 5 June 2024, 11 months ago

Problem/Motivation

This is folow up issue from https://www.drupal.org/project/drupal/issues/3393400#comment-15627607 โœจ Evaluate adding Nightwatch tests Active

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

โœจ Feature request
Status

Postponed

Version

11.0 ๐Ÿ”ฅ

Component
Navigationย  โ†’

Last updated 4 days ago

No maintainer
Created by

๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • ๐Ÿ‡ท๐Ÿ‡ธ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> 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>
    

    Other resources explaining this reasoning:

    Hope it helps.

  • Merge request !11122Test with aria-label #3452724 โ†’ (Open) created by finnsky
  • Pipeline finished with Failed
    3 months ago
    Total: 698s
    #416299
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    One backend fix need to be applied here. We can go with here.

  • First commit to issue fork.
  • Pipeline finished with Canceled
    3 months ago
    Total: 242s
    #416847
  • Pipeline finished with Canceled
    3 months ago
    Total: 115s
    #416849
  • Pipeline finished with Failed
    3 months ago
    Total: 370s
    #416851
  • Pipeline finished with Failed
    3 months ago
    Total: 372s
    #417650
  • Pipeline finished with Failed
    3 months ago
    Total: 78s
    #417757
  • ๐Ÿ‡ช๐Ÿ‡ธSpain plopesc Valladolid
  • Pipeline finished with Success
    3 months ago
    #417770
  • ๐Ÿ‡ช๐Ÿ‡ธ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::loadHtml and 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

    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.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada m4olivei Grimsby, ON
  • ๐Ÿ‡จ๐Ÿ‡ฆ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.

  • Pipeline finished with Success
    about 2 months ago
    Total: 830s
    #437217
  • ๐Ÿ‡บ๐Ÿ‡ธ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.

  • Pipeline finished with Success
    about 2 months ago
    Total: 448s
    #441986
  • ๐Ÿ‡บ๐Ÿ‡ธ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.

  • Pipeline finished with Success
    26 days ago
    Total: 604s
    #461933
  • Status changed to Needs review 12 days ago
  • ๐Ÿ‡บ๐Ÿ‡ธ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.

  • Pipeline finished with Success
    7 days ago
    Total: 370s
    #477151
  • Pipeline finished with Failed
    7 days ago
    Total: 604s
    #477164
  • Pipeline finished with Success
    7 days ago
    Total: 1183s
    #477181
  • ๐Ÿ‡ท๐Ÿ‡ธ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.

  • Production build 0.71.5 2024