Skip to content
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
20 changes: 17 additions & 3 deletions packages/react-core/src/components/Form/FormGroup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ export interface FormGroupProps extends Omit<React.HTMLProps<HTMLDivElement>, 'l
helperTextInvalidIcon?: React.ReactNode;
/** ID of the included field. It has to be the same for proper working. */
fieldId: string;
/** Sets the role of the form group. Pass in "radiogroup" when the form group contains multiple
* radio inputs, or pass in "group" when the form group contains multiple of any other input type.
*/
role?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this only be radiogroup or group? If so i would suggest defining a union.

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be more than just radiogroup or group, role is an attribute like aria-label that could be a lot of things. But if it's radio/radiogroup this PR makes it automatically also do some dynamic wiring of a few aria-attributes so that the inputgroups of radio or checkboxes will be labelled correctly.

}

export const FormGroup: React.FunctionComponent<FormGroupProps> = ({
Expand All @@ -60,6 +64,7 @@ export const FormGroup: React.FunctionComponent<FormGroupProps> = ({
helperTextIcon,
helperTextInvalidIcon,
fieldId,
role,
...props
}: FormGroupProps) => {
const validHelperText =
Expand Down Expand Up @@ -96,30 +101,39 @@ export const FormGroup: React.FunctionComponent<FormGroupProps> = ({
const helperTextToDisplay =
validated === ValidatedOptions.error && helperTextInvalid ? inValidHelperText : showValidHelperTxt(validated);

const isGroupOrRadioGroup = role === 'group' || role === 'radiogroup';
const LabelComponent = isGroupOrRadioGroup ? 'span' : 'label';

const labelContent = (
<React.Fragment>
<label className={css(styles.formLabel)} htmlFor={fieldId}>
<LabelComponent className={css(styles.formLabel)} {...(!isGroupOrRadioGroup && { htmlFor: fieldId })}>
<span className={css(styles.formLabelText)}>{label}</span>
{isRequired && (
<span className={css(styles.formLabelRequired)} aria-hidden="true">
{' '}
{ASTERISK}
</span>
)}
</label>{' '}
</LabelComponent>{' '}
{React.isValidElement(labelIcon) && labelIcon}
</React.Fragment>
);

return (
<div {...props} className={css(styles.formGroup, className)}>
<div
className={css(styles.formGroup, className)}
{...(role && { role })}
{...(isGroupOrRadioGroup && { 'aria-labelledby': `${fieldId}-legend` })}
{...props}
>
{label && (
<div
className={css(
styles.formGroupLabel,
labelInfo && styles.modifiers.info,
hasNoPaddingTop && styles.modifiers.noPaddingTop
)}
{...(isGroupOrRadioGroup && { id: `${fieldId}-legend` })}
>
{labelInfo && (
<React.Fragment>
Expand Down
22 changes: 18 additions & 4 deletions packages/react-core/src/components/Form/FormSection.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as React from 'react';
import styles from '@patternfly/react-styles/css/components/Form/form';
import { css } from '@patternfly/react-styles';
import { GenerateId } from '../../helpers/GenerateId/GenerateId';

export interface FormSectionProps extends Omit<React.HTMLProps<HTMLDivElement>, 'title'> {
/** Content rendered inside the section */
Expand All @@ -20,9 +21,22 @@ export const FormSection: React.FunctionComponent<FormSectionProps> = ({
titleElement: TitleElement = 'div',
...props
}: FormSectionProps) => (
<section {...props} className={css(styles.formSection, className)}>
{title && <TitleElement className={css(styles.formSectionTitle, className)}>{title}</TitleElement>}
{children}
</section>
<GenerateId prefix="pf-form-section-title">
{sectionId => (
<section
className={css(styles.formSection, className)}
role="group"
{...(title && { 'aria-labelledby': sectionId })}
{...props}
>
{title && (
<TitleElement id={sectionId} className={css(styles.formSectionTitle, className)}>
{title}
</TitleElement>
)}
{children}
</section>
)}
</GenerateId>
);
FormSection.displayName = 'FormSection';
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ export const InternalFormFieldGroup: React.FunctionComponent<InternalFormFieldGr
return (
<div
className={css(styles.formFieldGroup, isExpanded && isExpandable && styles.modifiers.expanded, className)}
role="group"
{...(headerTitleText && { 'aria-labelledby': `${header.props.titleText.id}` })}
{...props}
>
{isExpandable && (
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from 'react';

import { render } from '@testing-library/react';
import { render, screen } from '@testing-library/react';

import { FormFieldGroup } from '../FormFieldGroup';
import { FormFieldGroupExpandable } from '../FormFieldGroupExpandable';
Expand Down Expand Up @@ -53,3 +53,25 @@ test('Verify console error logged when there is no aria-label or title', () => {
const { asFragment } = render(FieldGroup);
expect(consoleErrorMock).toHaveBeenCalled();
});

test('Verify field group has accessible name when header is passed in', () => {
render(
<FormFieldGroup
header={
<FormFieldGroupHeader
titleText={{ text: 'Field group 4 (non-expandable)', id: 'title-text-id1' }}
titleDescription="Field group 4 description text."
actions={<Button />}
/>
}
/>
);

expect(screen.getByRole('group')).toHaveAccessibleName('Field group 4 (non-expandable)');
});

test('Verify field group does not have accessible name when header is not passed in', () => {
render(<FormFieldGroup data-testid="field-group-test-id" />);

expect(screen.getByRole('group')).not.toHaveAccessibleName();
});
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';

import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

import { ValidatedOptions } from '../../../helpers/constants';
import { FormGroup } from '../FormGroup';
Expand Down Expand Up @@ -203,4 +204,92 @@ describe('FormGroup', () => {
);
expect(asFragment()).toMatchSnapshot();
});

test('should render without group or radiogroup role when role is not passed in', () => {
render(
<FormGroup label="label" fieldId="no-group-role">
<input />
</FormGroup>
);

expect(screen.queryByRole('group')).not.toBeInTheDocument();
expect(screen.queryByRole('radiogroup')).not.toBeInTheDocument();
});

test('input should receive focus when clicking text label', () => {
render(
<FormGroup label="label" fieldId="native-label-element">
<input id="native-label-element" />
</FormGroup>
);

const labelElement = screen.getByText('label');
const input = screen.getByRole('textbox');

userEvent.click(labelElement);
expect(input).toHaveFocus();
});

describe('With multiple inputs per group', () => {
test('inputs should not receive focus when clicking text label', () => {
render(
<FormGroup label="label" fieldId="span-label-element" role="group">
<input id="span-label-element" />
<input />
</FormGroup>
);

const labelElement = screen.getByText('label');
const inputs = screen.getAllByRole('textbox');

userEvent.click(labelElement);
inputs.forEach(input => {
expect(input).not.toHaveFocus();
});
});

test('should render with group role', () => {
render(
<FormGroup label="label" fieldId="group-role" role="group">
<input />
<input />
</FormGroup>
);

expect(screen.getByRole('group')).toBeInTheDocument();
});

test('should render with radiogroup role', () => {
render(
<FormGroup label="label" fieldId="radiogroup-role" role="radiogroup">
<input />
<input />
</FormGroup>
);

expect(screen.getByRole('radiogroup')).toBeInTheDocument();
});

test('should have accessible name when group role is passed in', () => {
render(
<FormGroup label="label" fieldId="accessible-name" role="group">
<input />
<input />
</FormGroup>
);

expect(screen.getByRole('group')).toHaveAccessibleName('label');
});

test('should have accessible name when radiogroup role is passed in', () => {
render(
<FormGroup label="label" fieldId="accessible-name" role="radiogroup">
<input />
<input />
</FormGroup>
);

expect(screen.getByRole('radiogroup')).toHaveAccessibleName('label');
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

One test I would add here: a sanity check test for the expected behavior when multipleInputs is not passed or is otherwise falsy.

});
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from 'react';
import { FormSection } from '../FormSection';
import { render } from '@testing-library/react';
import { render, screen } from '@testing-library/react';

test('Check form section example against snapshot', () => {
const Section = <FormSection />;
Expand All @@ -13,3 +13,15 @@ test('Check form section example with title', () => {
const { asFragment } = render(Section);
expect(asFragment()).toMatchSnapshot();
});

test('Verify form section has accessible name when title is passed in', () => {
render(<FormSection title="Form title" />);

expect(screen.getByRole('group')).toHaveAccessibleName('Form title');
});

test('Verify form section does not have accessible name when title is not passed in', () => {
render(<FormSection />);

expect(screen.getByRole('group')).not.toHaveAccessibleName();
});
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
exports[`Check expandable form field group example against snapshot 1`] = `
<DocumentFragment>
<div
aria-labelledby="title-text-id2"
class="pf-c-form__field-group pf-m-expanded"
role="group"
>
<div
class="pf-c-form__field-group-toggle"
Expand Down Expand Up @@ -88,7 +90,9 @@ exports[`Check expandable form field group example against snapshot 1`] = `
exports[`Check form field group example against snapshot 1`] = `
<DocumentFragment>
<div
aria-labelledby="title-text-id1"
class="pf-c-form__field-group"
role="group"
>
<div
class="pf-c-form__field-group-header"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,21 @@ exports[`Check form section example against snapshot 1`] = `
<DocumentFragment>
<section
class="pf-c-form__section"
role="group"
/>
</DocumentFragment>
`;

exports[`Check form section example with title 1`] = `
<DocumentFragment>
<section
aria-labelledby="pf-form-section-title1"
class="pf-c-form__section"
role="group"
>
<h4
class="pf-c-form__section-title"
id="pf-form-section-title1"
>
Title
</h4>
Expand Down
Loading