Skip to content

LG-9424: Simplify MFA setup option labels#8327

Merged
aduth merged 3 commits intomainfrom
aduth-lg-9424-mfa-checkbox
May 5, 2023
Merged

LG-9424: Simplify MFA setup option labels#8327
aduth merged 3 commits intomainfrom
aduth-lg-9424-mfa-checkbox

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented May 3, 2023

🎫 Ticket

LG-9424

🛠 Summary of changes

Updates the labels associated with MFA setup choices so that they are more succinct and non-repetitive, and so that additional descriptive content is read as a descriptor (using aria-describedby), allowing essential role information to be shared sooner.

Recommend reviewing with whitespace changes hidden: https://github.com/18F/identity-idp/pull/8327/files?w=1

📜 Testing Plan

  1. Go to http://localhost:3000
  2. Create an account
  3. Continue through account creation until the MFA selection screen
  4. Activate screen reader
  5. Navigate checkbox options

👀 Screenshots

Screenshots via Chrome's Accessibility DevTools inspector:

Before After
image image

@aduth aduth requested a review from a team May 3, 2023 16:19
Copy link
Contributor Author

@aduth aduth May 3, 2023

Choose a reason for hiding this comment

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

Note: I realized this was a deeper rabbit hole than I imagined it would be after already pretty far down the rabbit hole 😅 (sunk cost fallacy and all that)

The reason for this is that Capybara's "label" checks are pretty forgiving, and don't require exactness. They also don't account for things like aria-hidden elements within the label. So a naive check for a label containing just "Text or voice message" would pass on main and not be sufficient for protecting against regressions.

@jmdembe
Copy link
Contributor

jmdembe commented May 3, 2023

As a part of this ticket, I am unable to trigger the reading of the auth methods description. Is that something that we want to do in this ticket?

Screen.Recording.2023-05-03.at.2.44.00.PM.mov

@aduth
Copy link
Contributor Author

aduth commented May 3, 2023

@jmdembe Is the description announced when you stay on the checkbox for a moment? It's expected that aria-describedby descriptions are often announced only after a brief pause depending on the screen reader.

I'll plan to test this out as well.

@jmdembe
Copy link
Contributor

jmdembe commented May 3, 2023

@jmdembe Is the description announced when you stay on the checkbox for a moment? It's expected that aria-describedby descriptions are often announced only after a brief pause depending on the screen reader.

I'll plan to test this out as well.

On VoiceOver, it is not automatic. After reading and pausing, it will ask if you want to see more content and then read the description from there. I think this is fine because its a tool that is available. This is probably more of a "Today I Learned" moment.

@jmdembe
Copy link
Contributor

jmdembe commented May 3, 2023

Here is what I experienced as I said in my second comment. Again, this looks okay because I would think that users would know about that toggle after it is announced

Screen.Recording.2023-05-03.at.3.18.36.PM.mov

@aduth
Copy link
Contributor Author

aduth commented May 4, 2023

Here is what I experienced as I said in my second comment. Again, this looks okay because I would think that users would know about that toggle after it is announced

This might depend on the browser? In Safari + VoiceOver, it reads for me after a short pause†. I would expect that this being the default treatment of description text is "expected", though I think it's a totally valid point to consider how (or if) this information is able to be effectively communicated. Similar to some pragmatic tips around incorporating descriptions in labels instead of aria-describedby, I think I could see a case for including it in the label if it were essential and concise, but the current content is definitely not concise, and my sense is that the role (checkbox) and value (checked/unchecked) are at least more important to be announced early.

† Aside: Unrelated to this work, I notice it also re-reads the entire "Add another layer of security by selecting a multi-factor authentication method. We recommend you select at least (2) two different options in case you lose one of your methods." for every single checkbox when navigating through the content, which is pretty exhausting. It might have to do with how we mark up that content as a fieldset``legend. It seems like we might want to rethink that, either removing the semantics, updating the content, or changing it to be referenced also as aria-describedby so that it comes later. I'll plan to create a ticket for that.

aduth added 3 commits May 5, 2023 08:24
changelog: User-Facing Improvements, Accessibility, Improve labels for MFA setup options
@aduth aduth force-pushed the aduth-lg-9424-mfa-checkbox branch from d3dc5d3 to 216adb7 Compare May 5, 2023 12:24
@aduth aduth merged commit f788592 into main May 5, 2023
@aduth aduth deleted the aduth-lg-9424-mfa-checkbox branch May 5, 2023 13:02
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.

3 participants