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.

Sign up for GitHub

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

added: [M3-6989] - aria-describedby to TextField with helper text #11351

Merged
merged 7 commits into from
Dec 6, 2024
Merged

added: [M3-6989] - aria-describedby to TextField with helper text #11351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b012453
Add aria-describedby to TextField with helper text
abailly-akamai Dec 2, 2024
addbae4
Always some ID
abailly-akamai Dec 3, 2024
3ab45e5
cleanup
abailly-akamai Dec 3, 2024
6d9cb2a
add coverage
abailly-akamai Dec 3, 2024
8437f9c
Added changeset: aria-describedby to TextField with helper text
abailly-akamai Dec 3, 2024
b317a03
cleanup
abailly-akamai Dec 3, 2024
fd53887
feedback @hkhalil-akamai
abailly-akamai Dec 4, 2024
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 packages/manager/.changeset/pr-11351-added-1733194164261.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Added
---

aria-describedby to TextField with helper text ([#11351](https://github.com/linode/manager/pull/11351))
32 changes: 32 additions & 0 deletions packages/ui/src/components/TextField/TextField.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,36 @@ describe('TextField', () => {
fireEvent.change(input, { target: { value: '1' } });
expect(input?.getAttribute('value')).toBe('2');
});

it('renders a helper text with an input id', () => {
const { getByText } = renderWithTheme(
<TextField helperText="Helper text" inputId="input-id" label="" />
);

expect(getByText('Helper text')).toBeInTheDocument();
const helperText = getByText('Helper text');
expect(helperText.getAttribute('id')).toBe('input-id-helper-text');
});

it('renders a helper text with a label', () => {
const { getByText } = renderWithTheme(
<TextField helperText="Helper text" label="Label" />
);

const helperText = getByText('Helper text');

expect(helperText).toBeInTheDocument();
expect(helperText.getAttribute('id')).toBe('label-helper-text');
});

it('renders a helper text with a fallback id', () => {
const { getByText } = renderWithTheme(
<TextField helperText="Helper text" label="" />
);

const helperText = getByText('Helper text');

// ':rg:' being the default react generated id
expect(helperText.getAttribute('id')).toBe(':rg:-helper-text');
});
});
19 changes: 16 additions & 3 deletions packages/ui/src/components/TextField/TextField.tsx
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make the same change when displaying error messages. VoiceOver currently isn't able to pick them up:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hkhalil-akamai That's fair, this one has a role="alert" attribute which is very much meant for this type of behavior (aka, a validation error injected into the DOM) but could indeed use more context.

see commit 95a8d3e which follows this recommended pattern

Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ interface InputToolTipProps {
tooltipWidth?: number;
}

interface TextFieldPropsOverrides extends StandardTextFieldProps {
interface TextFieldPropsOverrides
extends Omit<StandardTextFieldProps, 'label'> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This helps TS in general not having to compare overridden types. While in this case this is just a string and this likely has no impact, it's just good practice.

// We override this prop to make it required
label: string;
}
Expand Down Expand Up @@ -166,6 +167,7 @@ export const TextField = (props: TextFieldProps) => {

const [_value, setValue] = React.useState<Value>(value);
const theme = useTheme();
const fallbackId = React.useId();

React.useEffect(() => {
setValue(value);
Expand Down Expand Up @@ -249,7 +251,14 @@ export const TextField = (props: TextFieldProps) => {
}

const validInputId =
inputId || (label ? convertToKebabCase(`${label}`) : undefined);
inputId ||
(label
? convertToKebabCase(label)
: // label could still be an empty string
fallbackId);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

always making sure we have an ID


const helperTextId = `${validInputId}-helper-text`;
const errorTextId = `${validInputId}-error-text`;

const labelSuffixText = required
? '(required)'
Expand Down Expand Up @@ -317,6 +326,7 @@ export const TextField = (props: TextFieldProps) => {
marginTop: theme.spacing(),
}}
data-qa-textfield-helper-text
id={helperTextId}
>
{helperText}
</FormHelperText>
Expand Down Expand Up @@ -364,6 +374,9 @@ export const TextField = (props: TextFieldProps) => {
...SelectProps,
}}
inputProps={{
'aria-describedby': helperText ? helperTextId : undefined,
'aria-errormessage': errorText ? errorTextId : undefined,
'aria-invalid': !!error || !!errorText,
'data-testid': 'textfield-input',
id: validInputId,
...inputProps,
Expand Down Expand Up @@ -437,7 +450,7 @@ export const TextField = (props: TextFieldProps) => {
</FormHelperText>
)}
{helperText && helperTextPosition === 'bottom' && (
<FormHelperText data-qa-textfield-helper-text>
<FormHelperText data-qa-textfield-helper-text id={helperTextId}>
{helperText}
</FormHelperText>
)}
Expand Down
Loading