Skip to content

Avoid ValidatedFieldComponent missing aria-describedby references#7484

Merged
aduth merged 17 commits intomainfrom
aduth-validated-field-describedby
Dec 16, 2022
Merged

Avoid ValidatedFieldComponent missing aria-describedby references#7484
aduth merged 17 commits intomainfrom
aduth-validated-field-describedby

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Dec 14, 2022

🎫 Ticket

Resolves LG-7579

🛠 Summary of changes

The changes here seek to improve the accessibility of ValidatedFieldComponent by avoiding assignment of aria-describedby ID references which aren't present on the page.

I noticed in a recent aXe run of our homepage that an issue is flagged for "ARIA attributes must confirm to valid values". This is not flagged by our automated feature specs for accessibility, despite using the same underlying aXe tooling (even after upgrading). It may be that the browser extension is ahead of the RSpec integration.

Previously, it was intentional that the ID references were assigned even if the elements were not present, so that the ID reference would be used in creating the element in JavaScript for displaying an error message if a field becomes invalid. It was assumed that the absence of the element corresponding to the ID would be ignored, which was the desired effect, and which appeared to be the case in testing. It's quite likely that the issue being flagged is primarily accounting for a situation where a description is expected to be present. Regardless, we should aim to avoid any issues being identified in scans.

An additional issue is being fixed here: Previously, when a field become valid again after previously being invalid, we would hide the error message by assigning a display: none; style. While this would visually hide the error message, the message would continue to be used as the computed accessible description of the field, since aria-describedby does not ignore elements which are hidden†. (cc @NavaTim I recall this being added for MemorableDateComponent behaviors, and I'd want to double-check there are no regressions in the behavior of what it had been added for)

† Source: https://www.w3.org/TR/accname-1.1/#mapping_additional_nd_te

an author can [...] include hidden text as part of the accessible name or accessible description by using aria-labelledby or aria-describedby.

📜 Testing Plan

  • Confirm that a ValidatedFieldComponent input does not have aria-describedby unless it is described by either a hint or an error message
    • Examples:
      • Sign In page fields have no aria-describedby
      • MFA OTP fields have aria-describedby corresponding to the example hint
      • On the "Add phone" screen, if you bypass native form validation by adding novalidate the page's form element and then submit the form, the subsequent page should have aria-describedby corresponding to the initial page-rendered field error
  • Confirm there are no regressions in the expected behavior of a field:
    • Error messages are shown on attempt to submit with any invalid values
    • Error messages are cleared on any input

changelog: Bug Fixes, Accessibility, Fix missing ID references for field descriptors
@aduth aduth requested a review from NavaTim December 14, 2022 15:52
@aduth aduth mentioned this pull request Dec 14, 2022
6 tasks
aduth and others added 4 commits December 14, 2022 15:19
Inspired by @svalexander's idea in #7488

Co-Authored-By: Shannon A <20867088+svalexander@users.noreply.github.com>
May want to revisit. Could also consider ":empty" pseudo-selector
@aduth aduth marked this pull request as draft December 15, 2022 16:29
Previously, I picked "month" as an arbitrary field to find the error, but since now the field validity aria-describedby is managed per-field, the relationship would happen at the field level. We can (and ideally should) check the accessible description for the field we're interested in checking.
technically not an issue, but we should avoid having "empty" id refs be considered
we could revert to what existed previously with "not_to have_css", but stronger guarantee that we check both the text is empty, and that it's explicitly present but not visible
@aduth aduth marked this pull request as ready for review December 15, 2022 16:47
Copy link
Contributor

@NavaTim NavaTim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, LGTM! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants