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

Sub-Task: Accessibility Audit for SideNav #2553

Closed
Karak888 opened this issue Oct 18, 2019 · 22 comments
Closed

Sub-Task: Accessibility Audit for SideNav #2553

Karak888 opened this issue Oct 18, 2019 · 22 comments
Assignees
Labels
accessibility Public Websites Global Unauthenticated Experience team for va.gov. Products include home page, content hubs... vsa Work associated with the Veteran-facing Services Applications contract

Comments

@Karak888
Copy link
Contributor

As part of validation for SideNAV and menu for Pittsburgh, need to ensure accessbility improvements are validated and accounted for prior to deploying to Production.

@1Copenut would you kindly let me know or include any specific tasks that are needed to fulfill this validation? This relates to #2278.

Page: https://staging.va.gov/pittsburgh-health-care/
Design: https://github.com/department-of-veterans-affairs/va.gov-team/blob/master/products/global/header-footer-and-navigation/page-nav/design/VA-gov-on-page-nav-MASTER.pdf

@Karak888 Karak888 added Public Websites Global Unauthenticated Experience team for va.gov. Products include home page, content hubs... accessibility labels Oct 18, 2019
@kelsonic kelsonic added the vsa Work associated with the Veteran-facing Services Applications contract label Oct 18, 2019
@kelsonic kelsonic changed the title Sub-Task: Accessibility Improvements for SideNAV Menu for Pittsburgh Sub-Task: Accessibility Improvements for SideNav Oct 21, 2019
@kelsonic kelsonic changed the title Sub-Task: Accessibility Improvements for SideNav Sub-Task: Accessibility Audit for SideNav Oct 21, 2019
@kelsonic
Copy link
Contributor

image

It doesn't look like there's any SideNav-specific accessibility errors on this page. @1Copenut could you confirm if that's true on your side?

@1Copenut
Copy link
Contributor

@kelsonic I'm gathering notes tonight for this and will add them as a single comment here ASAP.

@1Copenut
Copy link
Contributor

@Karak888 && @kelsonic
I'm going to hand this to Jennifer Strickland ( @jenstrickland ) our VSA 508 specialist. We have discussed the issue and reviewed the live code.

@1Copenut 1Copenut assigned jenstrickland and unassigned 1Copenut Oct 22, 2019
@kelsonic
Copy link
Contributor

Hey @jenstrickland, do you have any update on this? :)

@jenstrickland
Copy link
Contributor

@kelsonic - Sorry for the delay, had some hardware issues to sort out. Just got the right laptop back for this, and codebase set back up. It will have my attention today and tomorrow.

@kelsonic
Copy link
Contributor

No problem! Thanks! 🙂

@jenstrickland
Copy link
Contributor

Issue Description: [aria-*] attributes do not have valid values (I see aria-controls refers to va-detailpage-sidebar but do not see that on the page. In addition, aria-controls lacks sufficient support across screen readers.)

Element source:
<button type="button" class="va-btn-sidebarnav-trigger vads-u-width--full" aria-controls="va-detailpage-sidebar">

Impact: Critical user impact

Reference:

====================================

Issue Description: Heading levels should only increase by one

Element location:
.medium-screen\:vads-u-margin-bottom--4.vads-u-margin-bottom--3.region-list:nth-child(2) > .vads-u-margin-right--2.region-grid > .medium-screen\:vads-u-margin-bottom--0p5.vads-u-margin-top--0

Element source:
<h4 class=" vads-u-margin-top--0 medium-screen:vads-u-margin-bottom--0p5"><a href="/pittsburgh-health-care/stories/va-nurse-gives-a-family-the-chance-to-say-goodbye">VA nurse gives a family the chance to say goodbye</a></h4>

Impact: Moderate

Reference: https://dequeuniversity.com/rules/axe/3.3/heading-order

====================================

Issue Description: All page content must be contained by landmarks

Remediation guidance:
Some page content is not contained by landmarks

Related nodes:

  • #youre-on-a-preview-of-the-new-
  • .usa-alert-text > p:nth-child(1)
  • .usa-alert-text > p:nth-child(2)
  • p:nth-child(3) > strong
  • #\34 5d9ee57ffa3839deeb54035546f2e86

Impact: Moderate

Reference: https://dequeuniversity.com/rules/axe/3.3/region

===========

If you're up for it, the Accessibility Best Practices recommends running code through axe (I also suggest Lighthouse). Happy to hop on a call to do it together if you are unfamiliar with these tools.

@Karak888 - There are things about the design that have accessibility considerations. Is there an opportunity to review the design, as well?

@kelsonic
Copy link
Contributor

@jenstrickland Thanks for identifying all the potential issues! I have a few questions as I think some of the issues you've identified may be out-of-scope for this ticket.

Issue 1:

Issue Description: [aria-*] attributes do not have valid values (I see aria-controls refers to va-detailpage-sidebar but do not see that on the page. In addition, aria-controls lacks sufficient support across screen readers.)

Element source:
<button type="button" class="va-btn-sidebarnav-trigger vads-u-width--full" aria-controls="va-detailpage-sidebar">

Impact: Critical user impact

This DOM node doesn't actually show anything here as it must be legacy code. What page did you see that on as when I ran axe as seen above I didn't get that axe error 🤔
image

==================================

Issue 2:

Issue Description: Heading levels should only increase by one

Element location:
.medium-screen\:vads-u-margin-bottom--4.vads-u-margin-bottom--3.region-list:nth-child(2) > .vads-u-margin-right--2.region-grid > .medium-screen\:vads-u-margin-bottom--0p5.vads-u-margin-top--0

Element source:
<h4 class=" vads-u-margin-top--0 medium-screen:vads-u-margin-bottom--0p5"><a href="/pittsburgh-health-care/stories/va-nurse-gives-a-family-the-chance-to-say-goodbye">VA nurse gives a family the chance to say goodbye</a></h4>

Impact: Moderate

Great find! However, this does not have to do with the SideNav code so we should create a separate ticket for this.

==================================

Issue 3:

Issue Description: All page content must be contained by landmarks

Remediation guidance:
Some page content is not contained by landmarks

Related nodes:

#youre-on-a-preview-of-the-new- .usa-alert-text > p:nth-child(1) .usa-alert-text > p:nth-child(2) p:nth-child(3) > strong #\34 5d9ee57ffa3839deeb54035546f2e86

Impact: Moderate

Another great find! However, this also does not have to do with the SideNav code so we should create a separate ticket for this.

==================================

Both issue 2 and 3 are out-of-scope for the SideNav and should instead have separate tickets created for them. Issue 1, however, is interesting as I didn't touch that code nor does the code appear to be visible in any way to the user 🤔 Not sure about that one...

@Karak888
Copy link
Contributor Author

@kelsonic please do not work on this at this time. @jenstrickland this is on staging but is on hold now and Kelson is on CMS superteam for awhile. We can table this for later time!

@jenstrickland
Copy link
Contributor

@kelsonic @Karak888 - for when it is worked on in the future, the first item came out of a Lighthouse audit.

@Karak888
Copy link
Contributor Author

Thanks @jenstrickland !

@1Copenut
Copy link
Contributor

aria-controls lacks sufficient support across screen readers.

In testing, I haven't seen aria-controls cause any issues to assistive tech that doesn't support it. I actually prefer to use it to move to the controlled element when I encounter buttons with aria-expanded attributes, assuming they are handlers for accordion content.

Just my 2¢ on the subject. @jenstrickland happy to discuss further!

@Karak888
Copy link
Contributor Author

Karak888 commented Nov 6, 2019

@kelsonic when you are back we'll introduce you to @rtwell and we'll start from there to address any design/ compliance issues. Thanks!

CC: @jenniferlee-dsva

@jenstrickland
Copy link
Contributor

jenstrickland commented Nov 12, 2019

Looped in @andaleliz for design modifications to address accessibility issues, after discussion with @kelsonic CC: @Karak888 Also, related #2278

@Karak888
Copy link
Contributor Author

@jenstrickland totally understand your concerns on pittsburgh staging but we can't just pull Liz to address pittsburgh as she has other work she is committed to for this sprint. I ask that you please always make sure you go through @jenniferlee-dsva first before assigining work to be done. We will incorporate your suggestions, and I recognize the importance of what you want done, but we have to make sure that it is vetted and planned for with Jen Lee first. Thank you!

@kelsonic
Copy link
Contributor

I'm not sure if this ticket is ready for dev work yet. Going to postpone working on this ticket until it has clear acceptance criteria for the frontend :)

@kelsonic
Copy link
Contributor

Hey @rtwell! @jenstrickland and I had a conversation yesterday and 2 questions were brought up:

1) Could we find a different solution for duplicating the selected nav item label (e.g. Locations, Locations)?
2) Could we reuse already existing components for the side nav?

image

@rtwell
Copy link
Contributor

rtwell commented Nov 13, 2019

thanks for noting, @kelsonic
my answers are

  1. we'll see how this approach lands with users (I believe there will be accessibility-specific usability tests included in the research).
  2. no, this is a new design.

@kelsonic
Copy link
Contributor

Great, thanks @rtwell! So if I understand correctly, I think we can close this ticket for now until more specific accessibility issues come up.

@Karak888
Copy link
Contributor Author

@kelsonic @rtwell I will just move this to icebox until ready to pull back in for review. Thanks!!

@jenstrickland
Copy link
Contributor

jenstrickland commented Nov 13, 2019

@Karak888 @kelsonic @rtwell

I compiled into an epic all accessibility issues in detail that were noted in the accessibility audit for the Pittsburgh sidenav. This morning I coordinated with @1Copenut about the process for accessibility audits, and discovered I didn't have the information on how to do/document them. https://app.zenhub.com/workspaces/vft-59c95ae5fda7577a9b3184f8/issues/department-of-veterans-affairs/va.gov-team/3411

@jenstrickland
Copy link
Contributor

jenstrickland commented Nov 19, 2019

Sidenav accessibility audit complete, and issues collected in the epic referenced in the above comment from six days ago. https://app.zenhub.com/workspaces/vft-59c95ae5fda7577a9b3184f8/issues/department-of-veterans-affairs/va.gov-team/3411

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Public Websites Global Unauthenticated Experience team for va.gov. Products include home page, content hubs... vsa Work associated with the Veteran-facing Services Applications contract
Projects
None yet
Development

No branches or pull requests

5 participants