[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.
There was a problem hiding this comment.
@renow-luxembourg , do we really need this role change? This component does not guarantee an accessible name because accessibilityLabel is optional, so switching from group to region can turn existing unlabeled carousels into unnamed component. It may be safer to keep it as is.
There was a problem hiding this comment.
The role=region attribute value is recommended by the W3C design pattern.
I think that it's only a nice to have.
| 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”.
There was a problem hiding this comment.
This may break the tab to panel labeling relationship. Could we avoid this?
There was a problem hiding this comment.
The aria-labelledby attribute has a higher priority than the aria-label attribute in calculating the accessibility name.
| 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.
There was a problem hiding this comment.
The tabindex attribute of the tabpanel allows the focus to be visible when this element receives focus programmatically.
| <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?
There was a problem hiding this comment.
The aria-label attribute takes precedence over the title attribute for calculating the accessibility name. Therefore, there will be no difference for the speaker tool reading the text. However, mouse users can obtain additional information by hovering their cursor over the text.
|
@renow-luxembourg , you may need to adjust ComponentsIT.testCarousel() since the markup is different now. Please try to execute the ITs locally and see. |
4 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 :