Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TextInput: Add LeadingVisual/TrailingVisual to be part of the accessible description in TextInput #4939

Merged
merged 20 commits into from
Oct 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/five-windows-invent.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

Adds `aria-describedby` for `LeadingVisual` and `TrailingVisual` in `TextInput`; adds new prop `loaderText` to convey loading state to screen readers
6 changes: 6 additions & 0 deletions packages/react/src/TextInput/TextInput.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@
"defaultValue": "",
"description": "<div>Which position to render the loading indicator</div> <ul> <li> 'auto' (default): at the end of the input, unless a `leadingVisual` is passed. Then, it will render at the beginning </li> <li>'leading': at the beginning of the input</li> <li>'trailing': at the end of the input</li> </ul>"
},
{
"name": "loaderText",
"type": "string",
"defaultValue": "Loading",
"description": "Text for screen readers to convey the loading state, should be descriptive and explain what is loading. This prop should only be used if there is visible context explaining what is loading, to ensure that context is provided to all users."
},
{
"name": "leadingVisual",
"type": "string | React.ComponentType",
Expand Down
56 changes: 32 additions & 24 deletions packages/react/src/TextInput/TextInput.features.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -98,31 +98,39 @@ export const Required = () => (
</Box>
)

export const WithLeadingVisual = () => (
<Box as="form">
<FormControl>
<FormControl.Label>Default label</FormControl.Label>
<TextInput leadingVisual={CheckIcon} />
</FormControl>
<FormControl>
<FormControl.Label>Enter monies</FormControl.Label>
<TextInput leadingVisual="$" />
</FormControl>
</Box>
)
export const WithLeadingVisual = () => {
const Checkmark = () => <CheckIcon aria-label="Checkmark" />

export const WithTrailingIcon = () => (
<Box>
<FormControl>
<FormControl.Label>Default label</FormControl.Label>
<TextInput trailingVisual={CheckIcon} />
</FormControl>
<FormControl>
<FormControl.Label>Enter monies</FormControl.Label>
<TextInput trailingVisual="minutes" placeholder="200" />
</FormControl>
</Box>
)
return (
<Box as="form">
<FormControl>
<FormControl.Label>Default label</FormControl.Label>
<TextInput leadingVisual={Checkmark} />
</FormControl>
<FormControl>
<FormControl.Label>Enter monies</FormControl.Label>
<TextInput leadingVisual="$" />
</FormControl>
</Box>
)
}

export const WithTrailingIcon = () => {
const Checkmark = () => <CheckIcon aria-label="Checkmark" />

return (
<Box>
<FormControl>
<FormControl.Label>Default label</FormControl.Label>
<TextInput trailingVisual={Checkmark} />
</FormControl>
<FormControl>
<FormControl.Label>Enter monies</FormControl.Label>
<TextInput trailingVisual="minutes" placeholder="200" />
</FormControl>
</Box>
)
}

export const WithTrailingAction = () => {
const [value, setValue] = useState('sample text')
Expand Down
24 changes: 23 additions & 1 deletion packages/react/src/TextInput/TextInput.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type {MouseEventHandler} from 'react'
import React, {useCallback, useState} from 'react'
import React, {useCallback, useState, useId} from 'react'
import {isValidElementType} from 'react-is'
import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'
import {clsx} from 'clsx'
Expand All @@ -11,6 +11,7 @@ import type {StyledWrapperProps} from '../internal/components/TextInputWrapper'
import TextInputWrapper from '../internal/components/TextInputWrapper'
import TextInputAction from '../internal/components/TextInputInnerAction'
import UnstyledTextInput from '../internal/components/UnstyledTextInput'
import VisuallyHidden from '../_VisuallyHidden'

export type TextInputNonPassthroughProps = {
/** @deprecated Use `leadingVisual` or `trailingVisual` prop instead */
Expand All @@ -24,6 +25,8 @@ export type TextInputNonPassthroughProps = {
* 'trailing': at the end of the input
**/
loaderPosition?: 'auto' | 'leading' | 'trailing'
/** Text for screen readers to convey the loading state */
loaderText?: string
Comment on lines +28 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 I wonder if it's really necessary / a good idea for consumers to be able to customize this? It inherently means they'll be providing a different experience for sighted vs non-sighted users, because sighted users won't be able to access this text. Visually a loading indicator only indicates that something is loading, so "Loading" should be sufficient always.

Alternatively, we could make this visible by putting it in a tooltip. But that doesn't seem ideal since the loader is not focusable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, and good point! This mainly stems from https://github.com/github/primer/issues/3465. I believe the idea is, if there's context outside of the input that conveys what is loading then it might be a good idea to add more descriptive text to indicate what is loading so users don't have to navigate outside of it to get that context.

I think this is a more specific use case that won't be used unless necessary, so in most cases having the default "Loading" is okay, which we could make more descriptive 🤔

Cc: @patrickhlauke - curious if you have any additional thoughts here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the idea is, if there's context outside of the input that conveys what is loading then it might be a good idea to add more descriptive text to indicate what is loading so users don't have to navigate outside of it to get that context.

If we do keep this prop, I think this (particularly the par about the context outside the input) seems like it would be useful to add to the props documentation. I think what may happen is consumers will see that this prop exists and think that they should fill it out similarly to how they fill out alt text in an image - they'll have good intentions but actually create a less accessible experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, that's totally fair @iansan5653! I can add a note on this in the props docs to make sure consumers aren't haphazardly using it!

/**
* A visual that renders inside the input before the typing area
*/
Expand Down Expand Up @@ -67,6 +70,7 @@ const TextInput = React.forwardRef<HTMLInputElement, TextInputProps>(
disabled,
loading,
loaderPosition = 'auto',
loaderText = 'Loading',
monospace,
validationStatus,
sx: sxProp,
Expand Down Expand Up @@ -96,6 +100,18 @@ const TextInput = React.forwardRef<HTMLInputElement, TextInputProps>(
const focusInput: MouseEventHandler = () => {
inputRef.current?.focus()
}
const leadingVisualId = useId()
const trailingVisualId = useId()
const loadingId = useId()

const inputDescribedBy =
clsx(
inputProps['aria-describedby'],
LeadingVisual && leadingVisualId,
TrailingVisual && trailingVisualId,
loading && loadingId,
) || undefined

const handleInputFocus = useCallback(
(e: React.FocusEvent<HTMLInputElement>) => {
setIsInputFocused(true)
Expand Down Expand Up @@ -137,6 +153,8 @@ const TextInput = React.forwardRef<HTMLInputElement, TextInputProps>(
visualPosition="leading"
showLoadingIndicator={showLeadingLoadingIndicator}
hasLoadingIndicator={typeof loading === 'boolean'}
id={leadingVisualId}
data-testid="text-input-leading-visual"
>
{typeof LeadingVisual !== 'string' && isValidElementType(LeadingVisual) ? <LeadingVisual /> : LeadingVisual}
</TextInputInnerVisualSlot>
Expand All @@ -149,12 +167,16 @@ const TextInput = React.forwardRef<HTMLInputElement, TextInputProps>(
aria-required={required}
aria-invalid={validationStatus === 'error' ? 'true' : undefined}
{...inputProps}
aria-describedby={inputDescribedBy}
data-component="input"
/>
{loading && <VisuallyHidden id={loadingId}>{loaderText}</VisuallyHidden>}
<TextInputInnerVisualSlot
visualPosition="trailing"
showLoadingIndicator={showTrailingLoadingIndicator}
hasLoadingIndicator={typeof loading === 'boolean'}
id={trailingVisualId}
TylerJDev marked this conversation as resolved.
Show resolved Hide resolved
data-testid="text-input-trailing-visual"
>
{typeof TrailingVisual !== 'string' && isValidElementType(TrailingVisual) ? (
<TrailingVisual />
Expand Down
68 changes: 68 additions & 0 deletions packages/react/src/__tests__/TextInput.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -236,4 +236,72 @@ describe('TextInput', () => {
const {getByRole} = HTMLRender(<TextInput onChange={onChange} aria-invalid="true" value="" />)
expect(getByRole('textbox')).toHaveAttribute('aria-invalid', 'true')
})

it('should include the leadingVisual as part of the input accessible description', () => {
const {getByRole} = HTMLRender(<TextInput leadingVisual="Search" />)
expect(getByRole('textbox')).toHaveAccessibleDescription('Search')
})

it('should include the leadingVisual icon as part of the input accessible description', () => {
const Icon = () => <SearchIcon aria-label="Search" />

const {getByRole} = HTMLRender(<TextInput leadingVisual={Icon} />)
const icon = getByRole('img', {hidden: true})

expect(getByRole('textbox')).toHaveAttribute('aria-describedby', icon.parentElement?.id)
expect(icon).toHaveAccessibleName('Search')
})

it('should include the trailingVisual as part of the input accessible description', () => {
const {getByRole} = HTMLRender(<TextInput trailingVisual="Search" />)
expect(getByRole('textbox')).toHaveAccessibleDescription('Search')
})

it('should include the trailingVisual icon as part of the input accessible description', () => {
const Icon = () => <SearchIcon aria-label="Search" />

const {getByRole} = HTMLRender(<TextInput trailingVisual={Icon} />)
const icon = getByRole('img', {hidden: true})

expect(getByRole('textbox')).toHaveAttribute('aria-describedby', icon.parentElement?.id)
expect(icon).toHaveAccessibleName('Search')
})

it('should include both the leadingVisual and trailingVisual as part of the input accessible description', () => {
const {getByRole} = HTMLRender(<TextInput leadingVisual="$" trailingVisual="Currency" />)
expect(getByRole('textbox')).toHaveAccessibleDescription('$ Currency')
})

it('should keep the passed aria-describedby value', () => {
const {getByRole} = HTMLRender(
<>
<span id="passedValue">value</span>
<TextInput leadingVisual="leading" trailingVisual="trailing" aria-describedby="passedValue" />
</>,
)
expect(getByRole('textbox').getAttribute('aria-describedby')).toContain('passedValue')
expect(getByRole('textbox')).toHaveAccessibleDescription('value leading trailing')
})

it('should include the loading indicator as part of the input accessible description', () => {
const {getByRole} = HTMLRender(<TextInput loading />)
expect(getByRole('textbox')).toHaveAccessibleDescription('Loading')
})

it('should include the leadingVisual and loading indicator as part of the input accessible description', () => {
const {getByRole} = HTMLRender(
<TextInput loading loaderText="Loading search items" loaderPosition="trailing" leadingVisual="Search" />,
)
expect(getByRole('textbox')).toHaveAccessibleDescription('Search Loading search items')
})

it('should include the trailingVisual and loading indicator as part of the input accessible description', () => {
const {getByRole} = HTMLRender(<TextInput loading loaderPosition="leading" trailingVisual="Search" />)
expect(getByRole('textbox')).toHaveAccessibleDescription('Search Loading')
})

it('should not have an aria-describedby if there is no leadingVisual, trailingVisual, or loading indicator', () => {
const {getByRole} = HTMLRender(<TextInput />)
expect(getByRole('textbox')).not.toHaveAttribute('aria-describedby')
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ exports[`snapshots renders a loading state 1`] = `
>
<svg
aria-hidden={true}
aria-labelledby=":r1v:"
aria-labelledby=":r2h:"
className="c3"
fill="none"
height="32px"
Expand All @@ -404,7 +404,7 @@ exports[`snapshots renders a loading state 1`] = `
</svg>
<span
className="c4"
id=":r1v:"
id=":r2h:"
>
Loading
</span>
Expand Down
Loading
Loading