This repository was archived by the owner on Mar 4, 2020. It is now read-only.
fix(carousel): pagination carousel - accessibility improvements #2278
There are no files selected for viewing
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| import { carouselBehavior } from '@fluentui/accessibility' | ||
|
|
||
| const roleDescription = 'carousel' | ||
| const label = 'portrait collection' | ||
|
|
||
| describe('carouselBehavior.ts', () => { | ||
| describe('root', () => { | ||
| test(`sets "role=region" when carousel has NO navigation`, () => { | ||
| const expectedResult = carouselBehavior({ ariaLiveOn: false, navigation: false }) | ||
| expect(expectedResult.attributes.root.role).toEqual('region') | ||
| }) | ||
|
|
||
| test('sets "aria-roledescription" when carousel has NO navigation', () => { | ||
| const expectedResult = carouselBehavior({ | ||
| ariaLiveOn: false, | ||
| navigation: false, | ||
| ariaRoleDescription: roleDescription, | ||
| }) | ||
| expect(expectedResult.attributes.root['aria-roledescription']).toEqual(roleDescription) | ||
| }) | ||
|
|
||
| test('sets "aria-label" when carousel has NO navigation', () => { | ||
| const expectedResult = carouselBehavior({ | ||
| ariaLiveOn: false, | ||
| navigation: false, | ||
| ariaLabel: label, | ||
| }) | ||
| expect(expectedResult.attributes.root['aria-label']).toEqual(label) | ||
| }) | ||
|
|
||
| test('do NOT set "aria-roledescription" when carousel has navigation', () => { | ||
| const expectedResult = carouselBehavior({ | ||
| ariaLiveOn: false, | ||
| navigation: true, | ||
| ariaRoleDescription: roleDescription, | ||
| }) | ||
| expect(expectedResult.attributes.root['aria-roledescription']).toBeUndefined() | ||
| }) | ||
|
|
||
| test('do NOT set "aria-label" when carousel has navigation', () => { | ||
| const expectedResult = carouselBehavior({ | ||
| ariaLiveOn: false, | ||
| navigation: true, | ||
| ariaLabel: label, | ||
| }) | ||
| expect(expectedResult.attributes.root['aria-label']).toBeUndefined() | ||
| }) | ||
|
|
||
| test(`do NOT set "role=region" when carousel has navigation`, () => { | ||
| const expectedResult = carouselBehavior({ ariaLiveOn: false, navigation: true }) | ||
| expect(expectedResult.attributes.root.role).toBeUndefined() | ||
| }) | ||
| }) | ||
|
|
||
| describe('itemsContainer', () => { | ||
| test('sets "aria-roledescription" when carousel has navigation', () => { | ||
| const expectedResult = carouselBehavior({ | ||
| ariaLiveOn: false, | ||
| navigation: true, | ||
| ariaRoleDescription: roleDescription, | ||
| }) | ||
| expect(expectedResult.attributes.itemsContainer['aria-roledescription']).toEqual( | ||
| roleDescription, | ||
| ) | ||
| }) | ||
|
|
||
| test('sets "aria-label" when carousel has navigation', () => { | ||
| const expectedResult = carouselBehavior({ | ||
| ariaLiveOn: false, | ||
| navigation: true, | ||
| ariaLabel: label, | ||
| }) | ||
| expect(expectedResult.attributes.itemsContainer['aria-label']).toEqual(label) | ||
| }) | ||
|
|
||
| test('do NOT set "aria-roledescription" when carousel has NO navigation', () => { | ||
| const expectedResult = carouselBehavior({ | ||
| ariaLiveOn: false, | ||
| navigation: false, | ||
| ariaRoleDescription: roleDescription, | ||
| }) | ||
| expect(expectedResult.attributes.itemsContainer['aria-roledescription']).toBeUndefined() | ||
| }) | ||
|
|
||
| test('do NOT set "aria-label" when carousel has NO navigation', () => { | ||
| const expectedResult = carouselBehavior({ | ||
| ariaLiveOn: false, | ||
| navigation: false, | ||
| ariaLabel: label, | ||
| }) | ||
| expect(expectedResult.attributes.itemsContainer['aria-label']).toBeUndefined() | ||
| }) | ||
|
|
||
| test(`do NOT set "role=group" when carousel has NO navigation`, () => { | ||
| const expectedResult = carouselBehavior({ ariaLiveOn: false, navigation: false }) | ||
| expect(expectedResult.attributes.itemsContainer.role).toBeUndefined() | ||
| }) | ||
| }) | ||
| }) |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import { carouselItemBehavior } from '@fluentui/accessibility' | ||
|
|
||
| describe('carouselItemBehavior.ts', () => { | ||
| test('sets tabIndex="0" on root when carousel has navigation and item is visible ', () => { | ||
| const expectedResult = carouselItemBehavior({ navigation: true, active: true }) | ||
| expect(expectedResult.attributes.root.tabIndex).toEqual(0) | ||
| }) | ||
|
|
||
| test('sets tabIndex="-1" on root when carousel has navigation and item is NOT visible ', () => { | ||
| const expectedResult = carouselItemBehavior({ navigation: true, active: false }) | ||
| expect(expectedResult.attributes.root.tabIndex).toEqual(-1) | ||
| }) | ||
|
|
||
| test('sets tabIndex="-1" on root when carousel has NO navigation and item is visible', () => { | ||
| const expectedResult = carouselItemBehavior({ navigation: false, active: true }) | ||
| expect(expectedResult.attributes.root.tabIndex).toEqual(-1) | ||
| }) | ||
|
|
||
| test('sets tabIndex="-1" on root when carousel has NO navigation and item is NOT visible', () => { | ||
| const expectedResult = carouselItemBehavior({ navigation: false, active: false }) | ||
| expect(expectedResult.attributes.root.tabIndex).toEqual(-1) | ||
| }) | ||
| }) |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,11 @@ export interface CarouselProps extends UIComponentProps, ChildrenComponentProps | |
| */ | ||
| ariaRoleDescription?: string | ||
|
|
||
| /** | ||
| * Sets the aria-label attribute for carousel. | ||
| */ | ||
| ariaLabel?: string | ||
|
|
||
| /** Specifies if the process of switching slides is circular. */ | ||
| circular?: boolean | ||
|
|
||
|
|
@@ -128,6 +133,7 @@ class Carousel extends AutoControlledComponent<WithAsProp<CarouselProps>, Carous | |
| }), | ||
| activeIndex: PropTypes.oneOfType([PropTypes.number, PropTypes.string]), | ||
| ariaRoleDescription: PropTypes.string, | ||
| ariaLabel: PropTypes.string, | ||
| circular: PropTypes.bool, | ||
| defaultActiveIndex: PropTypes.oneOfType([PropTypes.number, PropTypes.string]), | ||
| getItemPositionText: PropTypes.func, | ||
|
|
@@ -185,27 +191,11 @@ class Carousel extends AutoControlledComponent<WithAsProp<CarouselProps>, Carous | |
| }, | ||
| showNextSlideByPaddlePress: e => { | ||
| e.preventDefault() | ||
| this.showNextSlide(e, false) | ||
| }, | ||
| showPreviousSlideByPaddlePress: e => { | ||
| e.preventDefault() | ||
| this.showPreviousSlide(e, false) | ||
| }, | ||
| } | ||
|
|
||
|
|
@@ -251,7 +241,7 @@ class Carousel extends AutoControlledComponent<WithAsProp<CarouselProps>, Carous | |
| } | ||
|
|
||
| renderContent = (accessibility, styles, unhandledProps) => { | ||
| const { getItemPositionText, items } = this.props | ||
| const { activeIndex, itemIds } = this.state | ||
|
|
||
| this.itemRefs = [] | ||
|
|
@@ -260,7 +250,6 @@ class Carousel extends AutoControlledComponent<WithAsProp<CarouselProps>, Carous | |
| <div style={styles.itemsContainerWrapper} {...accessibility.attributes.itemsContainerWrapper}> | ||
| <div | ||
| className={Carousel.slotClassNames.itemsContainer} | ||
| style={styles.itemsContainer} | ||
| {...accessibility.attributes.itemsContainer} | ||
| {...applyAccessibilityKeyHandlers( | ||
|
|
@@ -294,10 +283,22 @@ class Carousel extends AutoControlledComponent<WithAsProp<CarouselProps>, Carous | |
|
|
||
| showPreviousSlide = (e: React.SyntheticEvent, focusItem: boolean) => { | ||
| this.setActiveIndex(e, this.state.activeIndex - 1, focusItem) | ||
| // if 'previous' paddle will disappear, will focus 'next' one. | ||
| if (!this.props.navigation && this.state.activeIndex <= 1 && !this.props.circular) { | ||
| this.paddleNextRef.current.focus() | ||
| } | ||
| } | ||
|
|
||
| showNextSlide = (e: React.SyntheticEvent, focusItem: boolean) => { | ||
| this.setActiveIndex(e, this.state.activeIndex + 1, focusItem) | ||
| // if 'next' paddle will disappear, will focus 'previous' one. | ||
| if ( | ||
| !this.props.navigation && | ||
| this.state.activeIndex >= this.props.items.length - 2 && | ||
| !this.props.circular | ||
| ) { | ||
| this.paddlePreviousRef.current.focus() | ||
| } | ||
| } | ||
|
|
||
| handlePaddleOverrides = (predefinedProps: ButtonProps, paddleName: string) => ({ | ||
|
|
@@ -390,6 +391,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. |
||
| className={Carousel.slotClassNames.pagination} | ||
| content={getItemPositionText(activeIndex, items.length)} | ||
| /> | ||
|
|
||
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also you can probably add the condition only once since it's the same one. something like