-
-
Notifications
You must be signed in to change notification settings - Fork 40
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 accessible names for embed youtube link #165
Conversation
✅ Deploy Preview for astro-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
<a {href} class="lty-playbtn" role="link" aria-label={title}> | ||
<span class="lyt-visually-hidden">{attrs.playlabel}</span> |
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 the PR!
This looks like it’s actually a bug in the use of attrs.playLabel
as a child of the link. As documented in the <YouTube>
component docs, this should have a default value of "Play"
, but it looks like we’re not providing that default, which we should do here:
<a {href} class="lty-playbtn" role="link" aria-label={title}> | |
<span class="lyt-visually-hidden">{attrs.playlabel}</span> | |
<a {href} class="lty-playbtn"> | |
<span class="lyt-visually-hidden">{attrs.playlabel || 'Play'}</span> |
In general, actually text content is preferable to aria-label
as it is more widely accessible. There is also no need to apply a role="link"
to an <a>
element as this is already the default role.
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.
yeah, you're right if i provide playlabel
that accessiblity issue gone, so i'll add default value now as you suggested.
🦋 Changeset detectedLatest commit: d1d0c43 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Perfect — thanks @anotheri! I added a test and changeset and will get this released now.
ARIA commands must have an accessible name, read more: https://dequeuniversity.com/rules/axe/4.10/aria-command-name
Otherwise i'm getting this issue in lighthouse Accessibility report: