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 203 forks source link

[SCREENREADER]: VAOS - Continue button MUST give meaningful feedback during transitions #10267

Closed 1Copenut closed 4 years ago

1Copenut commented 4 years ago

508-defect-2

Feedback framework

Description

Our VA 508 office contact noticed a situation on the /choose-eye-care page where after pressing the Continue button, our gray spinner button announced itself as "unlabeled zero button unavailable" to JAWS + IE11. I've attempted to re-create it by clearing cache and throttling to slow 3G, but wasn't able to reproduce it. Looking at our code for the platform LoadingButton.jsx I have a proposed code solution. Screen shots attached below.

Point of Contact

VFS Point of Contact: Trevor

Acceptance Criteria

Environment

Steps to Recreate

Possible Fixes (optional)

I believe we can add an SR-only span next to the spinner icon, make the icon invisible to screen readers, and wrap them in a React.Fragment to maintain the current API and introduce a new prop for loading text safely:

// Platform LoadingButton.jsx

export default function LoadingButton({
  children,
  disabled,
  isLoading,
+ loadingText,
  onClick,
  ...props
}) {
  const contents = isLoading ? (
+ <>
      <i
+       aria-hidden="true"
        className="fa fa-spinner fa-spin"
+       role="presentation"
      />
+     <span className="sr-only">{loadingText}</span>
 + </>
  ) : (
    children
  );
  return (
    <button
      className="usa-button"
      disabled={isLoading || disabled}
      onClick={onClick}
      {...props}
    >
      {contents}
    </button>
  );
}

WCAG or Vendor Guidance (optional)

Screenshots or Trace Logs

Picture1

1Copenut commented 4 years ago

@lenaecb && @jbalboni I want to reopen this issue. The Continue button text is reading out after the new page loads in IE11 + JAWS. I took a look at the code in LoadingButton.jsx, specifically the change on line 11 to poll for the loadingText prop instead of isLoading. I think if we keep other items the same and change that back to isLoading, it should read out when we expect, instead of on the next screen.

I'll retest quickly on localhost and Staging when you're ready.

jbalboni commented 4 years ago

@1Copenut That was made right after this PR. The current master version checks isLoading: https://github.com/department-of-veterans-affairs/vets-website/blob/master/src/platform/site-wide/loading-button/LoadingButton.jsx#L11

1Copenut commented 4 years ago

@jbalboni Hm, I'll have to look into it a bit more then. This issue is a tough one--I wasn't able to re-create it myself, but our 508 contact was able to re-create it several times inside the VA network. I'll log in and see if I get the same outcome, and make a new suggestion.

Thank you for the confirmation.

jbalboni commented 4 years ago

@1Copenut I think the issue is that we're not resetting the loading state for the Continue button until after the page change starts. So JAWS might see that text there after the url changes, and queue it up to be read?

I'll make the change in a PR to flip the order there and we can see if that works or breaks other things.

1Copenut commented 4 years ago

@jbalboni That sounds like a great plan! I think you're right on point there; IE11 + JAWS tends to read out first rendered state instead of final reconciled state in cases like this. I'll wait for your PR and re-test IE in staging after a quick spot check. Thank you.

jbalboni commented 4 years ago

@1Copenut Up now: https://github.com/department-of-veterans-affairs/vets-website/pull/13471

jbalboni commented 4 years ago

@1Copenut This should be ready on staging whenever you get a chance.

1Copenut commented 4 years ago

@jbalboni I just verified this in staging. I think we're solid now, and I'll ask the VA 508 office to confirm on their network. Thank you.