-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Conversation
* remove some extra span element * add aria-label attribute to icon
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 to me
Thanks for review 👍 |
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.
This code does not appear to work as intended. When I enter a really long tag that should overflow to test, the content is not hidden but completely removed.
For example "testing a really long tage that should overflow content" is cut to "testing a really long tag" (not hidden or truncated, but the content is modified)
In addition while testing with a screen reader on production and this branch I see no difference in how the screen reader perceives the content.
Can you please offer more testing detail to show where this problem exists on production and how to properly test on the branch to see the update.
My testing environment:
gitpod in chrome
using mac voice over screen reader
tested the account page and the public profile.
I also tested the same actions in the production environment btu was not able to elicit a situation where a tag would overflow and is hidden
cc @EmmaDawsonDev for thoughts on this improvement as you are more advanced at testing for accessibility.
Thank you for your review, I have tested the code on my end, and I'm not experiencing the issue you mentioned with the long tag. Could you please provide any specific scenarios where you experienced this issue
<button
type="button"
className="inline-block text-center"
onClick={() => onTagRemove(tag)}
>
<span className="absolute h-px w-px whitespace-nowrap overflow-hidden">
remove {tag} tag
</span>
<span aria-hidden="true">
<XMarkIcon
className="w-4 h-4 hover:text-tertiary-medium"
title={`Remove ${tag}`}
/>
</span>
</button> on production code we can see the first <XMarkIcon
aria-label={`remove ${tag} tag`}
aria-hidden="false"
className="w-4 h-4 hover:text-tertiary-medium"
/> In this PR we can see, my testing environment: google chrome using Microsoft Narrator over screen reader. Thanks again, looking for your feedback. Your insights are greatly appreciated |
Hey @badrivlog - I just tested again and this time the tag cutting off behavior didn't happen so we are good there, however, the tag also never gets truncated with a hidden part of the text (which is what this ticket intends to solve for screen readers if I am understanding it correctly) //see video to confirm that long tags are printing fully Re: the accessibiltiy, while what you are saying about the code is valid, what I mean to suggest and was hoping Emma can weigh in on is that the screen reader may not need the aria attribute in this case as when I tested with one on production and this branch, the readout was the same for the profile page and the account management page. (If I should be testing something else, please let me know.) Thank you again for the great collab! tag-test.mov |
I have investigated the issue you mentioned, and it seems that it's not related to the changes in this pull request. I think it would be more appropriate to create a separate issue or pull request to address this specific problem. Please let me know how you'd like to proceed, and I will be happy to make the necessary changes or create new issue if needed.
Sorry for the oversight, I forgot to include test instructions on how to test it
apologize for any inconvenience. Please let me know if you have any questions. Thank you! |
@badrivlog thank you for the clarification. The issue title was unclear so I did not realize this only covered the icon and not the entire tag. Thank you for the details! I will test again tomorrow |
@amandamartin-dev we discussed in the issue that the purpose of the span with lots of random tailwind classes was not clear and therefore we suggested removing it and replacing with an aria-label as it's more obvious what it is doing. |
Thanks Emma, I had trouble deciphering the purpose of this ticket from the title and conversation so my question can be disregarded. We've cleared up that it's only for the X icon now. Thank you for the input! |
no changes needed per conversation about scope of ticket and clarity provided by the author. These changes have been tested and are working as expected.
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.
Tested with gitpod and mac voice over. The screen reader recognizes and reads out the close button icon as expected. This is the same as on production currently, but an improvement without the extra span. Thank you for your contribution and the discussion
Fixes Issue
Closes #9132
Changes proposed
Check List (Check all the applicable boxes)
Screenshots
Note to reviewers