Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-dropdown]Added aria-haspoup and aria-controls attribute #3936

Merged
merged 32 commits into from
Oct 31, 2023
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
caadb3e
Added aria-properties
Oct 13, 2023
536f8c8
addde changelog
Oct 13, 2023
3044177
changed id to fix wdio issues
Oct 16, 2023
4bab927
snap update
Oct 16, 2023
0f71abe
id updated
Oct 16, 2023
fcb7910
id updated
Oct 16, 2023
08b4966
id updated
Oct 16, 2023
20141c9
id updated
Oct 16, 2023
62206f0
id updated
Oct 16, 2023
5ed5690
updated snap files
Oct 16, 2023
b766425
snap update
Oct 16, 2023
9a8b867
snap update
Oct 17, 2023
a81aa81
snap update
Oct 17, 2023
1172c73
snap update
Oct 17, 2023
fa63590
snap update
Oct 17, 2023
9e8e1bf
snap update
Oct 17, 2023
21ad9ba
snap update
Oct 17, 2023
1bd9327
snap update
Oct 17, 2023
468dbd3
Review Comments Implemented
Oct 19, 2023
0a11b55
Review Comments Implemented
Oct 19, 2023
3344405
Merge branch 'main' of https://github.com/cerner/terra-core into drop…
Oct 19, 2023
e68b94f
Review Comments Implemented
Oct 19, 2023
ea5ae0d
Review Comments Implemented
Oct 19, 2023
f8779db
Merge branch 'main' of https://github.com/cerner/terra-core into drop…
Oct 26, 2023
7cb2fd3
updated as per review comments
Oct 26, 2023
3768685
updated as per review comments
Oct 26, 2023
d5e7bb7
updated as per review comments
Oct 26, 2023
0d7aa0d
added custom Props.classname for dropdown button
Oct 26, 2023
fb23b80
Merge branch 'main' of https://github.com/cerner/terra-core into drop…
Oct 30, 2023
d029e14
Merge branch 'dropdown-aria-property' of https://github.com/cerner/te…
Oct 30, 2023
9c44d50
Merge branch 'main' of https://github.com/cerner/terra-core into drop…
Oct 30, 2023
5ac876d
Merge branch 'main' of https://github.com/cerner/terra-core into drop…
Oct 31, 2023
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
1 change: 1 addition & 0 deletions packages/terra-core-docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

* Updated
* Updated `terra-alert` documentation for custom titles.
* Updated `terra-dropdown-button` documentation for testing uuid details.
* Updated exisiting examples upto a11y standards

* Fixed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ This component requires the following peer dependencies be installed in your app

<!-- AUTO-GENERATED-CONTENT:END -->

## Implementation Notes:
The Form-Field component must be composed inside the [Base][1] component with a locale in order for it to load the correct translation strings.

[1]: https://engineering.cerner.com/terra-ui/application/terra-application/components/application-base

## Usage

```jsx
Expand All @@ -62,3 +67,26 @@ import DropdownButton, { Item } from 'terra-dropdown-button';

## Item Props
<ItemPropsTable />

## Testing

Terra Dropdown button uses `uuid` which changes the component's description id dynamically. To mock the return value with the Jest testing library, `jest.spyOn` can be used.

If Enzyme `shallow` is being used for the tests then the mock may not be required depending on the depth of the returned wrapper. However, if `mount` is used then `uuid` should be mocked as shown below:

```js
import { v4 as uuidv4 } from 'uuid';

let mockSpyUuid;

// using a variable may result in failures. For best results, mock return value.
beforeAll(() => {
mockSpyUuid = jest.spyOn(uuidv4, 'v4').mockReturnValue('00000000-0000-0000-0000-000000000000');
});

// restore the mock
afterAll(() => {
mockSpyUuid.mockRestore();
});

```
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ Children must be the `Item` subcomponent for proper functionality and appearance
## Implementation Notes:
The SplitButton component must be composed inside the [Base][1] component with a locale in order for it to load the correct translation strings.

[1]: https://engineering.cerner.com/terra-core/components/terra-base/base/base
[1]: https://engineering.cerner.com/terra-ui/application/terra-application/components/application-base

## Usage

Expand All @@ -51,4 +51,27 @@ import { Item, SplitButton } from 'terra-dropdown-button';
<SplitButtonPropsTable />

## Item Props
<ItemPropsTable />
<ItemPropsTable />

## Testing

Terra Split Button uses `uuid` which changes the component's description id dynamically. To mock the return value with the Jest testing library, `jest.spyOn` can be used.

If Enzyme `shallow` is being used for the tests then the mock may not be required depending on the depth of the returned wrapper. However, if `mount` is used then `uuid` should be mocked as shown below:

```js
import { v4 as uuidv4 } from 'uuid';

let mockSpyUuid;

// using a variable may result in failures. For best results, mock return value.
beforeAll(() => {
mockSpyUuid = jest.spyOn(uuidv4, 'v4').mockReturnValue('00000000-0000-0000-0000-000000000000');
});

// restore the mock
afterAll(() => {
mockSpyUuid.mockRestore();
});

```
2 changes: 2 additions & 0 deletions packages/terra-dropdown-button/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# Changelog

## Unreleased
* Added
* Added 'aria-haspoup' and 'aria-controls' attributes for dropdown button.

## 1.36.0 - (August 8, 2023)

Expand Down
3 changes: 2 additions & 1 deletion packages/terra-dropdown-button/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@
"prop-types": "^15.5.8",
"terra-hookshot": "^5.0.0",
"terra-mixins": "^1.40.0",
"terra-theme-context": "^1.0.0"
"terra-theme-context": "^1.0.0",
"uuid": "3.4.0"
},
"scripts": {
"compile": "babel --root-mode upward src --out-dir lib --copy-files",
Expand Down
15 changes: 15 additions & 0 deletions packages/terra-dropdown-button/src/DropdownButton.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import PropTypes from 'prop-types';
import classNamesBind from 'classnames/bind';
import ThemeContext from 'terra-theme-context';
import * as KeyCode from 'keycode-js';
// eslint-disable-next-line import/no-extraneous-dependencies
import { v4 as uuidv4 } from 'uuid';
import { injectIntl } from 'react-intl';
import DropdownButtonBase from './_DropdownButtonBase';
import styles from './DropdownButton.module.scss';
Expand Down Expand Up @@ -185,6 +187,14 @@ class DropdownButton extends React.Component {
const customLabel = (selectText) ? `${selectText}, ${selectedLabel}, ${label}` : label;
buttonAriaLabel = `${customLabel}${buttonAriaLabel ? `, ${buttonAriaLabel}` : ''}`;

const dropDownMenuId = uuidv4();
const dropDownMenuListId = `dropdown-menu-list-${dropDownMenuId}`;
let dropDownMenuButtonId = `dropdown-menu-button-${dropDownMenuId}`;
if (modifiedButtonAttrs && modifiedButtonAttrs.id) {
dropDownMenuButtonId = modifiedButtonAttrs.id;
delete modifiedButtonAttrs.id;
}

return (
<DropdownButtonBase
{...customProps}
Expand All @@ -197,6 +207,8 @@ class DropdownButton extends React.Component {
refCallback={this.setListNode}
buttonRef={this.getButtonNode}
getSelectedOptionText={this.getSelectedOptionText}
menuId={dropDownMenuListId}
buttonId={dropDownMenuButtonId}
>
<button
{...modifiedButtonAttrs}
Expand All @@ -211,6 +223,9 @@ class DropdownButton extends React.Component {
ref={this.setButtonNode}
aria-expanded={isOpen}
aria-label={buttonAriaLabel}
aria-haspopup="true"
id={dropDownMenuButtonId}
aria-controls={dropDownMenuListId}
onBlur={this.handleBlur}
>
<span className={cx('dropdown-button-text')}>{label}</span>
Expand Down
5 changes: 4 additions & 1 deletion packages/terra-dropdown-button/src/_Dropdown.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,10 @@ const propTypes = {
};

const Dropdown = ({
requestClose, isOpen, targetRef, children, width, refCallback, buttonRef, getSelectedOptionText,
requestClose, isOpen, targetRef, children, width, refCallback, buttonRef, getSelectedOptionText, ...customProps
}) => {
const buttonFocused = useRef(false);
const { menuId, buttonId } = customProps;
useEffect(() => {
// added this change to bring focus back to button when dropdown list is closed.
if (buttonFocused.current && buttonRef) {
Expand Down Expand Up @@ -76,6 +77,8 @@ const Dropdown = ({
width={width}
refCallback={refCallback}
getSelectedOptionText={getSelectedOptionText}
menuId={menuId}
buttonId={buttonId}
>
{children}
</DropdownList>
Expand Down
2 changes: 2 additions & 0 deletions packages/terra-dropdown-button/src/_DropdownButtonBase.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ class DropdownButtonBase extends React.Component {
buttonRef={buttonRef}
refCallback={refCallback}
getSelectedOptionText={getSelectedOptionText}
menuId={customProps.menuId}
buttonId={customProps.buttonId}
>
{items}
</Dropdown>
Expand Down
3 changes: 3 additions & 0 deletions packages/terra-dropdown-button/src/_DropdownList.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ class DropdownList extends React.Component {
const {
width,
refCallback,
...customProps
} = this.props;

const theme = this.context;
Expand All @@ -225,6 +226,8 @@ class DropdownList extends React.Component {
onKeyDown={this.handleKeyDown}
onKeyUp={this.handleKeyUp}
role="menu"
id={customProps.menuId}
aria-labelledby={customProps.buttonId}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need aria-LabelledBy for ul menu ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As suggested by UX team and provided the reference URL to add aria-labelledby for dropdown menu

https://www.w3.org/WAI/ARIA/apg/patterns/menu-button/examples/menu-button-links/

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will result in announcing text of button on navigating of each item which is not expected. So I would double check on using this attribute on ul menu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a look on the button navigation and it is not announcing text of button on menu change, it is reading as 'menu label (1 of 4) menu item' please validate at your end as well if required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen.Recording.2023-10-20.at.12.10.45.PM.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

you can notice the reported behavior on the screen recording that VO did announced button text as menu Export, Deafult dropdown which is not correct for the first dropdown item

Copy link
Contributor Author

@AH106586Harika AH106586Harika Oct 26, 2023

Choose a reason for hiding this comment

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

Removed aria-labelledby property as discussed.

>
{this.cloneChildren()}
</ul>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,21 @@ import ThemeContextProvider from 'terra-theme-context/lib/ThemeContextProvider';
import { IntlProvider } from 'react-intl';
/* eslint-disable-next-line import/no-extraneous-dependencies */
import { shallowWithIntl, mountWithIntl } from 'terra-enzyme-intl';
// eslint-disable-next-line import/no-extraneous-dependencies
import { v4 as uuidv4 } from 'uuid';
import translationsFile from '../../translations/en.json';
/* eslint-disable-next-line import/no-extraneous-dependencies */
import DropdownButton, { Item, Variants } from '../../src/DropdownButton';

let mockSpyUuid;
beforeAll(() => {
mockSpyUuid = jest.spyOn(uuidv4, 'v4').mockReturnValue('00000000-0000-0000-0000-000000000000');
});

afterAll(() => {
mockSpyUuid.mockRestore();
Comment on lines +13 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to add similar spy for splitbutton test as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

});

describe('Dropdown Button', () => {
it('should render a default dropdown type', () => {
const wrapper = shallowWithIntl(
Expand Down Expand Up @@ -87,11 +98,13 @@ describe('Dropdown Button', () => {
it('correctly applies the theme context className', () => {
const wrapper = mountWithIntl(
<ThemeContextProvider theme={{ className: 'orion-fusion-theme' }}>
<DropdownButton label="Primary Option">
<DropdownButton label="Primary Option" id="dropDown">
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have jest test to ensure button is assigned with value passed by consumer as ID instead of the auto generated ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

<Item label="1st Option" onSelect={() => {}} />
</DropdownButton>
</ThemeContextProvider>,
);
const dropDownButtonId = wrapper.find('#dropDown');
expect(dropDownButtonId.exists()).toBe(true);
expect(wrapper).toMatchSnapshot();
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@ exports[`Dropdown Button correctly applies the theme context className 1`] = `
}
>
<InjectIntl(DropdownButton)
id="dropDown"
label="Primary Option"
>
<DropdownButton
buttonAttrs={Object {}}
id="dropDown"
intl={
Object {
"defaultFormats": Object {},
Expand Down Expand Up @@ -74,8 +76,10 @@ exports[`Dropdown Button correctly applies the theme context className 1`] = `
variant="neutral"
>
<DropdownButtonBase
buttonId="dropdown-menu-button-00000000-0000-0000-0000-000000000000"
buttonRef={[Function]}
getSelectedOptionText={[Function]}
id="dropDown"
isBlock={false}
isCompact={false}
isDisabled={false}
Expand All @@ -86,18 +90,25 @@ exports[`Dropdown Button correctly applies the theme context className 1`] = `
onSelect={[Function]}
/>
}
menuId="dropdown-menu-list-00000000-0000-0000-0000-000000000000"
refCallback={[Function]}
requestClose={[Function]}
>
<div
buttonId="dropdown-menu-button-00000000-0000-0000-0000-000000000000"
className="dropdown-button-base orion-fusion-theme"
id="dropDown"
menuId="dropdown-menu-list-00000000-0000-0000-0000-000000000000"
>
<button
aria-controls="dropdown-menu-list-00000000-0000-0000-0000-000000000000"
aria-disabled={false}
aria-expanded={false}
aria-haspopup="true"
aria-label="Primary Option"
className="dropdown-button neutral orion-fusion-theme"
disabled={false}
id="dropdown-menu-button-00000000-0000-0000-0000-000000000000"
onBlur={[Function]}
onClick={[Function]}
onKeyDown={[Function]}
Expand All @@ -114,9 +125,11 @@ exports[`Dropdown Button correctly applies the theme context className 1`] = `
/>
</button>
<Dropdown
buttonId="dropdown-menu-button-00000000-0000-0000-0000-000000000000"
buttonRef={[Function]}
getSelectedOptionText={[Function]}
isOpen={false}
menuId="dropdown-menu-list-00000000-0000-0000-0000-000000000000"
refCallback={[Function]}
requestClose={[Function]}
targetRef={[Function]}
Expand Down
Loading