Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(carousel): pagination carousel - accessibility improvements #2278

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

fix(carousel): pagination carousel - accessibility improvements #2278

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
757ce6d
fixes for carousel with pagination
mituron Jan 27, 2020
c979062
adding tests for carousel item behavior
mituron Jan 28, 2020
1cb3a01
fix some lint issue
mituron Jan 29, 2020
0104d59
prettier
mituron Jan 29, 2020
3639ce7
adding aria label as prop
mituron Jan 29, 2020
8027a40
Merge branch 'master' into mituron/carousel-app-mode-fix
kolaps33 Jan 30, 2020
9ea3d70
adding logic for adding region/group differently based on carousel type
mituron Feb 6, 2020
d25bbca
improving documentation
mituron Feb 7, 2020
0d8fe8d
adresing changes from review
mituron Feb 7, 2020
4c8063e
change of do description
mituron Feb 7, 2020
6993079
fix lint issues
mituron Feb 10, 2020
14730c8
another lint fix
mituron Feb 10, 2020
12b94e5
apply prettier
mituron Feb 10, 2020
fc6abda
merging with latest master
mituron Feb 18, 2020
553cbbe
Revert "merging with latest master"
mituron Feb 18, 2020
7cdfd95
merge with latest master
mituron Feb 18, 2020
b113e78
fix based on review, stop animation - fix for narration
mituron Feb 24, 2020
a5a4ac4
disable navigation for VC adding role none
mituron Feb 24, 2020
75a3e55
fix for stealing focus when rotating with arrow keys on carousel item…
mituron Feb 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/src/examples/components/Carousel/Types/CarouselExample.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const carouselItems = [
const CarouselExample = () => (
<Carousel
ariaRoleDescription="carousel"
ariaLabel="Portrait collection"
navigation={{
'aria-label': 'people portraits',
items: carouselItems.map((item, index) => ({
Expand Down
1 change: 1 addition & 0 deletions docs/src/examples/components/Carousel/Types/CarouselPaginationExample.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const carouselItems = [
const CarouselExample = () => (
<Carousel
ariaRoleDescription="carousel"
ariaLabel="Portrait collection"
items={carouselItems}
paddleNext={{ 'aria-label': 'go to next slide' }}
paddlePrevious={{ 'aria-label': 'go to previous slide' }}
Expand Down
1 change: 1 addition & 0 deletions docs/src/examples/components/Carousel/Variations/CarouselCircularExample.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const CarouselExample = () => (
<Carousel
circular
ariaRoleDescription="carousel"
ariaLabel="Portrait collection"
navigation={{
'aria-label': 'people portraits',
items: carouselItems.map((item, index) => ({
Expand Down
6 changes: 4 additions & 2 deletions packages/accessibility/src/behaviors/Carousel/carouselItemBehavior.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,20 @@ import { Accessibility } from '../../types'
import * as keyboardKey from 'keyboard-key'

/**
* @description
* Adds attribute 'tabIndex=0' to 'root' slot if 'active' property and 'navigation' property is true. Sets the attribute to '-1' otherwise.
*
* @specification
* Adds attribute 'role=tabpanel' to 'root' slot if 'navigation' property is true. Sets the attribute to 'group' otherwise.
* Adds attribute 'aria-hidden=false' to 'root' slot if 'active' property is true. Sets the attribute to 'true' otherwise.
* Adds attribute 'tabIndex=0' to 'root' slot if 'active' property is true. Sets the attribute to '-1' otherwise.
* 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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tabIndex: props.navigation ? (props.active ? 0 : -1) : -1,
tabIndex: (props.navigation && props.active) ? 0 : -1,

},
},

Expand Down
23 changes: 23 additions & 0 deletions packages/accessibility/test/behaviors/caroseltemBehavior-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { carouselItemBehavior } from '@fluentui/accessibility'

describe('carouselItemBehavior.ts', () => {
test('set tabIndex="0" when carousel has navigation and item is visible ', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sets ... on root when ...

const expectedResult = carouselItemBehavior({ navigation: true, active: true })
expect(expectedResult.attributes.root.tabIndex).toEqual(0)
})

test('set tabIndex="-1" when carousel has navigation and item is NOT visible ', () => {
const expectedResult = carouselItemBehavior({ navigation: true, active: false })
expect(expectedResult.attributes.root.tabIndex).toEqual(-1)
})

test('set tabIndex="-1" when carousel has NO navigation and item is visible', () => {
const expectedResult = carouselItemBehavior({ navigation: false, active: true })
expect(expectedResult.attributes.root.tabIndex).toEqual(-1)
})

test('set tabIndex="-1" when carousel has NO navigation and item is NOT visible', () => {
const expectedResult = carouselItemBehavior({ navigation: false, active: false })
expect(expectedResult.attributes.root.tabIndex).toEqual(-1)
})
})
38 changes: 21 additions & 17 deletions packages/react/src/components/Carousel/Carousel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -185,27 +191,11 @@ class Carousel extends AutoControlledComponent<WithAsProp<CarouselProps>, Carous
},
showNextSlideByPaddlePress: e => {
e.preventDefault()
const { activeIndex } = this.state
const { circular, items, navigation } = this.props

this.showNextSlide(e, false)

// if 'next' paddle will disappear, will focus 'previous' one.
if (!navigation && activeIndex >= items.length - 2 && !circular) {
this.paddlePreviousRef.current.focus()
}
},
showPreviousSlideByPaddlePress: e => {
e.preventDefault()
const { activeIndex } = this.state
const { circular, navigation } = this.props

this.showPreviousSlide(e, false)

// if 'previous' paddle will disappear, will focus 'next' one.
if (!navigation && activeIndex <= 1 && !circular) {
this.paddleNextRef.current.focus()
}
},
}

Expand Down Expand Up @@ -251,7 +241,7 @@ class Carousel extends AutoControlledComponent<WithAsProp<CarouselProps>, Carous
}

renderContent = (accessibility, styles, unhandledProps) => {
const { ariaRoleDescription, getItemPositionText, items } = this.props
const { ariaRoleDescription, ariaLabel, getItemPositionText, items } = this.props
const { activeIndex, itemIds } = this.state

this.itemRefs = []
Expand All @@ -261,6 +251,7 @@ class Carousel extends AutoControlledComponent<WithAsProp<CarouselProps>, Carous
<div
className={Carousel.slotClassNames.itemsContainer}
aria-roledescription={ariaRoleDescription}
aria-label={ariaLabel}
style={styles.itemsContainer}
{...accessibility.attributes.itemsContainer}
{...applyAccessibilityKeyHandlers(
Expand Down Expand Up @@ -294,10 +285,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) => ({
Expand Down Expand Up @@ -390,6 +393,7 @@ class Carousel extends AutoControlledComponent<WithAsProp<CarouselProps>, Carous
})
) : (
<Text
aria-hidden="true"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. If we are hiding this then what is the point in having it in the first place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

className={Carousel.slotClassNames.pagination}
content={getItemPositionText(activeIndex, items.length)}
/>
Expand Down