-
Notifications
You must be signed in to change notification settings - Fork 414
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
fix: add aria-label for removing attachment in new request form #543
Conversation
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.
👍🏼🙌🏼🌐
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 added a couple of comments, and we also need to wrap the FileListItem
components with the FileList
component here as shown in the Garden examples: https://66bcf1412f3b1af56ce3fee1--zendeskgarden.netlify.app/components/file-upload#files
<File type="generic" title={fileName} onKeyDown={handleFileKeyDown}> | ||
<File | ||
type="generic" | ||
title={fileName} |
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 think we don't need this title
anymore
title={fileName} | ||
tabIndex={0} | ||
aria-label={t( | ||
"new-request-form.attachemnts.file", |
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.
Nit: there is a typo in the key (attachemnts
)
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.
Hi @gosiexon-zen, thanks so much for making this change. I just tested it out in my local Guide instance, and I believe it's working as expected:
Screen.Recording.2024-11-20.at.3.35.57.PM.mov
Is there any chance we could update the "Remove file" button (line 74 of FileListItem.tsx) so it includes the file name, like the label for the group does? That way, if a screen reader user uploads multiple files, they'll be able to tell the "Remove" buttons apart, when navigating the page via form controls.
- Current behavior: "Remove file"
- Preferred behavior: "Remove file: {{fileName}}"
452cc68
to
7f7e667
Compare
7f7e667
to
42b503d
Compare
Hi @jgorfine-zendesk I applied your suggestions, but I think there should be |
1cd913c
to
57c4458
Compare
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.
Reapproving 👍🏼🌐
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.
Hi @gosiexon-zen, I've requested a couple more small changes, but, after that, we should be good to go. 🙂 (I'm approving this PR anyway because the requested changes are more of a nice-to-have than a must-have.)
Here's the current screen reader experience on VoiceOver x Safari:
Screen.Recording.2024-11-22.at.3.32.22.PM.mov
aria-label={t( | ||
"new-request-form.attachments.remove-file-arria-label", | ||
"Remove file: {{fileName}}", | ||
{ fileName } | ||
)} |
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.
Can we please add aria-describedby={undefined}
here? That way, the screen reader won't read out the "Remove file" text from the tooltip after it reads out the name of the button.
aria-label={t( | |
"new-request-form.attachments.remove-file-arria-label", | |
"Remove file: {{fileName}}", | |
{ fileName } | |
)} | |
aria-label={t( | |
"new-request-form.attachments.remove-file-arria-label", | |
"Remove file: {{fileName}}", | |
{ fileName } | |
)} | |
aria-describedby={undefined} |
- Current screen reader announcement: Remove file: [file name], Remove file, button
- Expected screen reader announcement: Remove file: [file name], button
(The word "button" comes from it being an HTML button element.)
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.
Ok I applied suggestion
aria-label={t( | ||
"new-request-form.attachments.stop-upload-aria-label", | ||
"Stop uploading {{fileName}}", | ||
{ fileName } | ||
)} |
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.
Can we please add aria-describedby={undefined}
to avoid a duplicate screen reader announcement here, too?
aria-label={t( | |
"new-request-form.attachments.stop-upload-aria-label", | |
"Stop uploading {{fileName}}", | |
{ fileName } | |
)} | |
aria-label={t( | |
"new-request-form.attachments.stop-upload-aria-label", | |
"Stop uploading {{fileName}}", | |
{ fileName } | |
)} | |
aria-describedby={undefined} |
- Current screen reader announcement: Stop uploading [file name], Stop uploading, button
- Expected screen reader announcement: Stop uploading [file name], button
(The word "button" comes from it being an HTML button element.)
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.
Ok I applied suggestion
57c4458
to
6127e57
Compare
🎉 This PR is included in version 4.2.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Added aria-label with information how to remove the attachment while navigating with keyboard.
This A11y issue was reported here: GG-3796
Here is also explanation why we don't make the remove button accessible by tab navigation: https://zendesk.atlassian.net/browse/GG-3796?focusedCommentId=2055655
Screenshots
Checklist