department-of-veterans-affairs / va.gov-team

Public resources for building on and in support of VA.gov. Visit complete Knowledge Hub:
https://depo-platform-documentation.scrollhelp.site/index.html
282 stars 202 forks source link

sitewide 508-defect-2 [COGNITION, SCREENREADER]: Heading SHOULD read as a single item #18389

Closed joshkimux closed 3 years ago

joshkimux commented 3 years ago

508-defect-2

Feedback framework

Definition of done

  1. Review and acknowledge feedback.
  2. Fix and/or document decisions made.
  3. Accessibility specialist will close ticket after reviewing documented decisions / validating fix.

Point of Contact

VFS Point of Contact: Josh

User Story or Problem Statement

As a screen reader user, I expect to hear a heading read as the number of items it is, generally just a single item, so that I am clearly able to understand what the content and context is.

Details

@caw310 This seems to be a sitewide issue as it has cropped up in facilities as well in ticket 17515

When the heading is read by a screen reader it is announced as nine items, which is confusing because it really is only one item — the heading. The reason for this is that the content within the H2 is separated by a series of items — such as spans or other [insert React term] — are strung together in a heading. VoiceOver reads them as the number of items, which is confusing information for screen reader users.

Acceptance Criteria

Environment

Steps to Recreate

  1. Enter https://staging.va.gov/find-forms in browser
  2. Start screen reading device listed in Environment
  3. Enter search parameters
  4. Verify that the H2 for results reads that it is 9 items (as shown in screenshots below)

Proposed Solution (if known)

Current HTML output

<h2 class="vads-u-font-size--base vads-u-line-height--3 vads-u-font-family--sans vads-u-font-weight--normal vads-u-margin-y--1p5" data-forms-focus="true" tabindex="-1">
  Showing 
  <strong>1</strong> 
  – 
  <strong>1</strong> 
  of 
  <strong>1</strong> 
  results for "
  <strong>caregiver</strong>
  "
</h2>

Recommended

<h2 class="vads-u-font-size--base vads-u-line-height--3 vads-u-font-family--sans vads-u-font-weight--normal vads-u-margin-y--1p5" data-forms-focus="true" tabindex="-1">
  Showing 1 result for "caregiver"
</h2>

WCAG or Vendor Guidance (optional)

Screenshots or Trace Logs

Screen Shot 2021-01-12 at 10 21 39 AM
MarciMcGuireGCIO commented 3 years ago

Added sitewide label based on comment under Details.

zacharymorel commented 3 years ago

With the recommended solution above we will loose the Bold on the numbers and searched term. I don't know of any other way to solve this though.

jenniferlee-dsva commented 3 years ago

I believe this is the design pattern used in Facility Locator search and also RS search. Perhaps @ncksllvn might be able to provide input as he has worked on all of these products.

One question would be - would this make a difference if the bold was just strong text, rather than a heading class?

image

joshkimux commented 3 years ago

@jenniferlee-dsva @zacharymorel Great point! That would also address another ticket within this epic regarding the incorrect semantics of this string.

MarciMcGuireGCIO commented 3 years ago

Moving all outstanding Find a VA Form 508 issues into a single Epic to clean up backlog.

zacharymorel commented 3 years ago

@MarciMcGuireGCIO the Recommended solution seems to be the best solution for this issue. However, we will loose the boldness appearance of the search results numbers. Is this okay from the product ux side?

MarciMcGuireGCIO commented 3 years ago

@zacharymorel in this comment: https://github.com/department-of-veterans-affairs/va.gov-team/issues/18389#issuecomment-760462963 Jen mentions using strong text rather than a heading class. That seems like it would still give a heavier appearance to the text. As far as whether it's OK from a UX standpoint, @laurellawrence would need to review.

zacharymorel commented 3 years ago

Hmm. That's a great point @MarciMcGuireGCIO . @joshkimux going with your recommended solution would remove the bolder text, but it would solve the current issue.

@laurellawrence Do you have some time to glance at this?

laurellawrence commented 3 years ago

I took a look at the source code for both Find a Form and Find a VA Location. It appears that Find a VA Location heading is using a H2 ID and < b > and the other is an H2 CLASS using < strong >. @joshkimux, does this page have the same accessibility issues? If not, we should implement the HTML structure being used for Find a VA Location.

The difference between the two tags is that the bold tag is intended to draw attention to the text, while the strong tag also highlights the text semantically and indicates that this is an important word or section of text. The use of the strong tag may be our issue here.

Untitled-2

zacharymorel commented 3 years ago

This fix for this, https://github.com/department-of-veterans-affairs/va.gov-team/issues/18398, should include a fix for this issue!

zacharymorel commented 3 years ago

@1Copenut can you give this a review on https://staging.va.gov/find-forms/ since Josh has been out.

1Copenut commented 3 years ago

@zacharymorel I took a look briefly before close of business Friday afternoon. The heading is still announcing as 9 items in Safari + VO with <strong> tags inside the H2. LMK if there's a branch I should check out and re-test locally or if I overlooked something. Thank you.

zacharymorel commented 3 years ago

@1Copenut Looks like Dev and Staging env's are behind Production in deployments. So to test, can you check out prod? https://www.va.gov/find-forms/?q=health

1Copenut commented 3 years ago

@zacharymorel Hm, I'm seeing <strong> tags inside the H2 on the prod link you sent. LMK if you want to discuss in person. We can schedule a short call or VSP Product Support has office hours on Wednesday at 11:30am Eastern Time if that works better.

ncksllvn commented 3 years ago

Would using a span instead of a strong tag make any difference? Right now it might say <h2>Showing <strong>two</strong> results</h2> but what if it said <h2>Showing <span class="vads-u-font-weight--bold">two</span> results</h2>? That way the 508 issue is fixed but we can keep the bolded keywords.

1Copenut commented 3 years ago

Not sure, but it's worth a try. MacOS Safari + VoiceOver is particular about multiple tags inside tags like our H2 in this case. Let's give it a try and if it's still an issue, re-evaluate.

zacharymorel commented 3 years ago

@1Copenut I've tried both using spans and bold tags. Both are counted, on safari, the same behavior as seen here. They count the number of items. It's weird that only Safari does this out of the 3 browsers I am testing.

Any other suggestions to solve this issue?

zacharymorel commented 3 years ago

What did just work is changing the <h2> to a <p> tag. When on safari, VO focused on the P tag and did not count the number of items.

          <p
            className="vads-u-font-size--md vads-u-line-height--3 vads-u-font-family--sans vads-u-font-weight--normal vads-u-margin-y--1p5"
            data-forms-focus
          >
            Showing <span class="vads-u-font-weight--bold">{startLabel}</span>{' '}
            &ndash; <span class="vads-u-font-weight--bold">{lastLabel}</span> of{' '}
            <span class="vads-u-font-weight--bold">{results.length}</span>{' '}
            results for "<span class="vads-u-font-weight--bold">{query}</span>"
          </p>

I enlarged the text to have the same font size has an h2.

1Copenut commented 3 years ago

I hesitate to make the H2 a P tag because it removes that H2 for assistive tech users. They won't hear that text if they're navigating by headings only, which might be overly disruptive.

I had a thought that you could try. It's messy and I'm not sure it works, but...

<h2>
  <span role="text">
    <!-- Your regular markup with span/strong tags here -->
  </span>
</h2>

This used to throw an axe error, but there was talk they might be passing this to alleviate Safari + VO headaches around groups.

zacharymorel commented 3 years ago

@1Copenut So your suggestion passes axe Coconut but it does not pass the vets-website axe check from the linter... Screen Shot 2021-02-24 at 12 39 02 PM

Which points line 187 > Screen Shot 2021-02-24 at 12 40 05 PM

zacharymorel commented 3 years ago

CC @1Copenut I found the package used for our comment linting https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/master/docs/rules/aria-role.md

I'm guessing it's safe to say you already know about it! haha

zacharymorel commented 3 years ago

Something else worth of note, making the span hidden but adding a aria-label for the header allows it to be read correctly. Not sure if that flys with y'all though.

Screen Shot 2021-02-24 at 2 07 33 PM

1Copenut commented 3 years ago

@zacharymorel You might be onto something with this last experiment. Adding an aria-label to the H2 and hiding the span with aria-hidden. Let's give that a shot. Also, could you remove the role="region" from the H2? That role removes it as a heading for assistive tech, and shouldn't be needed with the explicit focus set and live region.

joshkimux commented 3 years ago

@zacharymorel , just tested with Chrome + JAWS and Chrome + NVDA.

JAWS completely skips the h2 because of the aria-hidden="true" on the span element. My best guess is that JAWS will not read out the h2s aria-label because it is interpreting the h2 as empty. I was only able to get JAWS to announce the h2 by removing the aria-hidden="true" from the span.

NVDA will not announce the heading at all upon search. It will only announce "Loading search results, loading progress bar half checked" and nothing else (although the focus does move to the h2). NVDA will still announce the h2 if the user manually navigates to it though. My best guess is that this is being caused by competing announcements cutting each other off which could be addressed by having an empty aria-live region specific to the h2 results string present on page load or by using a javascript to delay the announcement by 2 seconds.

It's odd that this works perfectly fine on Safari/VO 😕

zacharymorel commented 3 years ago

Oh thank you for reviewing @joshkimux! I guess I am kinda at loss on this one. Have any ideas?

joshkimux commented 3 years ago

@zacharymorel and @1Copenut , I did some digging into dequelabs and found this ticket that confirms that role="text" will no longer throw an error as part of the 4.2 release.

That being said, I think we should be ok to roll with Trevor's original role="text" solution to VO's parsing issue. That will prevent us from having to use aria-hidden="true" which creates a significantly bigger issue for JAWS users.

1Copenut commented 3 years ago

As long as it doesn't throw errors with the current CI, I'm good with that suggestion. Our build pipeline is currently on 4.1.1. Will be happy to bump to axe-core 4.2 when it's ready.

zacharymorel commented 3 years ago

@1Copenut I currently can't commit any changes due to lint checks using the 4.1.1 version. So in order to be able to commit changes with role="text" attribute, I'd need the axe-core package updated to use v4.2 before I work locally.

1Copenut commented 3 years ago

@zacharymorel Hm, it seems we're at an impasse here. The axe-core releases schedule shows 4.1.3 was released just 15 days ago, and I'm not seeing a roadmap for 4.2. A quick look through appears 4.1.3 allows role="text" on spans and a few other elements: https://github.com/dequelabs/axe-core/search?q=role%3D%22text%22

If this needs to be ice boxed until we can run a point version of axe-core that passes the ESLint violation, I'm okay with that. This is (unfortunately) not a new issue with Safari. My want is to push a fix as soon as possible but not any sooner if it's going to cause breaks or problems for developers.

@joshkimux do you concur?

zacharymorel commented 3 years ago

If 4.1.3 allow role="text", is it okay if we bump up the minor version?

MarciMcGuireGCIO commented 3 years ago

@zacharymorel who was your question for specifically? Thx!

zacharymorel commented 3 years ago

@1Copenut Sorry, should have tagged you with my Q

1Copenut commented 3 years ago

@zacharymorel I can't commit to working on a bump to a higher version of axe right now. If you want to bump it and make a PR, I can act as a reviewer. A minor bump by two digits like this should be low risk.

zacharymorel commented 3 years ago

Okay, I'll make a PR @1Copenut

zacharymorel commented 3 years ago

@1Copenut @joshkimux https://staging.va.gov/find-forms/?q=health is now live with the new role attribute. It's ready for validation!

joshkimux commented 3 years ago

@zacharymorel Just tested this and it's working great on JAWS and VO. Firefox/NVDA will still not announce the h2 on search, but I think that's a separate issue that is not related to this ticket in particular. Great work!!

MarciMcGuireGCIO commented 3 years ago

Thank you both for your diligence in resolving this issue! 🥳