fix(avatar): Avatar component now always includes alt attribute #5512
+172
−57
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 6f855e0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 84 packages
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 |
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Tachometer resultsChromeavatar permalinktest-basic
Firefoxavatar permalinktest-basic
|
castastrophe
approved these changes
Jun 10, 2025
packages/avatar/src/Avatar.ts
Outdated
| alt=${ifDefined(this.label || undefined)} | ||
| src=${this.src} | ||
| /> | ||
| <img class="image" alt=${this.label || ''} src=${this.src} /> |
There was a problem hiding this comment.
Do we need to surface a dev-mode warning that alt text is missing and that the image will be skipped by screen readers if not provided?
There was a problem hiding this comment.
Having an empty alt attribute is acceptable for decorative avatars, so I'm afraid flagging it may just add console noise for valid use cases. WDYT?
There was a problem hiding this comment.
(e.g., the profile picture avatar in Adobe Home is decorative)
There was a problem hiding this comment.
Let's discuss, because the A11Y team also has concerns: #5513 (comment)
rubencarvalho
approved these changes
Jun 10, 2025
rubencarvalho
requested changes
Jun 11, 2025
There was a problem hiding this comment.
Revoking my approval for further discussion, after this comment:
#5513 (comment)
nikkimk
requested changes
Aug 1, 2025
| * When false and no label is provided, a warning will be logged. | ||
| */ | ||
| @property({ type: Boolean, reflect: true }) | ||
| public isdecorative = false; |
There was a problem hiding this comment.
nit: two-word property names are usually camelCase and then surfaced as kebab case attributes, as in isDecorative and is-decorative
I would recommend naming this either with the property and attribute name decorative.
| class="image" | ||
| alt=${ifDefined(this.label || undefined)} | ||
| alt=${ifDefined(this.isdecorative ? '' : this.label)} | ||
| aria-hidden=${this.isdecorative ? 'true' : 'false'} |
There was a problem hiding this comment.
If the avatar is keyboard navigable it should not be hidden from a screenreader.
Suggested change
| aria-hidden=${this.isdecorative ? 'true' : 'false'} | |
| aria-hidden=${this.isdecorative && !this.href ? 'true' : 'false'} |
Comment on lines
+106
to
+116
| // Log a warning if neither label nor isdecorative is set | ||
| if (!this.label && !this.isdecorative) { | ||
| window.__swc.warn( | ||
| this, | ||
| `<${this.localName}> is missing a label and is not marked as decorative. Either provide a label or set isdecorative="true" for accessibility.`, | ||
| 'https://opensource.adobe.com/spectrum-web-components/components/avatar/#accessibility', | ||
| { | ||
| type: 'accessibility', | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Suggested change
| // Log a warning if neither label nor isdecorative is set | |
| if (!this.label && !this.isdecorative) { | |
| window.__swc.warn( | |
| this, | |
| `<${this.localName}> is missing a label and is not marked as decorative. Either provide a label or set isdecorative="true" for accessibility.`, | |
| 'https://opensource.adobe.com/spectrum-web-components/components/avatar/#accessibility', | |
| { | |
| type: 'accessibility', | |
| } | |
| ); | |
| } | |
| if (!this.label) { | |
| if(this.href) { | |
| // Log a warning if avatar is a link but has no label | |
| window.__swc.warn( | |
| this, | |
| `<${this.localName}> is has a link but no label. To fix this issue, provide a label.`, | |
| 'https://opensource.adobe.com/spectrum-web-components/components/avatar/#accessibility', | |
| { | |
| type: 'accessibility', | |
| } | |
| ); | |
| } else if(!this.isdecorative) { | |
| // Log a warning if neither label nor isdecorative is set | |
| window.__swc.warn( | |
| this, | |
| `<${this.localName}> is missing a label and is not marked as decorative. Either provide a label or set isdecorative="true" for accessibility.`, | |
| 'https://opensource.adobe.com/spectrum-web-components/components/avatar/#accessibility', | |
| { | |
| type: 'accessibility', | |
| } | |
| ); | |
| } | |
| } |
Comment on lines
+163
to
+166
| - When a `label` is provided, it is used as the `alt` text for the image | ||
| - When `isdecorative` is true, the avatar is hidden from screen readers using `aria-hidden="true"` | ||
| - The component maintains focus management for keyboard navigation | ||
| - Color contrast meets WCAG 2.1 Level AA requirements |
There was a problem hiding this comment.
Suggested change
| - When a `label` is provided, it is used as the `alt` text for the image | |
| - When `isdecorative` is true, the avatar is hidden from screen readers using `aria-hidden="true"` | |
| - The component maintains focus management for keyboard navigation | |
| - Color contrast meets WCAG 2.1 Level AA requirements | |
| - When a `label` is provided, it is used as the `alt` text for the image | |
| - When the avatar has an `href`, the `label` serves as link text | |
| - When `isdecorative` is true, the avatar is hidden from screen readers using `aria-hidden="true"` | |
| - The component maintains focus management for keyboard navigation | |
| - Color contrast meets WCAG 2.1 Level AA requirements |
| ### Accessibility Best Practices | ||
|
|
||
| - Always provide a `label` for avatars that represent users or have meaningful content | ||
| - Use `isdecorative` for purely decorative avatars |
There was a problem hiding this comment.
Suggested change
| - Use `isdecorative` for purely decorative avatars | |
| - Use `isdecorative` for purely avatars that do not have an `href` and are purely decorative |
|
|
||
| ### Accessibility Best Practices | ||
|
|
||
| - Always provide a `label` for avatars that represent users or have meaningful content |
There was a problem hiding this comment.
Suggested change
| - Always provide a `label` for avatars that represent users or have meaningful content | |
| - Always provide a `label` for avatars that represent users, have meaningful content, or act as links |
caseyisonit
added
a11y
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR enhances the
<sp-avatar>component to improve accessibility, particularly for decorative avatars. The update introduces a new property, isdecorative, and updates the rendering and accessibility logic for avatars used purely for decoration.Key Changes
<sp-avatar>.Motivation
Some avatars are decorative and should not be announced by screen readers. This change allows consumers of
<sp-avatar>to explicitly mark avatars as decorative, ensuring correct ARIA attributes are applied and accessibility warnings are surfaced during development.Related Links
Checklist
isdecorativeproperty to<sp-avatar>isdecorativeisdecorativeLet me know if you want this tailored further, or if you have a specific template you’d like me to use!
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Descriptive Test Statement
Descriptive Test Statement
Device review