Skip to content

Jmax/lg 11294 add aria tags to cancel idv verify screen#9508

Merged
jmax-gsa merged 9 commits intomainfrom
jmax/LG-11294-add-aria-tags-to-cancel-idv-verify-screen
Nov 27, 2023
Merged

Jmax/lg 11294 add aria tags to cancel idv verify screen#9508
jmax-gsa merged 9 commits intomainfrom
jmax/LG-11294-add-aria-tags-to-cancel-idv-verify-screen

Conversation

@jmax-gsa
Copy link
Contributor

@jmax-gsa jmax-gsa commented Nov 1, 2023

🎫 Ticket

LG-11294

🛠 Summary of changes

  • Added aria-label attributes to the Rails-generated (via the button_to helper) <form> tags for the buttons on the 'cancel IdV' screen (the buttons are 'No, keep going', 'Start over', and either 'Go to account page' or 'Exit <>'
  • Added view spec support for testing button_to generated forms for aria attributes
  • Added feature spec support for testing aria labeling on form landmarks

📜 Testing Plan

The review app for testing is here

  • Create a user and enter IdV
  • Select 'Cancel' from any of the IdV screens
  • You are now on the 'Cancel IdV' screen. Use the browser's View Source to inspect the HTML, and verify that each of the buttons has the corret 'aria-label' attribute on its enclosing form.
  • Exit IdV, log out, and then start IdV from the Sinatra sample SP app.
  • Examine the HTML for the 'Cancel IdV' screen again, and verify that the aria-label tags are still present.

👀 Screenshots

Start over button

start_over

Keep going button

keep_going

Account page button

account_page

@jmax-gsa jmax-gsa requested a review from aduth November 1, 2023 15:21
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I think it makes sense to have a unique label for each form and it's probably okay that those are the same as the button text, but I'm not sure I follow that the changes to how we compute accessible names are necessary. For a regression spec, I might imagine we could check for this using a new matcher that checks for unique names of landmarks if multiple of the same type exist in the page (form in this case). Something like expect(page).to have_unique_landmark_labels.

Will replace with a more general check, per slack conversation w/
Andrew Duthie
No new entries, but the .erb file changed enough to confuse brakeman
about the three existing entries.
@jmax-gsa jmax-gsa force-pushed the jmax/LG-11294-add-aria-tags-to-cancel-idv-verify-screen branch from cb16209 to 57d5997 Compare November 2, 2023 13:53
changelog: User-Facing Improvements,remote proofing,Added accessibility tag  attributes to cancel screen
@jmax-gsa jmax-gsa force-pushed the jmax/LG-11294-add-aria-tags-to-cancel-idv-verify-screen branch from f05b1ce to 8d27610 Compare November 2, 2023 15:23
@jmax-gsa jmax-gsa marked this pull request as ready for review November 2, 2023 16:54
@jmax-gsa jmax-gsa requested a review from a team November 2, 2023 16:55
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Couple comments, but LGTM otherwise 👍

)

click_on t('idv.cancel.actions.keep_going')
expect(page).to have_unique_form_landmark_labels
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could add this to the existing expect_page_to_have_no_accessibility_violations common helper? Maybe that'd start flagging issues with unrelated screens.

Not sure if it covers all the variations, but we have a spec which calls that helper for the IdV cancel screen:

scenario 'cancel idv' do
sign_in_and_2fa_user
visit idv_cancel_path
expect(current_path).to eq idv_cancel_path
expect_page_to_have_no_accessibility_violations(page)
end

match do |page|
labels = landmarks(page).map { |element| AccessibleName.new(page:).computed_name(element) }

labels.none?(&:nil?) && labels.uniq.length == labels.length
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the first part of this condition? I think if we were to adapt this for other screens, we're more concerned with testing for unique labels if a page has more than one form, so if the page has only one form and it has a nil label, that seems fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmmm. Maybe (labels.length == 1 || labels.none?(&:nil)) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, though that doesn't check that the labels are unique across multiple forms. Originally I was thinking it'd be enough to keep the rest of the current logic (labels.uniq.length == labels.length) but that wouldn't flag if there was multiple forms and only one or some of them were labeled.

Maybe this?

labels.one? || labels.compact.uniq.length == labels.length

Co-authored by Andrew Duthie <andrew.duthie@gsa.gov>
@jmax-gsa jmax-gsa merged commit 7af3f49 into main Nov 27, 2023
@jmax-gsa jmax-gsa deleted the jmax/LG-11294-add-aria-tags-to-cancel-idv-verify-screen branch November 27, 2023 21:15
@aduth aduth mentioned this pull request Mar 28, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants