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

sitewide, CMS - 508-defect-2 [FOCUS MGMT]: Ensure focus moves when same page links are activated (IE11) #15244

Closed
2 tasks
jenstrickland opened this issue Oct 23, 2020 · 29 comments
Assignees
Labels
508-issue-focus-mgmt Focus Management - https://www.w3.org/TR/UNDERSTANDING-WCAG20/navigation-mechanisms-focus-order.html a11y-defect-2 High-severity accessibility issue that should be fixed in the next 1 - 2 sprints accessibility bug Something isn't working cms content management system drupal components Design system component issues Public Websites Global Unauthenticated Experience team for va.gov. Products include home page, content hubs... sitewide vsa Work associated with the Veteran-facing Services Applications contract

Comments

@jenstrickland
Copy link
Contributor

jenstrickland commented Oct 23, 2020

508-defect-2

Feedback framework

  • ❗️ Must for if the feedback must be applied
  • ⚠️ Should if the feedback is best practice
  • ✔️ Consider for suggestions/enhancements

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: Jennifer

User Story or Problem Statement

As a screen reader and/or keyboard user, I expect focus to move to the target location when I activate a same page link.

Details

The VA 508 Office found one issue during their review. It is specific to IE11. The focus for same page links must move to the target location. There is a known IE bug where this is broken.

Acceptance Criteria

  • Focus moves to the target id when a same page link is activated
  • Focus outline is not rendered if the target is not interactive

Environment

  • Operating System: Windows, latest
  • Browser: IE11
  • Screenreading device: JAWS, but the behavior is evident without the screen reader
  • Server destination: production

Steps to Recreate

  1. Enter https://www.va.gov/decision-reviews/ in browser
  2. Navigate to the On this page component
  3. Using only the keyboard, tab to the second bulleted link
  4. Activate the link
  5. Observe the new location displayed in the browser viewport
  6. Tab again
  7. Verify that focus remained in the On this page bulleted list of links

Solution (if known)

Please note: Another solution will be provided in a couple of hours. I believe @1Copenut has some JavaScript that is typically used.

Current code

<!-- On this page component -->
<h2 class="vads-u-margin-bottom--2 vads-u-font-size--lg" id="on-this-page">On this page</h2>
<ul>
          <li>
            <a href="#request-a-decision-review-or-appeal">
              Request a decision review or appeal
            </a>
          </li>
          <li>
            <a href="#manage-your-decision-reviews-and-appeals">
              Manage your decision reviews and appeals
            </a>
          </li>
          <li>
            <a href="#more-information-and-resources">
              More information and resources
            </a>
          </li>
        </ul>

<!-- target example -->
  <h2 id="request-a-decision-review-or-appeal" class="hub-page-link-list__title">
    Request a decision review or appeal
  </h2>

Recommendation

Two options to consider:

  1. https://stash.uconn.edu/users/jmr06005/repos/wordpress-josh/browse/wp-content/themes/twentysixteen/js/skip-link-focus-fix.js?at=578e32cb3718379cded8971a7d25cebf1d77d220

  2. From the Jim Thatcher link in WCAG or vendor guidance section (shown below): https://www.jimthatcher.com/news-070607.htm

<!-- On this page component -->
<h2 class="vads-u-margin-bottom--2 vads-u-font-size--lg" id="on-this-page">On this page</h2>
<ul>
          <li>
            <a href="#request-a-decision-review-or-appeal">
              Request a decision review or appeal
            </a>
          </li>
          <li>
            <a href="#manage-your-decision-reviews-and-appeals">
              Manage your decision reviews and appeals
            </a>
          </li>
          <li>
            <a href="#more-information-and-resources">
              More information and resources
            </a>
          </li>
        </ul>

<!-- target example -->
  <h2 class="hub-page-link-list__title">
    <span style="position:absolute;">
      <a name="request-a-decision-review-or-appeal" id="request-a-decision-review-or-appeal">&nbsp;</a>
    </span>
    Request a decision review or appeal
  </h2>

WCAG or Vendor Guidance (optional)

@jenstrickland jenstrickland added bug Something isn't working Public Websites Global Unauthenticated Experience team for va.gov. Products include home page, content hubs... accessibility sitewide cms content management system drupal components Design system component issues vsa Work associated with the Veteran-facing Services Applications contract a11y-defect-2 High-severity accessibility issue that should be fixed in the next 1 - 2 sprints 508-issue-focus-mgmt Focus Management - https://www.w3.org/TR/UNDERSTANDING-WCAG20/navigation-mechanisms-focus-order.html labels Oct 23, 2020
@joshkimux
Copy link
Contributor

I've just tested and confirmed that this issue still needs to be fixed. @brianalloyd could we prioritize this asap? This was flagged by the VA 508 office half a year ago 🙇

@brianalloyd
Copy link
Contributor

@joshkimux we will prioritize and place in this sprint for action. Thanks for following up.

@ncksllvn
Copy link
Contributor

Summary of what I am gathering from this issue...

Our "On this page" sections link to heading tags on the same page using an ID on the heading tag, like <a href="#heading-tag-id">Jump to a heading tag</a>. Then later on the page..<h3 id="heading-tag-id">Heading</h3>.

This issue is proposing that instead of the heading tag having the ID, we have an <a> tag inside it that contains the ID. <h3><a id="heading-tag-id" name="heading-tag-id">&nbsp;</a>Heading</h3>. I suppose this fixes an issue w/IE.

These heading IDs are generated during the front-end build. I think we would need to make this change here.

@ncksllvn
Copy link
Contributor

ncksllvn commented Apr 1, 2021

This would be sort of a medium risk change btw. Think I would rate this a 5.

@kelsonic
Copy link
Contributor

@joshkimux is this still a ticket we should do? e.g. putting anchor tags within our heading tags for TOC same-page linking

@joshkimux
Copy link
Contributor

Yes, but I recommend following the articles linked closely to ensure it's an anchor tag without an href

@Rleahy22
Copy link

Fixed. I have a pr up now with an updated screen recording, and it should create a branch deployment.

@Rleahy22
Copy link

For validation, this should not affect use on any browser other than IE11. Changes should go out with the Tuesday (9/14) deployment.

@timwright12
Copy link
Contributor

timwright12 commented Sep 13, 2021

@joshkimux @Rleahy22 Just wanted to poke my head in here as I saw this PR get merged. It looks to me like we're violating two WCAG rules with this fix:

Link purpose fill trigger an issue on an empty link and focus visible, while it won't trigger an automated error it will cause visible focus to be lost on the link.

Update:
This is the axe violation on staging for the empty link:
Screen Shot 2021-09-13 at 5 01 18 PM

@Rleahy22
Copy link

Did those issues not exist prior to this change as well? What would we like these links to do in that case?

@timwright12
Copy link
Contributor

@Rleahy22 these would be new issues that care coming up with the PR. In dealing with the IE11 issue, it's standard to add some JS to fix it. This is how WordPress deals with it: Automattic/_s#1206 -- basically a small amount of JS that binds to the click of a skip link, adds tabindex="-1" to the target element, then sets focus to that element. There's also a blur event that removes the tabindex="-1" so it doesn't hang around afterwards.

The PR isn't deployed to prod yet and we run a11y tests each morning, so this should throw and error in #-daily-accessibility-scan tomorrow morning. Ideally we should try to fix it up before that.

@joshkimux
Copy link
Contributor

Thanks for linking to how WordPress handles it! 🙏

@Rleahy22
Copy link

Rleahy22 commented Sep 14, 2021

@Rleahy22 these would be new issues that care coming up with the PR. In dealing with the IE11 issue, it's standard to add some JS to fix it. This is how WordPress deals with it: Automattic/_s#1206 -- basically a small amount of JS that binds to the click of a skip link, adds tabindex="-1" to the target element, then sets focus to that element. There's also a blur event that removes the tabindex="-1" so it doesn't hang around afterwards.

The PR isn't deployed to prod yet and we run a11y tests each morning, so this should throw and error in #-daily-accessibility-scan tomorrow morning. Ideally we should try to fix it up before that.

@timwright12 I'm not sure I see the code you're referring to. That issue is about removing this code: https://github.com/Automattic/_s/pull/1424/files which they did remove, but it does not include all the functionality outlined above. Additionally the issue with our site is that the skip link takes the user to an h2 which is not a focusable element, so I don't think this code would even solve the issue. Do you know where I can see the code WordPress uses?

@Rleahy22
Copy link

Rleahy22 commented Sep 15, 2021

@timwright12 @joshkimux since we use h2 tags for the in page link anchors we can't use .focus() on them so I have 2 proposals for how we can use a tags.

  1. Wrap the h2 in an a tag with link styles reset. This makes it so the link has text unlike in the initial solution which used empty links. This solves our issue, but does make the Headings clickable, and the resulting scroll may catch the user off guard:

Screen Shot 2021-09-15 at 3 39 35 PM

  1. Use a link icon to denote the anchor:

Screen Shot 2021-09-15 at 3 42 04 PM

Thoughts?

@joshkimux
Copy link
Contributor

@Rleahy22 By any chance, did you test using &nbsp; instead of tabindex="-1" as listed in the original ticket?

<h2 class="hub-page-link-list__title">
    <span style="position:absolute;">
      <a name="request-a-decision-review-or-appeal" id="request-a-decision-review-or-appeal">&nbsp;</a>
    </span>
    Request a decision review or appeal
  </h2>

It's solution A in the original article cited by Jen as opposed to solution B which is an empty link

@Rleahy22
Copy link

Ah I did, but it affected display. I could make the same style changes as I did in solution 1 though. Thanks @joshkimux

@Rleahy22
Copy link

@joshkimux it looks like the non breaking space doesn't meet our requirements either. I get this error with that solution as well.
Screen Shot 2021-09-22 at 4 10 27 PM

@joshkimux
Copy link
Contributor

Let me test this separately! I'll get back to you before the end of the week 😄

@joshkimux
Copy link
Contributor

joshkimux commented Sep 23, 2021

@Rleahy22 Alright, getting back to you now! I should have read the earlier comments in more detail 🙇

since we use h2 tags for the in page link anchors we can't use .focus()

.focus() will work for h2 provided you set tabindex="-1" as an attribute. Here's a codepen demonstrating the steps to do so!

The solution Tim suggested should work! The codepen I attached explains how to set focus to a heading e.g. h2 and a content block e.g. main 😄

@Rleahy22
Copy link

Alright, third times the charm! @joshkimux this should go out with the deployment today.

@brianalloyd
Copy link
Contributor

@joshkimux could validate the work here and let us know what additional work if any is needed. Appreciate @Rleahy22 for taking the lead and moving this forward, awesome work Ryan.

@joshkimux
Copy link
Contributor

Great work @Rleahy22 as always 🥳

VA.Decision.Reviews.And.Appeals._.Veterans.Affairs.-.Internet.Explorer.2021-09-28.11-52-50.mp4

Last nitpick before we close this one out-- would it be possible for us to remove the focus halo from the heading? Since it is not an interactive element, previous VA guidance has recommended not to have the focus halo around it even if we send focus to it.

@Rleahy22
Copy link

@joshkimux good catch. The fix for that should go out in today's deploy

@brianalloyd
Copy link
Contributor

@joshkimux this should be ready to validate and close. Thanks to @Rleahy22 for the solid work on resolving this issue.

@joshkimux
Copy link
Contributor

Validated. @Rleahy22 is on a roll 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
508-issue-focus-mgmt Focus Management - https://www.w3.org/TR/UNDERSTANDING-WCAG20/navigation-mechanisms-focus-order.html a11y-defect-2 High-severity accessibility issue that should be fixed in the next 1 - 2 sprints accessibility bug Something isn't working cms content management system drupal components Design system component issues Public Websites Global Unauthenticated Experience team for va.gov. Products include home page, content hubs... sitewide vsa Work associated with the Veteran-facing Services Applications contract
Projects
None yet
Development

No branches or pull requests

7 participants