Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Describe and Waypoint Main Menu via Signpost Heading #366

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jkva
Copy link
Contributor

@jkva jkva commented Nov 26, 2021

This PR addresses the following item from PAC's "ARIA-AT App Screen Reader Accessibility Observations" document:

The navigation bar items ("Test Reports", " Test Queue", etc.) are not marked up inside an unordered list to convey their grouped nature to screen reader users. They are also not preceded by a heading, either visible or off-screen/screen-reader-only.

The "The navigation bar items ("Test Reports", " Test Queue", etc.) are not marked up inside an unordered list to convey their grouped nature to screen reader users" has previously been addressed.


Effective changes:

  • A visually hidden h2 element containing "Main Menu" was added to precede the main menu nav; and
  • the main menu nav is described by this h2.

@jkva jkva requested a review from jscholes November 26, 2021 09:48
@jkva jkva changed the title Describe and Waypoint Main Menu via Signpost Heading. Describe and Waypoint Main Menu via Signpost Heading Nov 26, 2021
Copy link
Contributor

@jscholes jscholes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkva Looks good. A few things:

  1. Can we change "Main Menu" to "Main Navigation"?
  2. Exchange aria-labelledby on the nav for aria-label="Main", to avoid duplicate "navigation" announcements.
  3. Move the <h2> inside the nav.
  4. If not already done, add aria-labelledby to the actual list wrapping the nav links, with aria-labelledby referencing the new heading.

@jkva jkva requested a review from jscholes November 29, 2021 18:45
@isaacdurazo
Copy link
Member

@jkva I noticed a couple of things here about the UI:

  1. The Home link now reads "HOME" instead of "ARIA-AT"
  2. When any link in the main nav is clicked, the "Skip to main content" button gets displayed. Is this intentional?

@jkva
Copy link
Contributor Author

jkva commented Feb 7, 2022

@jkva I noticed a couple of things here about the UI:

  1. The Home link now reads "HOME" instead of "ARIA-AT"
  2. When any link in the main nav is clicked, the "Skip to main content" button gets displayed. Is this intentional?

Hmm, neither of these things are proper. I've locally rebased this PR on #367, which overlaps with this (#366) PR in the sense that it also addresses navigation and styling.

Once #367 gets merged I'll rebase #366 on main and force push to this branch, that should put things in their proper place.

cc @alflennik @jscholes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants