Address all pa11y violations in cypress testing.

[JasonLin0991]

OY2-16586

Summary

Thoughts on pa11y options if a fix is not found.

• ignore

  • This option in pa11y, in my opinion, should be used sparingly. Ignoring rules will ignore all violations, current and future, of this type and will run into issues with legitimate violations being overlooked.

  • For example <Fieldsets> component will show aria-allowed-attr violations when aria-required is present and using its implicit group role so to let the user know, in VO, this is a required group. If we ignore this type of violation then in the case where <Fieldsets> contained radios, but wasn’t assigned the radiogroup role ignoring this violation type would overlook an easy fix of assigning the radiogroup role. Not only did this fix the violation, but VO is more descriptive when describing this group.

  • So how do we deal with the above example when we cannot use a role that allows the aria-required attribute? This is where we would use the threshold option. See threshold bullet point for more details.

  • The only case I used the ignore option was for the <dl>, <dt> and <dd> violations with structure in the DoubleColumnGrid component. See the Issue bullet point below for more details.

• hideElement

  • Hide elements from pa11y tests when the element is a part of a third party component, where child elements are NOT passed in and the component is not extensively altered. The <StepIndicator> and <StepIndicatorStep> are good examples of components where we can ignore elements. See Issue below for more details.

  • This assumes that the third library component has been accessibility tested. We are using React USWDS components, which I believe some components are being tested for accessibility.

  • We should test the components ourselves anyways just to make sure there are no actual violations before hiding.

• threshold

  • Threshold should be used in cases where we should not ignore a violation and should not hide the element.
    Again with <Fieldset> example, see example in Ignore bullet point, when the element contains other elements that are not radios. For example date selection/input, there is no role that accommodates a complicated widget like this and the implicit group role does not permit aria-required attribute. In this case we could use Ignore, but the better option would be to add threshold counts to bypass the violation. This ensures that we address each new violation and either find a fix or add to the threshold.

My conclusion on these pa11y options is that they should all be used sparingly and each violation needs to be carefully investigated before excluding them from tests. From my investigation some violations do not cause issues in VO, but fixing them did improve VO and that is the goal.

---

• Issue: definition-list & dlitem - DoubleColumnGrid

  • <dl> elements must only directly contain properly-ordered <dt> and <dd> groups, <script>, <template> or <div> elements (https://dequeuniversity.com/rules/axe/4.3/definition-list?application=axeAPI)

  • <dt> and <dd> elements must be contained by a <dl> (https://dequeuniversity.com/rules/axe/4.3/dlitem?application=axeAPI)

  • After investigating these two violations, the cause is the DoubleColumnGrid component that has <dt> and <dd> nested in three <div> layers within the <dl> element. According to the docs on <dl>, you can nest <dt> and <dd> elements within one <div> layer, but nothing suggests nesting in two or more layers is acceptable or not. The second violation I suspect was due to the deep nesting of the <dt> and <dd> elements in <divs>. Testing VO and checking the accessibility tree on chrome shows no issues with accessibility.

  • Two possible solutions were refactoring the DoubleColumnGrid to address this issue or using pa11y options to ignore/hide/threshold the violation. I chose the later.

  • Ignore, hideElement, or threshold the violation? My choice was to ignore the violation for the specific test, because the DoubleColumnGrid component is being passed child elements, which if you hid the component, would ignore all potentially valid violations from child elements. Threshold would work, but adding more children elements to this component would require increasing the threshold for each additional child. Since we know the error is a structure issue and adding children doesn’t alter the structure pattern and accessibility is not hindered, we can assume the violations should be the same type.

  • We will need to keep in mind to revisit this violation in case any <dl> element is added to these tests that are not a part of the DoubleColumnGrid component.

  • Github issue going over the spec <dl> change to allow <div> wrapping <dt> & <dd>. Reconsider FAQ: <di>, <div> or <li> wrapping <dt> and <dd> whatwg/html#1937

  • <dl> element spec https://html.spec.whatwg.org/multipage/grouping-content.html#the-dl-element

• Issue: aria-allowed-attr & color-contrast - DynamicStepIndicator

  • Elements must only use allowed ARIA attributes (https://dequeuniversity.com/rules/axe/4.3/aria-allowed-attr?application=axeAPI)
    Context: <div class="usa-step-indicator" data-testid="step-indicator" aria-label="progress"><ol class="usa-step-indicator__...</div>
    Selector concerned: "#main-content > div:nth-child(1) > div"

  • Elements must have sufficient color contrast (https://dequeuniversity.com/rules/axe/4.3/color-contrast?application=axeAPI)
    Context: <span class="usa-step-indicator__segment-label">Submission type&nbsp;</span>
    Selector concerned: "#main-content > div:nth-child(1) > div > ol > li:nth-child(1) > span,#main-content > div:nth-child(1) > div > ol > li:nth-child(2) > span,#main-content > div:nth-child(1) > div > ol > li:nth-child(3) > span,#main-content > div:nth-child(1) > div > ol > li:nth-child(4) > span,#main-content > div:nth-child(1) > div > ol > li:nth-child(5) > span"

  • DynamicStepIndicator uses React USWDS StepIndicator and StepIndicatorStep components with minimal customization. I suspect the Aria attribute error is due to the element not having a role which allows the aria-label attribute. Possibly make a PR on React USWDS to add a role? The color contrast error is the <span> element containing the step indicator step text is more complicated and will probably need more investigation. From VO accessibility testing, neither of these violations caused issues.

  • The decision was to hide both from testing. This choice was made because the DynamicStepIndicator component has minimal customization, no child elements passed in and VO testing worked fine. I recommended someone on React USWDS to look into these violations.

• Issue: aria-allowed-attr - FileUpload

  • Elements must only use allowed ARIA attributes (https://dequeuniversity.com/rules/axe/4.3/aria-allowed-attr?application=axeAPI)
    Context: <span id="documents-hint" aria-labelledby="documents" class="FileUpload_fileInputHint__3IFBV"><a class="usa-link usa-link--ex...</span>
    Selector concerned: "#documents-hint"

  • This error was due to <span> not having a role that permits the aria-labelledby attribute. Adding a role of group to the <span> fixed the violation.

• Issue: aria-allowed-attr - FieldSet

  • Elements must only use allowed ARIA attributes (https://dequeuniversity.com/rules/axe/4.3/aria-allowed-attr?application=axeAPI)
    Context: <fieldset data-testid="fieldset" class="usa-fieldset" aria-required="true"><legend class="usa-legend">Choo...</fieldset>
    Selector concerned: "#SubmissionTypeForm > fieldset > div:nth-child(4) > fieldset,#SubmissionTypeForm > fieldset > div:nth-child(5) > div"

  • The cause of the violation is that the implicit aria-role role of group for <Fieldset> does not support aria-required. The <Fieldset> React USWDS component is composed of a <fieldset> element. The aria-required prop is passed to this element.

  • The solution I chose was to give <Fieldset> containing <FieldRadio> components the role of radiogroup, this takes care of the violation in these elements and VO is more descriptive as a result. The leftover violations will be addressed using thresholds instead of hideElements and ignore options in pa11y.

  • The reason for using the threshold option is because using ignore for violation type is too broad and would overlook actual violations and fixes like above with radios. Hiding the <Fieldset> element is would hide any children element passed into it of which there are many.

  • Docs on

    . https://developer.mozilla.org/en-US/docs/Web/HTML/Element/fieldset

  • Docs on aria role group. https://w3c.github.io/aria/#group

• Issue: aria-allowed-attr - FileListItem

  • ARIA attributes must conform to valid values (https://dequeuniversity.com/rules/axe/4.3/aria-valid-attr-value?application=axeAPI)
    Context: <div role="progressbar" aria-valuetext="" aria-label="Status of file trussel-guide.pdf"><img id="f98fbd53-e314-4ced-8b7...</div>
    Selector concerned: "#f98fbd53-e314-4ced-8b70-fd161f42c517 > div:nth-child(1) > div"

  • The aria-valuetext is using statusValue prop as it’s value. When uploads are finished the statusValue is an empty string, which causes the invalid values issue. Adding an additional status of ‘upload complete’ to be passed to statusValue fixed the violation.

• Issue: aria-allowed-attr - FieldTextarea

  • Elements must only use allowed ARIA attributes (https://dequeuniversity.com/rules/axe/4.3/aria-allowed-attr?application=axeAPI)
    Context: <div aria-labelledby="submissionDescription" class="usa-hint margin-top-1"><p id="submissionDescriptionHel...</div>
    Selector concerned: "#SubmissionTypeForm > fieldset > div:nth-child(5) > div"

  • This violation is caused by the <div>, containing the hint prop, needing a valid role to be permitted to use aria-labelledby.

  • The solution was to give the <div> a role that is allowed to use the aria-labelledby. The note role was the best match for our use case.

---

Test cases covered

dashboard.spec.ts
contacts.spec.ts
contractDetails.spec.ts
documents.spec.ts
newSubmissionSpec.ts
rateDetails.spec.ts

QA guidance

Please test VO on the changes done to some elements.

• <Fieldsets> that contain <FieldRadio>.
• Hints above uploading and hint above submission description text area.
• area-valuetext for listed uploaded items when completed.

[rswerve]

This write-up is 💯. Really great to spell everything out. I'm going to dig into the details in a bit, but a couple things I wanted to note:

The color contrast error is the <span> element containing the step indicator step text is  
more complicated and will probably need more investigation. From VO accessibility testing,  
neither of these violations caused issues.

For contrast violations, the violation pretty much is the issue. It's considering people who are sighted, but may be color-blind, or have other issues with their vision. As much as possible, we should try to fix these (if that means making a React-USDWS PR, maybe we do that...

When uploads are finished the statusValue is an empty string, which causes  
the invalid values issue. Adding an additional status of ‘upload complete’ to be passed to  
statusValue fixed the violation.

One thing to test here is whether adding the "upload complete" status causes a screen reader to announce each upload as it completes. Because there's no limit to how many docs a user can attach, we wanted to avoid having the reader call out every file.

[JasonLin0991]

The color contrast error is the <span> element containing the step indicator step text is  
more complicated and will probably need more investigation. From VO accessibility testing,  
neither of these violations caused issues.

For contrast violations, the violation pretty much is the issue. It's considering people who are sighted, but may be color-blind, or have other issues with their vision. As much as possible, we should try to fix these (if that means making a React-USDWS PR, maybe we do that...

I agree we should try to fix this, although I'm not sure if that would be in the scope of this PR. My reasoning is that, this violation is coming from the React USWDS component, not any code we wrote, and seems to be able to pass 508 testing. It might be something with axe runner that is unable to calculate contrast when the text element is several layers apart from the background? To find out if this is the case, in my opinion, we would have to spend some time looking into it and same case with refactoring DoubleColumnGrid, is it worth the time investment? If we did want to tackle it, I think it should be it's own jira item and my instinct is telling me it should not be included in MC Review work. Let me know what everyone thinks @macrael @haworku.

When uploads are finished the statusValue is an empty string, which causes  
the invalid values issue. Adding an additional status of ‘upload complete’ to be passed to  
statusValue fixed the violation.

One thing to test here is whether adding the "upload complete" status causes a screen reader to announce each upload as it completes. Because there's no limit to how many docs a user can attach, we wanted to avoid having the reader call out every file.

It doesn't from my testing, but I'm new to VO so anther go at it from someone else would be great! 😀

[haworku]

In the spirit of unifying our re-useable form components could you please add the role=note to the other XXField components as well please. We have the exact same time of hint handling for FieldDropdown FieldTextInput looks like.

[haworku]

re: react-uswds

• fwiw @rswerve pa11y test runner is probably not catching a valid color contrast issue with StepIndicator. In my understanding from peeking at this last week, the warning is triggered because the test trunner can't properly verify the color contrast is valid from the way the HTML is structured. 🤷
• @JasonLin0991 for the future I suggest rather than VO for these types of color related errors the best way to verify is to use a tool like Wave which is browser based. Here's a link: https://wave.webaim.org/extension/ . Its great thing to just turn on occasionally in your work. I double checked step indicator and it looks fine.
• for the aria attribute issue, can just file an issue in https://github.com/trussworks/react-uswds with the notes.

re: dl/dt issue with the Grid

• I think we should just leave it for now. No further issue needed imo. I suspect we are nesting html more than is needed for sure, but its not causing usability/a11y issues from our testing so I don't view it as a bug.
• I agree with using ignore here

[haworku]

Looks good. Let's ship. I might just copypaste the ignore versus threshold versus hideElement guidance into docs/ADR too i think you basically wrote a mini decision record about how to triage pa11y errors.