Problem/Motivation

While working on #3507866: Enable the Navigation Top Bar when Navigation is enabled found that Navigation Top Bar has some accessibility issues disclosed by aXe Nightwatch tests.

This issues need to be solved before we can make the Top Bar stable.

From https://git.drupalcode.org/issue/drupal-3507866/-/jobs/4431574

  ️TEST FAILURE (1m 56s):  
   - 2 assertions failed; 1433 passed
   ✖ 1) Tests/a11yTest
   – Accessibility - Navigation Module - Claro page (1.698s)
   → ✖ NightwatchAssertError
   aXe rule: button-name - Buttons must have discernible text
	In element: .toolbar-button--icon--dots
   → ✖ NightwatchAssertError
   aXe rule: region - All page content should be contained by landmarks
	In element: .top-bar__context

Steps to reproduce

Expected results

Inspect the source and look for .top-bar__context. Notice that of all its parent elements, none of those elements are landmark elements (eg. <aside>, <nav>, <main>, etc.. This is what the Axe error is complaining about. We expect that there will be a landmark parent element.

Using a screen reader, like VoiceOver, open up the Rotor (VO + U) and review the Landmarks menu. We expect that there is a landmark listed identifying the top bar.

Also, for the More Actions button, we expect it to have text available for non-sighted users that a screen reader can pick up. eg. this button:

Screenshot of the more actions top bar menu

Actual results

There is no landmark parent to the .top-bar__context element, and no landmark is identified in the VoiceOver Rotor Landmarks menu.

There is no text available for the more actions button for non-sighted users.

Proposed resolution

Add a wrapping landmark element for the Top Bar.

Add some visually hidden text for non-sighted users to describe the More Actions menu in the top bar.

Remaining tasks

Testing

User interface changes

Accessibility improvements for the Top Bar.

CommentFileSizeAuthor
#10 more_actions.png170.68 KBm4olivei

Issue fork drupal-3508199

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

plopesc created an issue. See original summary.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Issue tags: +Navigation stable blocker

plopesc opened merge request !11257

plopesc opened merge request !11259

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented

Since this is not an easy bug to reproduce, created 2 branches:

  • 3508199-navigation-top-bar-axe-test-only: It contains only the code necessary to make the error visible in Nightwatch tests
  • 3508199-navigation-top-bar-axe: It should contain the code necessary to make the error visible in Nightwatch tests + the changes to fix it

Once tests in 3508199-navigation-top-bar-axe are green and approved, might be necessary to create a new branch that contains only the code necessary to fix the error. This branch will contain a diff between the 2 original ones, and that's the one that should be merged.

I know this is a bit tricky, but I have not been capable to figure out a better flow for this issue. If someone else have a better proposal, please feel free to show your concerns and ideas.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Priority: Major » Normal

finnsky made their first commit to this issue’s fork.

finnsky’s picture

finnsky commented
Status: Active » Needs review
Issue tags: +a11y

I added a role and text for the button. Nw tests passed.

But a11y needs to be checked here.

And in general, what is top_bar? What is its role? Header or navigation?

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

And in general, what is top_bar? What is its role? Header or navigation?

I would argue it's not navigation, there are informational bits within it as well. I want to suggest we mark it up as an <aside> with a title for screen readers. This follows what has been done with the sidebar.

Will push an update to show what I mean.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Issue summary: View changes
StatusFileSize
new more_actions.png170.68 KB
katannshaw’s picture

katannshaw commented

I've tested it using Apple VoiceOver, ARC Toolkit, and axe DevTools and everything looked good regarding the two reported issues:

  1. aXe rule: button-name - Buttons must have discernible text - In element: .toolbar-button--icon--dots
  2. aXe rule: region - All page content should be contained by landmarks - In element: .top-bar__context

I reviewed the code as well, and it all appeared to be set up properly from an accessibility perspective. The `` landmark is set up properly and didn't throw any errors with the automated checkers.

I did find one related issue when testing with the screen reader that needs to be resolved with this module or the main Navigation module itself.

  • The "Edit" button is read aloud as "Ed Edit" to screen readers
  • This is because there's an icon that's no needed for the button as it's purely decorative
  • I recommend removing the altogether
finnsky’s picture

finnsky commented

Thank you!

The "Edit" button is read aloud as "Ed Edit" to screen readers

It should gone with this MR landed:
https://www.drupal.org/project/drupal/issues/3483209

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

Thanks @katannshaw!

OK, so now that we've had some solid accessibility testing done here. I think we're ready to move forward. As @plopesc noted in #5, what we've done here is first expose the Nightwatch axe rule errors by turning the Navigation Top Bar always on in the 3508199-navigation-top-bar-axe-test-only branch. The Nightwatch axe rule errors are here, or:

TEST FAILURE (2m 53s):  
   - 2 assertions failed; 1433 passed
   ✖ 1) Tests/a11yTest
   – Accessibility - Navigation Module - Claro page (2.587s)
   → ✖ NightwatchAssertError
   aXe rule: button-name - Buttons must have discernible text
	In element: .toolbar-button--icon--dots
   → ✖ NightwatchAssertError
   aXe rule: region - All page content should be contained by landmarks
	In element: .top-bar__context

Note that there are other test failures in there as well. This issue is solely focused on the Nightwatch axe rules error.

We've then fixed the Nightwatch axe rule errors in the 3508199-navigation-top-bar-axe branch. We can see that the Nightwatch axe rule errors are fixed here!

The next step will be to take out the code that is exposing the navigation Top Bar here, b/c that is the work of #3507866: Enable the Navigation Top Bar when Navigation is enabled. Again, we're only interested in fixing the accessibility issue with the Navigation Top Bar here. I will next push this commit, and the 3508199-navigation-top-bar-axe branch, will be ready to merge!

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

Ready to go!

@plopesc and/or @finnsky could you review the MR, and my last comment?🤞 RTBC.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs review » Reviewed & tested by the community

Code looks good to me!

And it seems new bits fond are coming as part of other MRs.

Note for other reviewers: Instead of the 3 MRs approach suggested in #5, the lines of code that triggers the error are in MR 11257, which will be always green because the Top Bar is not going to be present since the line to force it have been reverted. Please check the previous pipeline to confirm that worked, or test it locally adding back those lines.
That MR is ready to be merged as it is.

  • nod_ committed 0b439c08 on 11.x 
    Issue #3508199 by plopesc, m4olivei, finnsky, katannshaw: Navigation Top...

nod_ closed merge request !11257

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed 0b439c0 and pushed to 11.x. Thanks!

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

Want to note that this patch should get backported to 10.5.x once #3484564: Define the 3 areas the Top Bar will provide is backported.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.