fix(carousel): pagination carousel - accessibility improvements #2278
Conversation
kolaps33
requested review from
dvdzkwsk,
dzearing,
ecraig12345,
JasonGore,
jdhuntington,
kenotron,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Perf comparison
Generated by 🚫 dangerJS |
kolaps33
changed the title
| @@ -390,6 +393,7 @@ class Carousel extends AutoControlledComponent<WithAsProp<CarouselProps>, Carous | |||
| }) | |||
| ) : ( | |||
| <Text | |||
| aria-hidden="true" | |||
There was a problem hiding this comment.
I don't understand. If we are hiding this then what is the point in having it in the first place?
There was a problem hiding this comment.
There was relevant complain that with virtual cursor navigation user hit the text twice.
I am hiding here the second text. First text appearance still stay in each tab/slide.
| import { carouselItemBehavior } from '@fluentui/accessibility' | ||
|
|
||
| describe('carouselItemBehavior.ts', () => { | ||
| test('set tabIndex="0" when carousel has navigation and item is visible ', () => { |
There was a problem hiding this comment.
sets ... on root when ...
| @@ -17,11 +25,18 @@ import * as keyboardKey from 'keyboard-key' | |||
| const carouselBehavior: Accessibility<CarouselBehaviorProps> = props => ({ | |||
| attributes: { | |||
| root: { | |||
| role: 'region', | |||
| role: props.navigation ? undefined : 'region', | |||
There was a problem hiding this comment.
if we say that we are not setting the attribute otherwise, shouldn't we destructure these 3 attributes? Because how we are doing it now we are adding either the value or undefined, any of these options meaning that we are setting a value.
There was a problem hiding this comment.
also you can probably add the condition only once since it's the same one. something like
...(props.navigation && { role: 'region', 'aria-roledescription': props.ariaRoleDescription, 'aria-label': props.ariaLabel })| * Triggers 'arrowKeysNavigationStopPropagation' action with 'ArrowRight' or 'ArrowLeft' on 'root'. | ||
| */ | ||
| const carouselItemBehavior: Accessibility<CarouselItemProps> = props => ({ | ||
| attributes: { | ||
| root: { | ||
| role: props.navigation ? 'tabpanel' : 'group', | ||
| 'aria-hidden': props.active ? 'false' : 'true', | ||
| tabIndex: props.active ? 0 : -1, | ||
| tabIndex: props.navigation ? (props.active ? 0 : -1) : -1, |
There was a problem hiding this comment.
| tabIndex: props.navigation ? (props.active ? 0 : -1) : -1, | |
| tabIndex: (props.navigation && props.active) ? 0 : -1, |
This reverts commit fc6abda.
| }, | ||
| itemsContainerWrapper: { | ||
| 'aria-live': props.ariaLiveOn ? 'polite' : 'off', | ||
| }, | ||
| itemsContainer: { |
There was a problem hiding this comment.
same comments as above here.
|
Added a few comments @kolaps33 |
Related carousel: Carousel with Pagination
In this PR are fixes related to the feedback which we got from Rekha when I tried fix application mode for the carousel with pagination.
Issue: Index numbers (1 of 4) are read twice as you down arrow through the carousel item.
Fix: add aria-hidden="true" on the text
2.1 Issue: The first and the last item in the carousel are not read when the prev/next button is pressed in browse mode.
2.2 #2224
Fix: moving focusing the next/previous paddle to generic function, which is call always, even when "onclick" is executed...
Issue: Should there be focus on the entire carousel control and the left/right buttons? Seems redundant and adds more keyboard stops
Fix: change the behavior that for carousel without the "tab" navigation there will be tabindex=-1 on CarouselItem
Issue: Carousel itself should have a label property defined. The roledescription is set to “Carousel” so label can be “Portrait collection” for this example
Fix: added prop aria-label to be able label the carousel
Issue: #2225
Fix: was decided that if reader will narrate "carousel" user will know how to use keyboard to rotate through the carousel
For this reason to narrate "carousel" in application mode was added additional props(role, aria-label,aria-roledescription) into "itemsContainer" in "carouselBehavior.ts".
Now there is role "group". After more testing could be change for "region". For me region was not narrating "carousel" in focus mode for NVDA.
Issue: Programmatically Prev/Next buttons are outside carousel container so it will likely confuse the user. Carousel container element should include all elements within carousel.
Fix: apply region only for "Carousel with Pagination", provide there proper label and roledescription. Fix is in "carouselBehavior.ts" for "root" slot.