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

fix(Loader): Adding labeling when loader get focus #2352

Merged
merged 11 commits into from
Feb 14, 2020
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Fix `input` descenders being cropped in the Teams theme @bcalvery ([#2335](https://github.com/microsoft/fluent-ui-react/pull/2335))
- Use referentially equal objects for `actions` in `useStateManager` @layershifter ([#2347](https://github.com/microsoft/fluent-ui-react/pull/2347))
- Fix `Animation` component not to throw when `children` is not provided @mnajdova ([#2345](https://github.com/microsoft/fluent-ui-react/pull/2345))
- Fix `loader` - adding labeling when loader get focus @kolaps33 ([#2352](https://github.com/microsoft/fluent-ui-react/pull/2352))

### Features
- Added sourcemaps to the dist output to simplify debugging @miroslavstastny ([#2329](https://github.com/microsoft/fluent-ui-react/pull/2329))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import * as React from 'react'

import ComponentBestPractices from '../../../../components/ComponentBestPractices'

const doList = [
'Use react-aria-live or similar component to announce the loading state.',
'If loader is only the element in the screen or region, consider making it focusable by setting `tabIndex` prop to the `Loader`. In most of these cases value of `tabIndex` should be 0.',
]

const LoaderBestPractices: React.FunctionComponent<{}> = () => {
return <ComponentBestPractices doList={doList} />
}

export default LoaderBestPractices
32 changes: 26 additions & 6 deletions packages/accessibility/src/behaviors/Loader/loaderBehavior.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,37 @@ import { Accessibility } from '../../types'
/**
* @description
* Loader is usually an element that displays the progress status for a task that take a long time or consists of several steps.
* Adds attribute 'aria-labelledby' on 'root' when loader has 'tabIndex' prop. This can be overriden by providing 'aria-labelledby' or 'aria-label' property directly to the component.
*
* @specification
* Adds role 'progressbar' to 'root' slot.
*/

const loaderBehavior: Accessibility = () => ({
attributes: {
root: {
role: 'progressbar',
const loaderBehavior: Accessibility<LoaderBehaviorProps> = props => {
return {
attributes: {
root: {
role: 'progressbar',
'aria-labelledby': getDefaultAriaLabelledBy(props),
},
},
},
})
}
}

type LoaderBehaviorProps = {
/** id of the loader label element. */
labelId?: string
}

export default loaderBehavior

/**
* Returns the id of the loader label if user provide tabIndex prop. It is used when user does not provide aria-label or
* aria-labelledby as prop.
*/
const getDefaultAriaLabelledBy = (props: LoaderBehaviorProps) => {
if (props['aria-label'] || props['aria-labelledby']) {
return undefined
}
return props['tabIndex'] === undefined ? undefined : props.labelId
}
33 changes: 33 additions & 0 deletions packages/accessibility/test/behaviors/loaderBehavior-test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { loaderBehavior } from '@fluentui/accessibility'

describe('LoaderBehavior.ts', () => {
test('do NOT add aria-labelledby, when aria-label was set already', () => {
const props = { labelId: 'label-id', 'aria-label': 'any loading string' }
const expectedResult = loaderBehavior(props)
expect(expectedResult.attributes.root['aria-labelledby']).toEqual(undefined)
})

test('do NOT add aria-labelledby, when aria-labelled was set already', () => {
const props = { labelId: 'label-id', 'aria-labelledby': 'id' }
const expectedResult = loaderBehavior(props)
expect(expectedResult.attributes.root['aria-labelledby']).toEqual(undefined)
})

test('do NOT add aria-labelledby, when there is no tabIndex specified', () => {
const props = { labelId: 'label-id' }
const expectedResult = loaderBehavior(props)
expect(expectedResult.attributes.root['aria-labelledby']).toEqual(undefined)
})

test('add aria-labelledby, when there is tabIndex=0 specified', () => {
const props = { labelId: 'label-id', tabIndex: 0 }
const expectedResult = loaderBehavior(props)
expect(expectedResult.attributes.root['aria-labelledby']).toEqual('label-id')
})

test('add aria-labelledby, when there is tabIndex=-1 specified', () => {
const props = { labelId: 'label-id', tabIndex: -1 }
const expectedResult = loaderBehavior(props)
expect(expectedResult.attributes.root['aria-labelledby']).toEqual('label-id')
})
})
17 changes: 15 additions & 2 deletions packages/react/src/components/Loader/Loader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
commonPropTypes,
SizeValue,
ShorthandFactory,
getOrGenerateIdFromShorthand,
} from '../../utils'
import { WithAsProp, ShorthandValue, withSafeTypeForAs } from '../../types'
import Box, { BoxProps } from '../Box/Box'
Expand Down Expand Up @@ -49,6 +50,7 @@ export interface LoaderProps extends UIComponentProps {

export interface LoaderState {
visible: boolean
labelId: string
}

/**
Expand Down Expand Up @@ -94,6 +96,13 @@ class Loader extends UIComponent<WithAsProp<LoaderProps>, LoaderState> {

this.state = {
visible: this.props.delay === 0,
labelId: '',
}
}

static getDerivedStateFromProps(props, state) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's type this method as in Dialog

return {
labelId: getOrGenerateIdFromShorthand('loader-label-', props.label, state.labelId),
}
Copy link
Member

Choose a reason for hiding this comment

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

Why don't use getDerivedStateFromProps()? This will produce an additional render

Copy link
Contributor

@jurokapsiar jurokapsiar Feb 13, 2020

Choose a reason for hiding this comment

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

do you mean getAutoControlledStateFromProps @layershifter? - please see Dialog.tsx

Copy link
Contributor

Choose a reason for hiding this comment

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

getDerivedStateFromProps should be enough. This is not Autocontrolled component.

}

Expand All @@ -114,7 +123,7 @@ class Loader extends UIComponent<WithAsProp<LoaderProps>, LoaderState> {

renderComponent({ ElementType, classes, accessibility, variables, styles, unhandledProps }) {
const { indicator, label, svg } = this.props
const { visible } = this.state
const { visible, labelId } = this.state

const svgElement = Box.create(svg, {
defaultProps: () => ({ className: Loader.slotClassNames.svg, styles: styles.svg }),
Expand All @@ -135,7 +144,11 @@ class Loader extends UIComponent<WithAsProp<LoaderProps>, LoaderState> {
}),
})}
{Text.create(label, {
defaultProps: () => ({ className: Loader.slotClassNames.label, styles: styles.label }),
defaultProps: () => ({
className: Loader.slotClassNames.label,
styles: styles.label,
id: labelId,
}),
})}
</ElementType>
)
Expand Down