Skip to content

Commit

Permalink
[v4] fix(InputGroup): don't add aria-describedby automatically (#9671)
Browse files Browse the repository at this point in the history
* fix(InputGroup): don't add aria-describedby automatically

For fixing #9667

* test(NumberInput): drop aria-describedby workaround

Test was setting the `aria-describedby=''` attribute to a NumericInput
inside an InputGroup, most probably to avoid the autogenerated value
(see #9667).

However, after cleaning up the InputGroup component,
`aria-describedby=''` is not a workaround anymore for unset the
attribute, breaking the snapshot associated to the test.
  • Loading branch information
dgdavid authored Oct 16, 2023
1 parent f5feb0b commit 388ec12
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 54 deletions.
18 changes: 1 addition & 17 deletions packages/react-core/src/components/InputGroup/InputGroup.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import * as React from 'react';
import styles from '@patternfly/react-styles/css/components/InputGroup/input-group';
import { css } from '@patternfly/react-styles';
import { FormSelect } from '../FormSelect';
import { TextArea } from '../TextArea';
import { TextInput } from '../TextInput';

export interface InputGroupProps extends React.HTMLProps<HTMLDivElement> {
/** Additional classes added to the input group. */
Expand All @@ -20,25 +17,12 @@ export const InputGroup: React.FunctionComponent<InputGroupProps> = ({
innerRef,
...props
}: InputGroupProps) => {
const formCtrls = [FormSelect, TextArea, TextInput].map(comp => comp.displayName);
const idItem = React.Children.toArray(children).find(
(child: any) => !formCtrls.includes(child.type.displayName) && child.props.id
) as React.ReactElement<{ id: string }>;

const ref = React.useRef(null);
const inputGroupRef = innerRef || ref;

return (
<div ref={inputGroupRef} className={css(styles.inputGroup, className)} {...props}>
{idItem
? React.Children.map(children, (child: any) =>
!formCtrls.includes(child.type.displayName) || child.props['aria-describedby']
? child
: React.cloneElement(child, {
'aria-describedby': child.props['aria-describedby'] === '' ? undefined : idItem.props.id
})
)
: children}
{children}
</div>
);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,47 +7,18 @@ import { Button } from '../../Button';
import { TextInput } from '../../TextInput';

describe('InputGroup', () => {
test('add aria-describedby to form-control if one of the non form-controls has id', () => {
// In this test, TextInput is a form-control component and Button is not.
// If Button has an id props, this should be used in aria-describedby.
// Regression test for https://github.com/patternfly/patternfly-react/issues/9667
test('wont add aria-describedby automatically to form-control', () => {
render(
<InputGroup>
<TextInput value="some data" aria-label="some text" />
<Button variant="primary" id="button-id">
hello
<TextInput aria-label="User password" />
<Button variant="primary" id="show-password-toggler">
Show
</Button>
</InputGroup>
);
expect(screen.getByLabelText('some text')).toHaveAttribute('aria-describedby', 'button-id');
});

test('wont add aria-describedby to form-control if describedby is empty string', () => {
// In this test, TextInput is a form-control component and Button is not.
// If Button has an id props, this should be used in aria-describedby, but this
// example has an empty aria-describedby to prevent that from happening.
render(
<InputGroup>
<TextInput value="some data" aria-describedby="" aria-label="some text" />
<Button id="button-id">
hello
</Button>
</InputGroup>
);
expect(screen.getByLabelText('some text')).not.toHaveAttribute('aria-describedby');
});

test('wont override aria-describedby in form-control if describedby has value', () => {
// In this test, TextInput is a form-control component and Button is not.
// If Button has an id props, this should be used in aria-describedby, but this
// example has a predefined aria-describedby to prevent that from happening
render(
<InputGroup>
<TextInput value="some data" aria-describedby="myself" aria-label="some text" />
<Button id="button-id">
hello
</Button>
</InputGroup>
);
expect(screen.getByLabelText('some text')).toHaveAttribute('aria-describedby', 'myself');
const formControl = screen.getByLabelText("User password");
expect(formControl).not.toHaveAttribute("aria-describedby");
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ describe('numberInput', () => {
<NumberInput
value={5}
onMinus={jest.fn()}
inputProps={{ 'aria-describedby': '' }}
minusBtnProps={{ id: 'minus-id' }}
onPlus={jest.fn()}
plusBtnProps={{ id: 'plus-id' }}
Expand Down

0 comments on commit 388ec12

Please sign in to comment.