-
Notifications
You must be signed in to change notification settings - Fork 33
[FE-112] Boundary Enterprise a11y audit- User details form #2977
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
cameronperera
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
a40d4ac to
334e7d6
Compare
ZedLi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for the update
hashicc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for helping us with our a11y!
2ec32be
| delete: Delete User | ||
| edit-details: | ||
| button-text: 'Edit User details' | ||
| aria-describedby: 'user-details' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change aria-describedby to 'edit-user-details' to match button text (if you need aria-describedby).
This adds edit user details translations so the User detail form submit button is a11y compliant ✅ Closes: FE-112
Per a11y audit, this changes the input fields to readonly instead of disabled. It also updates the submit button text so that it is context based and has an aria-label for screenreaders. ✅ Closes: FE-112
This moves the User detail action translation to resources/en-us.yaml since it is specific to the user resource and is not a generic form action. ✅ Closes: FE-112
This was not sending the aria-label down to the Edit Users detail button. However, taking a look further at Hds::Button, this component does not allow aria-label to be set unless it is an icon only button. With that said, this cleans up the translation structure and removes the aria-label translation and mark-up. ✅ Closes: FE-112
Since we cannot apply aria-label to an Hds::Button when it has text, we can alternatively apply the aria-describedby attribute instead. Its value must match the id of the form. With that said, a form id value was also added. ✅ Closes: FE-112
2ec32be to
ee255d4
Compare
This updates the translation to be more explicit to edit user details and removes the unnecessary aria at attribute in the form ✅ Closes: FE-112
| {{yield to='edit'}} | ||
| {{else}} | ||
| <Hds::Button | ||
| aria-describedby={{@ariaDescribedBy}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we pass in @ariaDescribedBy now?
| delete: Delete User | ||
| edit-details: | ||
| button-text: 'Edit User details' | ||
| aria-describedby: 'edit-user-details' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have a translation for an aria-describedby
| {{#if (can 'save model' @model)}} | ||
| <form.actions | ||
| @enableEditText={{t 'actions.edit-form'}} | ||
| @enableEditText={{t 'resources.user.actions.edit-details.button-text'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe we need the extra aria-described by stuff since you've already modified the edit text itself to be more descriptive for everyone.
To be clear, I think we have 3 reasonable options here of which only one needs to get done:
- Do what you did here and just change the edit text itself to more descriptive
- Keep the edit button to just say
Edit Formbut add anaria-descriptionwith the translated string that saysEdit Form: Users. - Keep the edit button to just say
Edit Formbut add anaria-describedbywith the ID of the form title, in this case it would be the one here. We would need to add in our own ID for the title so it's not an auto-generated one so we can refer to it with thearia-describedby. We would also need to refactor to remove theDocLinkfrom the title element (and fix the CSS as well) since that content would also be read by the screen reader and instead probably put it in theBadgessection like this:
<@header.Title id="header-title">
{{t 'resources.user.title'}}
</@header.Title>
<@header.Badges>
<DocLink @doc='user' />
</@header.Badges>
Of the three, I think 1 and 2 is easiest but 3 is more convenient as a developer since you don't need to add a new translated string for every edit button, just the ID and refactor of the header. They're all fine at the end of the day though.
Description
This updates the input fields on the User details page so that they are
readonlyinstead ofdisabled. Additionally it changes the edit button text fromEdit formtoEdit User detailsso that it is more descriptive. Lastly,aria-describedbywas also added to the button so that it is a11y compliant.🎟️ FE-112
Screenshots (if appropriate)
How to Test
Navigate to a User in the Users list. Verify that the input fields are readonly (you should be able to highlight the text with your mouse) and that the submit button reflects the text changes shown in the after screenshot.
Checklist
a11y-testslabel to run a11y audit tests if neededPCI review checklist
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.