[Carousel] Accessibility improvments - v2#3018
[Carousel] Accessibility improvments - v2#3018renow-luxembourg wants to merge 2 commits intoadobe:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
| id="${carousel.id}" | ||
| class="cmp-carousel" | ||
| role="group" | ||
| role="region" |
There was a problem hiding this comment.
Switching from role="group" to role="region" makes sense here since the carousel is a distinct section. Just wondering if we should ensure carousel.accessibilityLabel is always present so the region doesn’t end up unnamed for screen readers.
| role="group" | ||
| role="region" | ||
| aria-label="${carousel.accessibilityLabel}" | ||
| aria-live="polite" |
There was a problem hiding this comment.
Removing aria-live="polite" seems like a good change. Auto-rotating carousels can otherwise keep announcing updates and become noisy for screen reader users.
| id="${item.id}-tabpanel" | ||
| class="cmp-carousel__item${item.name == carousel.activeItem ? ' cmp-carousel__item--active' : ''}" | ||
| role="tabpanel" | ||
| aria-labelledby="${item.id}-tab" |
There was a problem hiding this comment.
The new "Slide X of Y" label is clearer than relying on aria-labelledby. Quick question: does itemList.count start at 1 here? Just checking to avoid the first slide being announced as “Slide 0”.
| data-cmp-data-layer="${item.data.json}" | ||
| data-cmp-hook-carousel="item"></div> | ||
| data-cmp-hook-carousel="item" | ||
| tabindex="-1"></div> |
There was a problem hiding this comment.
I noticed tabindex="-1" was added to the slide container. Is this used for programmatic focus when slides change? Just wanted to confirm the intent.
| <button class="cmp-carousel__action cmp-carousel__action--previous" | ||
| type="button" | ||
| aria-label="${carousel.accessibilityPrevious || 'Previous' @ i18n}" | ||
| title="${carousel.accessibilityPrevious || 'Previous' @ i18n}" |
There was a problem hiding this comment.
Adding the title attribute on the navigation buttons looks helpful for hover tooltips. Since we already have aria-label, this shouldn’t affect accessibility behavior, right?
3 participants
Here is a simplify version of the following PR: #2930
This PR doesn't modify the DOM structure to avoid any backwards compatibility issues.
Suggestions that are regression-free to improve accessibility :