Skip to content

LG-15252 Add accessible link to password strength feedback when creating password#11907

Merged
kevinsmaster5 merged 9 commits intomainfrom
kmas-lg-15252-accessible-password-strength-feedback
Feb 25, 2025
Merged

LG-15252 Add accessible link to password strength feedback when creating password#11907
kevinsmaster5 merged 9 commits intomainfrom
kmas-lg-15252-accessible-password-strength-feedback

Conversation

@kevinsmaster5
Copy link
Contributor

@kevinsmaster5 kevinsmaster5 commented Feb 20, 2025

🎫 Ticket

Link to the relevant ticket:
LG-15252

🛠 Summary of changes

In order to have screen reader recognize what shows up in the password strength block, we wrap the heading, 'rating' and hint text in a div and add its id to the password aria-describedby attribute.

📜 Testing Plan

Confirm the changes.

Pre requisite: have ability for assistive screen reading on your device

  • go to http://localhost:3000 and create a new account
  • After verifying email and at the password screen turn on the screen reader (MacOs use VoiceOver in System settings > Accessibility
  • Enter a password that would give a non-passing grade such as a common word or just repeated characters. Tab into Confirm password or click over there.
  • Notice the screen reader will read out the Password strength and tip advice along with the current password instructions.

👀 Screenshots

Screen recording of MacOS VoiceOver captioning showing rough idea of how a screen reader plays the described text

Screen.Recording.2025-02-20.at.2.20.30.PM.mov

@kevinsmaster5 kevinsmaster5 marked this pull request as ready for review February 20, 2025 21:29
@kevinsmaster5 kevinsmaster5 requested a review from a team February 21, 2025 13:30
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 minor suggestions, but LGTM otherwise 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test coverage 👍 It could also be a good idea to incorporate what's expected to happen if the user clears a value after adding one (i.e. that the aria-describedby is removed again).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a case where TypeScript is right in warning us that we can't guarantee that aria-describedby attribute is there. I might suggest using the optional chaining operator (?.) instead of the non-null assertion (!.).

Suggested change
this.input.getAttribute('aria-describedby')!.replace(/\s*password-strength\s*/, ''),
this.input.getAttribute('aria-describedby')?.replace(/\s*password-strength\s*/, ''),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works if I add a non-null to the end otherwise TS was still complaining.

Copy link
Contributor

Choose a reason for hiding this comment

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

This works if I add a non-null to the end otherwise TS was still complaining.

Can you share the error you are seeing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
  Type 'undefined' is not assignable to type 'string'.ts(2345)

Screenshot 2025-02-25 at 9 20 23 AM (2)

Copy link
Contributor

@aduth aduth Feb 25, 2025

Choose a reason for hiding this comment

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

Ah, it's because the optional chaining will result in trying to set null as the value if there is no attribute present. Maybe we could address this by changing the else to an else if (this.input.hasAttribute('aria-describedby')), and then it'd be safe to use the non-null assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That TypeScript error remains but interestingly if I take the constant I'm setting in the if () { move it up and use it in both cases TypeScript stays happy. I think it's inferring string | null

161ba96

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that seems like a sensible approach 👍

I think the issue you were seeing is that TypeScript can't infer a connection between hasAttribute and getAttribute to know that the condition satisfied that the attribute definitely exists, whereas storing and reusing the variable reference, it knows it's safe to use if already checked in the condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much!

@kevinsmaster5 kevinsmaster5 force-pushed the kmas-lg-15252-accessible-password-strength-feedback branch from 7a1ed46 to 58b203a Compare February 25, 2025 14:00
@kevinsmaster5 kevinsmaster5 merged commit 1697ec7 into main Feb 25, 2025
2 checks passed
@kevinsmaster5 kevinsmaster5 deleted the kmas-lg-15252-accessible-password-strength-feedback branch February 25, 2025 16:17
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