Skip to content

Render InputGroup, ButtonGroup, and Radio as <fieldset> to improve controlling and accessibility #466

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

Merged
merged 2 commits into from
May 22, 2023
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
2 changes: 1 addition & 1 deletion src/lib/components/Button/Button.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export const Button = React.forwardRef((props, ref) => {
inputGroupContext && styles.isRootInInputGroup,
feedbackIcon && styles.hasRootFeedback,
)}
disabled={resolveContextOrProp(buttonGroupContext && buttonGroupContext.disabled, disabled) || !!feedbackIcon}
disabled={resolveContextOrProp(primaryContext && primaryContext.disabled, disabled) || !!feedbackIcon}
id={id}
ref={ref}
>
Expand Down
2 changes: 1 addition & 1 deletion src/lib/components/Button/__tests__/Button.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ describe('rendering', () => {
expect(within(rootElement).getByText('label')).toHaveAttribute('id', 'id__labelText');
},
],
...labelPropTest,
...labelPropTest(),
[
{ labelVisibility: 'sm' },
(rootElement) => expect(rootElement).toHaveClass('hasLabelVisibleSm'),
Expand Down
6 changes: 3 additions & 3 deletions src/lib/components/ButtonGroup/ButtonGroup.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@ export const ButtonGroup = ({
}

return (
<div
<fieldset
{...transferProps(restProps)}
className={classNames(
styles.root,
block && styles.isRootBlock,
getRootPriorityClassName(priority, styles),
)}
role="group"
disabled={disabled}
>
<ButtonGroupContext.Provider
value={{
Expand All @@ -40,7 +40,7 @@ export const ButtonGroup = ({
>
{children}
</ButtonGroupContext.Provider>
</div>
</fieldset>
);
};

Expand Down
7 changes: 7 additions & 0 deletions src/lib/components/ButtonGroup/README.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@ See [API](#api) for all available options.
the [SelectField](/components/select-field) or
[Radio](/components/radio) components.

- In the background, ButtonGroup uses the [`fieldset`][fieldset] element. Not
only it improves the [accessibility] of the group, it also allows you to make
use of its built-in features like disabling all nested inputs or pairing the
group with a form outside. Consult [the MDN docs][fieldset] to learn more.

- Be careful with using `startCorner` and `endCorner` options for grouped
buttons. Overflowing elements may cause undesired interaction problems.

Expand Down Expand Up @@ -277,5 +282,7 @@ its accessibility.
| `--rui-ButtonGroup--flat__separator__width` | Separator width for `flat` buttons |
| `--rui-ButtonGroup--flat__separator__color` | Separator color for `flat` buttons |

[fieldset]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/fieldset
[accessibility]: https://www.w3.org/WAI/tutorials/forms/grouping/
[React synthetic events]: https://reactjs.org/docs/events.html
[div]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/div#attributes
10 changes: 8 additions & 2 deletions src/lib/components/ButtonGroup/__tests__/ButtonGroup.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,17 @@ describe('rendering', () => {
],
[
{ disabled: true },
(rootElement) => expect(within(rootElement).getByRole('button')).toBeDisabled(),
(rootElement) => {
expect(rootElement).toBeDisabled();
expect(within(rootElement).getByRole('button')).toBeDisabled();
},
],
[
{ disabled: false },
(rootElement) => expect(within(rootElement).getByRole('button')).not.toBeDisabled(),
(rootElement) => {
expect(rootElement).not.toBeDisabled();
expect(within(rootElement).getByRole('button')).not.toBeDisabled();
},
],
[
{ priority: 'filled' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { disabledPropTest } from '../../../../../tests/propTests/disabledPropTes
import { refPropTest } from '../../../../../tests/propTests/refPropTest';
import { helpTextPropTest } from '../../../../../tests/propTests/helpTextPropTest';
import { formLayoutProviderTest } from '../../../../../tests/providerTests/formLayoutProviderTest';
import { isLabelVisible } from '../../../../../tests/propTests/isLabelVisible';
import { isLabelVisibleTest } from '../../../../../tests/propTests/isLabelVisibleTest';
import { labelPropTest } from '../../../../../tests/propTests/labelPropTest';
import { requiredPropTest } from '../../../../../tests/propTests/requiredPropTest';
import { validationStatePropTest } from '../../../../../tests/propTests/validationStatePropTest';
Expand Down Expand Up @@ -42,8 +42,8 @@ describe('rendering', () => {
expect(within(rootElement).getByText('validation text')).toHaveAttribute('id', 'id__validationText');
},
],
...isLabelVisible,
...labelPropTest,
...isLabelVisibleTest(),
...labelPropTest(),
...requiredPropTest,
...validationStatePropTest,
...validationTextPropTest,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { refPropTest } from '../../../../../tests/propTests/refPropTest';
import { fullWidthPropTest } from '../../../../../tests/propTests/fullWidthPropTest';
import { helpTextPropTest } from '../../../../../tests/propTests/helpTextPropTest';
import { formLayoutProviderTest } from '../../../../../tests/providerTests/formLayoutProviderTest';
import { isLabelVisible } from '../../../../../tests/propTests/isLabelVisible';
import { isLabelVisibleTest } from '../../../../../tests/propTests/isLabelVisibleTest';
import { labelPropTest } from '../../../../../tests/propTests/labelPropTest';
import { layoutPropTest } from '../../../../../tests/propTests/layoutPropTest';
import { requiredPropTest } from '../../../../../tests/propTests/requiredPropTest';
Expand Down Expand Up @@ -45,8 +45,8 @@ describe('rendering', () => {
expect(rootElement).toHaveAttribute('id', 'id__label');
},
],
...isLabelVisible,
...labelPropTest,
...isLabelVisibleTest(),
...labelPropTest(),
...layoutPropTest,
...requiredPropTest,
...validationStatePropTest,
Expand Down
2 changes: 1 addition & 1 deletion src/lib/components/FormLayout/README.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ This is a demo of all components supported by FormLayout.
value={fruit}
/>
<InputGroup label="Promo code">
<TextField label="Promo code" />
<TextField label="Code" />
<Button label="Apply" color="secondary" priority="outline" />
</InputGroup>
</FormLayout>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ describe('rendering', () => {
{ innerFieldSize: 'large' },
(rootElement) => expect(rootElement).toHaveClass('isRootSizeLarge'),
],
...labelPropTest,
...labelPropTest(),
[
{
label: 'label',
Expand Down
38 changes: 28 additions & 10 deletions src/lib/components/InputGroup/InputGroup.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import styles from './InputGroup.scss';

export const InputGroup = ({
children,
disabled,
id,
isLabelVisible,
label,
Expand All @@ -39,7 +40,7 @@ export const InputGroup = ({
);

return (
<div
<fieldset
{...transferProps(restProps)}
id={id}
className={classNames(
Expand All @@ -48,27 +49,36 @@ export const InputGroup = ({
resolveContextOrProp(formLayoutContext && formLayoutContext.layout, layout) === 'horizontal'
? styles.isRootLayoutHorizontal
: styles.isRootLayoutVertical,
disabled && styles.isRootDisabled,
getRootSizeClassName(size, styles),
getRootValidationStateClassName(validationState, styles),
)}
disabled={disabled}
>
<legend
className={styles.legend}
id={id && `${id}__label`}
>
{label}
</legend>
<div
aria-hidden
className={classNames(
styles.label,
!isLabelVisible && styles.isLabelHidden,
)}
id={id && `${id}__label`}
id={id && `${id}__displayLabel`}
>
{label}
</div>
<div className={styles.field}>
<div
className={styles.inputGroup}
id={id && `${id}__group`}
role="group"
>
<InputGroupContext.Provider
value={{
disabled,
layout,
size,
}}
Expand All @@ -77,24 +87,27 @@ export const InputGroup = ({
</InputGroupContext.Provider>
</div>
{validationTexts && (
<div
<ul
className={styles.validationText}
id={id && `${id}__validationTexts`}
>
{validationTexts.map((validationText) => (
<Text blockLevel key={validationText}>
{validationText}
</Text>
<li key={validationText}>
<Text blockLevel>
{validationText}
</Text>
</li>
))}
</div>
</ul>
)}
</div>
</div>
</fieldset>
);
};

InputGroup.defaultProps = {
children: null,
disabled: false,
id: undefined,
isLabelVisible: true,
layout: 'vertical',
Expand All @@ -112,12 +125,17 @@ InputGroup.propTypes = {
* If none are provided nothing is rendered.
*/
children: PropTypes.node,
/**
* If `true`, the whole input group with all nested inputs and buttons will be disabled.
*/
disabled: PropTypes.bool,
/**
* ID of the root HTML element.
*
* Also serves as base for ids of nested elements:
* * `<ID>__group`
* * `<ID>__label`
* * `<ID>__displayLabel`
* * `<ID>__group`
* * `<ID>__validationTexts`
*/
id: PropTypes.string,
Expand Down
27 changes: 26 additions & 1 deletion src/lib/components/InputGroup/InputGroup.scss
Original file line number Diff line number Diff line change
@@ -1,18 +1,29 @@
// 1. The class name is intentionally singular because it's targeted by other mixins too.
// 2. Use a block-level display mode to prevent extra white space below grouped inputs in Safari.
// 3. Prevent individual inputs from overlapping inside narrow containers.
// 4. Legends are tricky to style, let's use a `div` instead.
// https://developer.mozilla.org/en-US/docs/Web/HTML/Element/fieldset#styling_with_css

@use "../../styles/tools/form-fields/box-field-elements";
@use "../../styles/tools/form-fields/box-field-layout";
@use "../../styles/tools/form-fields/box-field-sizes";
@use "../../styles/tools/form-fields/foundation";
@use "../../styles/tools/form-fields/variants";
@use "../../styles/tools/accessibility";
@use "../../styles/tools/reset";
@use "theme";

.root {
@include box-field-elements.input-container();
@include foundation.root();
@include foundation.fieldset();
}

// 4.
.legend {
@include accessibility.hide-text();
}

// 4.
.label {
@include foundation.label();
}
Expand All @@ -26,6 +37,7 @@

// 1.
.validationText {
@include reset.list();
@include foundation.help-text();
}

Expand Down Expand Up @@ -65,3 +77,16 @@
.isRootInFormLayout {
@include box-field-layout.in-form-layout();
}

// Sizes
.isRootSizeSmall {
@include box-field-sizes.size(small, $has-input: false);
}

.isRootSizeMedium {
@include box-field-sizes.size(medium, $has-input: false);
}

.isRootSizeLarge {
@include box-field-sizes.size(large, $has-input: false);
}
18 changes: 18 additions & 0 deletions src/lib/components/InputGroup/README.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ See [API](#api) for all available options.
InputGroup is currently **not responsive: the inputs do not shrink nor wrap**.
Make sure your inputs fit their container, especially on small screens.

- In the background, InputGroup uses the [`fieldset`][fieldset] element. Not
only it improves the [accessibility] of the group, it also allows you to make
use of its built-in features like disabling all nested inputs or pairing the
group with a form outside. Consult [the MDN docs][fieldset] to learn more.

- InputGroup currently **supports grouping of**
[TextField](/components/text-field), [SelectField](/components/select-field),
and [Button](/components/button) components.
Expand Down Expand Up @@ -165,6 +170,17 @@ supports this kind of layout as well.

## States

### Disabled State

Disables all fields and buttons inside the group.

<Playground>
<InputGroup disabled label="Disabled group">
<TextField label="Label" />
<Button label="Submit" />
</InputGroup>
</Playground>

### Validation States

Validation states visually present the result of validation of the grouped
Expand Down Expand Up @@ -256,5 +272,7 @@ interactive and helps to improve its accessibility.
| `--rui-InputGroup__gap` | Gap between elements |
| `--rui-InputGroup__inner-border-radius` | Inner border radius of elements |

[fieldset]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/fieldset
[accessibility]: https://www.w3.org/WAI/tutorials/forms/grouping/
[React synthetic events]: https://reactjs.org/docs/events.html
[div]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/div#attributes
18 changes: 15 additions & 3 deletions src/lib/components/InputGroup/__tests__/InputGroup.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ import {
render,
within,
} from '@testing-library/react';
import { labelPropTest } from '../../../../../tests/propTests/labelPropTest';
import { Button } from '../../Button';
import { SelectField } from '../../SelectField';
import { TextField } from '../../TextField';
import { childrenEmptyPropTest } from '../../../../../tests/propTests/childrenEmptyPropTest';
import { isLabelVisible } from '../../../../../tests/propTests/isLabelVisible';
import { isLabelVisibleTest } from '../../../../../tests/propTests/isLabelVisibleTest';
import { layoutPropTest } from '../../../../../tests/propTests/layoutPropTest';
import { InputGroup } from '../InputGroup';

Expand Down Expand Up @@ -38,6 +39,15 @@ describe('rendering', () => {
expect(within(rootElement).getByRole('button')).toHaveClass('isRootInInputGroup');
},
],
[
{ disabled: true },
(rootElement) => {
expect(rootElement).toBeDisabled();
expect(within(rootElement).getByRole('textbox')).toBeDisabled();
expect(within(rootElement).getByRole('combobox')).toBeDisabled();
expect(within(rootElement).getByRole('button')).toBeDisabled();
},
],
[
{
id: 'id',
Expand All @@ -46,10 +56,12 @@ describe('rendering', () => {
(rootElement) => {
expect(rootElement).toHaveAttribute('id', 'id');
expect(within(rootElement).getByTestId('id__group'));
expect(within(rootElement).getByText('label')).toHaveAttribute('id', 'id__label');
expect(within(rootElement).getByTestId('id__label'));
expect(within(rootElement).getByTestId('id__displayLabel'));
},
],
...isLabelVisible,
...isLabelVisibleTest('legend'),
...labelPropTest('legend'),
...layoutPropTest,
[
{ size: 'small' },
Expand Down
Loading