Skip to content

Commit

Permalink
feat(ListBox): fix labels of trigger buttons (#4820)
Browse files Browse the repository at this point in the history
This change fixes an DAP error in `<ListBox.Field>` that happens
because its `aria-label` not matching its text content. DAP takes
`aria-labelledby` as a preferred label content (over text content or
`aria-label`), and thus this change add `aria-labelledby` to
`<ListBox.Field>`.

The original `aria-label` content, which is for lettting user know that
the UI can open/close menu, is replaced with a browser's native
approach to tell assistive technology that the button is a pop-up
button and let screen reader announce as such.

This change also changes `<label>` to `<span>` for dropdown and
non-filterable multi select given there is no form control (`<input>`)
associated with it.

Fixes #4531.
  • Loading branch information
asudoh authored Jan 9, 2020
1 parent 9c1a4ee commit f87a204
Show file tree
Hide file tree
Showing 12 changed files with 158 additions and 133 deletions.
13 changes: 10 additions & 3 deletions packages/react/src/components/ComboBox/ComboBox.js
Original file line number Diff line number Diff line change
Expand Up @@ -280,16 +280,22 @@ export default class ComboBox extends React.Component {
const titleClasses = cx(`${prefix}--label`, {
[`${prefix}--label--disabled`]: disabled,
});
const comboBoxHelperId = !helperText
? undefined
: `combobox-helper-text-${this.comboBoxInstanceId}`;
const comboBoxLabelId = `combobox-label-${this.comboBoxInstanceId}`;
const title = titleText ? (
<label htmlFor={id} className={titleClasses}>
<label id={comboBoxLabelId} htmlFor={id} className={titleClasses}>
{titleText}
</label>
) : null;
const helperClasses = cx(`${prefix}--form__helper-text`, {
[`${prefix}--form__helper-text--disabled`]: disabled,
});
const helper = helperText ? (
<div className={helperClasses}>{helperText}</div>
<div id={comboBoxHelperId} className={helperClasses}>
{helperText}
</div>
) : null;
const wrapperClasses = cx(`${prefix}--list-box__wrapper`);
const comboBoxA11yId = `combobox-a11y-${this.comboBoxInstanceId}`;
Expand Down Expand Up @@ -335,7 +341,8 @@ export default class ComboBox extends React.Component {
<ListBox.Field
id={id}
disabled={disabled}
translateWithId={translateWithId}
aria-labelledby={comboBoxLabelId}
aria-describedby={comboBoxHelperId}
{...getButtonProps({
disabled,
onClick: this.onToggleClick(isOpen),
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/components/Dropdown/Dropdown-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ describe('Dropdown', () => {

beforeEach(() => {
wrapper = mount(<Dropdown titleText="Email Input" {...mockProps} />);
renderedLabel = wrapper.find('label');
renderedLabel = wrapper.find('span[className="bx--label"]');
});

it('renders a title', () => {
Expand Down
143 changes: 78 additions & 65 deletions packages/react/src/components/Dropdown/Dropdown.js
Original file line number Diff line number Diff line change
Expand Up @@ -190,16 +190,21 @@ export default class Dropdown extends React.Component {
[`${prefix}--label--disabled`]: disabled,
});

const helperId =
!id || !helperText ? undefined : `dropdown-helper-text-${id}`;
const labelId = `dropdown-label-${id}`;
const fieldLabelId = `dropdown-field-label-${id}`;

const title = titleText ? (
<label htmlFor={id} className={titleClasses}>
{titleText}
</label>
<span className={titleClasses}>{titleText}</span>
) : null;
const helperClasses = cx(`${prefix}--form__helper-text`, {
[`${prefix}--form__helper-text--disabled`]: disabled,
});
const helper = helperText ? (
<div className={helperClasses}>{helperText}</div>
<div id={helperId} className={helperClasses}>
{helperText}
</div>
) : null;
const wrapperClasses = cx(
`${prefix}--dropdown__wrapper`,
Expand Down Expand Up @@ -234,69 +239,77 @@ export default class Dropdown extends React.Component {
getItemProps,
getLabelProps,
toggleMenu,
}) => (
<ListBox
type={type}
size={size}
id={id}
aria-label={ariaLabel}
className={className({ isOpen })}
disabled={disabled}
isOpen={isOpen}
invalid={invalid}
invalidText={invalidText}
light={light}
{...getRootProps({ refKey: 'innerRef' })}>
{invalid && (
<WarningFilled16
className={`${prefix}--list-box__invalid-icon`}
/>
)}
<ListBox.Field
}) => {
const buttonProps = {
...getButtonProps({
onKeyDown: event => {
if (match(event, keys.Enter)) {
toggleMenu();
}
},
disabled,
}),
'aria-label': undefined,
};
return (
<ListBox
type={type}
size={size}
id={id}
tabIndex="0"
aria-label={ariaLabel}
className={className({ isOpen })}
disabled={disabled}
aria-disabled={disabled}
translateWithId={translateWithId}
{...getButtonProps({
onKeyDown: event => {
if (match(event, keys.Enter)) {
toggleMenu();
}
},
disabled,
})}>
<span
className={`${prefix}--list-box__label`}
{...getLabelProps()}>
{selectedItem ? itemToString(selectedItem) : label}
</span>
<ListBox.MenuIcon
isOpen={isOpen}
translateWithId={translateWithId}
/>
</ListBox.Field>
{isOpen && (
<ListBox.Menu aria-labelledby={id} id={id}>
{items.map((item, index) => (
<ListBox.MenuItem
key={itemToString(item)}
isActive={selectedItem === item}
isHighlighted={
highlightedIndex === index || selectedItem === item
}
{...getItemProps({ item, index })}>
{itemToElement ? (
<ItemToElement key={itemToString(item)} {...item} />
) : (
itemToString(item)
)}
</ListBox.MenuItem>
))}
</ListBox.Menu>
)}
</ListBox>
)}
isOpen={isOpen}
invalid={invalid}
invalidText={invalidText}
light={light}
{...getRootProps({ refKey: 'innerRef' })}>
{invalid && (
<WarningFilled16
className={`${prefix}--list-box__invalid-icon`}
/>
)}
<ListBox.Field
id={id}
tabIndex="0"
disabled={disabled}
aria-disabled={disabled}
aria-labelledby={`${labelId} ${fieldLabelId}`}
aria-describedby={helperId}
{...buttonProps}>
<span
id={fieldLabelId}
className={`${prefix}--list-box__label`}
{...getLabelProps()}>
{selectedItem ? itemToString(selectedItem) : label}
</span>
<ListBox.MenuIcon
isOpen={isOpen}
translateWithId={translateWithId}
/>
</ListBox.Field>
{isOpen && (
<ListBox.Menu aria-labelledby={id} id={id}>
{items.map((item, index) => (
<ListBox.MenuItem
key={itemToString(item)}
isActive={selectedItem === item}
isHighlighted={
highlightedIndex === index || selectedItem === item
}
{...getItemProps({ item, index })}>
{itemToElement ? (
<ItemToElement key={itemToString(item)} {...item} />
) : (
itemToString(item)
)}
</ListBox.MenuItem>
))}
</ListBox.Menu>
)}
</ListBox>
);
}}
</Downshift>
</div>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ exports[`Dropdown should render 1`] = `
aria-disabled={false}
aria-expanded={false}
aria-haspopup={true}
aria-label="open menu"
aria-labelledby="dropdown-label-test-dropdown dropdown-field-label-test-dropdown"
data-toggle={true}
disabled={false}
id="test-dropdown"
Expand All @@ -94,15 +94,14 @@ exports[`Dropdown should render 1`] = `
onKeyDown={[Function]}
role="button"
tabIndex="0"
translateWithId={[Function]}
type="button"
>
<div
aria-controls={null}
aria-disabled={false}
aria-expanded={false}
aria-haspopup={true}
aria-label="Open menu"
aria-labelledby="dropdown-label-test-dropdown dropdown-field-label-test-dropdown"
aria-owns={null}
className="bx--list-box__field"
data-toggle={true}
Expand All @@ -116,6 +115,7 @@ exports[`Dropdown should render 1`] = `
<span
className="bx--list-box__label"
htmlFor="downshift-0-input"
id="dropdown-field-label-test-dropdown"
>
input
</span>
Expand Down Expand Up @@ -260,7 +260,7 @@ exports[`Dropdown should render custom item components 1`] = `
aria-disabled={false}
aria-expanded={true}
aria-haspopup={true}
aria-label="close menu"
aria-labelledby="dropdown-label-test-dropdown dropdown-field-label-test-dropdown"
data-toggle={true}
disabled={false}
id="test-dropdown"
Expand All @@ -269,15 +269,14 @@ exports[`Dropdown should render custom item components 1`] = `
onKeyDown={[Function]}
role="button"
tabIndex="0"
translateWithId={[Function]}
type="button"
>
<div
aria-controls="test-dropdown__menu"
aria-disabled={false}
aria-expanded={true}
aria-haspopup={true}
aria-label="Close menu"
aria-labelledby="dropdown-label-test-dropdown dropdown-field-label-test-dropdown"
aria-owns="test-dropdown__menu"
className="bx--list-box__field"
data-toggle={true}
Expand All @@ -291,6 +290,7 @@ exports[`Dropdown should render custom item components 1`] = `
<span
className="bx--list-box__label"
htmlFor="downshift-4-input"
id="dropdown-field-label-test-dropdown"
>
input
</span>
Expand Down Expand Up @@ -594,7 +594,7 @@ exports[`Dropdown should render with strings as items 1`] = `
aria-disabled={false}
aria-expanded={true}
aria-haspopup={true}
aria-label="close menu"
aria-labelledby="dropdown-label-test-dropdown dropdown-field-label-test-dropdown"
data-toggle={true}
disabled={false}
id="test-dropdown"
Expand All @@ -603,15 +603,14 @@ exports[`Dropdown should render with strings as items 1`] = `
onKeyDown={[Function]}
role="button"
tabIndex="0"
translateWithId={[Function]}
type="button"
>
<div
aria-controls="test-dropdown__menu"
aria-disabled={false}
aria-expanded={true}
aria-haspopup={true}
aria-label="Close menu"
aria-labelledby="dropdown-label-test-dropdown dropdown-field-label-test-dropdown"
aria-owns="test-dropdown__menu"
className="bx--list-box__field"
data-toggle={true}
Expand All @@ -625,6 +624,7 @@ exports[`Dropdown should render with strings as items 1`] = `
<span
className="bx--list-box__label"
htmlFor="downshift-3-input"
id="dropdown-field-label-test-dropdown"
>
input
</span>
Expand Down
34 changes: 4 additions & 30 deletions packages/react/src/components/ListBox/ListBoxField.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,37 +11,22 @@ import PropTypes from 'prop-types';

const { prefix } = settings;

export const translationIds = {
'close.menu': 'close.menu',
'open.menu': 'open.menu',
};

const defaultTranslations = {
[translationIds['close.menu']]: 'Close menu',
[translationIds['open.menu']]: 'Open menu',
};
export const translationIds = {}; // No longer used, left export for backward-compatibility

/**
* `ListBoxField` is responsible for creating the containing node for valid
* elements inside of a field. It also provides a11y-related attributes like
* `role` to make sure a user can focus the given field.
*/
const ListBoxField = ({
children,
id,
disabled,
tabIndex,
translateWithId: t,
...rest
}) => (
const ListBoxField = ({ children, id, disabled, tabIndex, ...rest }) => (
<div
role={rest['aria-expanded'] ? 'combobox' : rest.role || 'combobox'}
aria-owns={(rest['aria-expanded'] && `${id}__menu`) || null}
aria-controls={(rest['aria-expanded'] && `${id}__menu`) || null}
aria-haspopup="listbox"
className={`${prefix}--list-box__field`}
tabIndex={(!disabled && tabIndex) || -1}
{...rest}
aria-label={rest['aria-expanded'] ? t('close.menu') : t('open.menu')}>
{...rest}>
{children}
</div>
);
Expand All @@ -66,17 +51,6 @@ ListBoxField.propTypes = {
* Optional prop to specify the tabIndex of the <ListBox> trigger button
*/
tabIndex: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),

/**
* i18n hook used to provide the appropriate description for the given menu
* icon. This function takes in an id defined in `translationIds` and should
* return a string message for that given message id.
*/
translateWithId: PropTypes.func.isRequired,
};

ListBoxField.defaultProps = {
translateWithId: id => defaultTranslations[id],
};

export default ListBoxField;
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ exports[`ListBox should render 1`] = `
tabindex="-1"
>
<div
aria-label="Open menu"
aria-haspopup="listbox"
class="bx--list-box__field"
role="combobox"
tabindex="-1"
Expand Down Expand Up @@ -44,11 +44,10 @@ exports[`ListBox should render 1`] = `
>
<ListBoxField
id="test-listbox"
translateWithId={[Function]}
>
<div
aria-controls={null}
aria-label="Open menu"
aria-haspopup="listbox"
aria-owns={null}
className="bx--list-box__field"
role="combobox"
Expand Down
Loading

0 comments on commit f87a204

Please sign in to comment.