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

[Terra-Dropdown] VO does not announce Expanded state and Index of list items while navigating through arrow keys #3805

Merged
merged 44 commits into from
Jul 17, 2023
Merged

[Terra-Dropdown] VO does not announce Expanded state and Index of list items while navigating through arrow keys #3805

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
3c92a1a
Added aria-label to announce index of dropdown listitems
May 19, 2023
68f588e
Resolved merge conflicts
May 23, 2023
bc7b3f2
clear aria-label while dropdown opens
May 24, 2023
43f300d
added aria-haspopup
May 24, 2023
22eeb9b
updated jest testcases
May 26, 2023
6808c17
Merge branch 'main' of https://github.com/cerner/terra-core into AH10…
Jun 2, 2023
6430ad4
Added aria-label for expand/collapse state
Jun 2, 2023
1adf2f2
removes sticky focus from button on safari on VO mode.
Jun 2, 2023
2731c94
Default Dropdown Button
Jun 6, 2023
452467c
Implemented review comments
Jun 6, 2023
2ca0d2c
Update: voice over and focus fix for mac
MadanKumarGovindaswamy Jun 12, 2023
8c91883
Merge branch 'main' of https://github.com/cerner/terra-core into AH10…
Jun 12, 2023
c01ce54
Merge branch 'AH106586/Dropdown-announce-index' of https://github.com…
Jun 12, 2023
f81a8b5
Update: keyboard focus fix on edge browser
MadanKumarGovindaswamy Jun 12, 2023
dd64555
Removed openedViaKeyboard and buttonRef props
Jun 14, 2023
05892e7
Updated WDIO spec files
Jun 14, 2023
82755a4
Updated WDIO spec files
Jun 15, 2023
e77e019
updated Changelog
Jun 15, 2023
0b6bada
WDIO snapshots update
Jun 15, 2023
34742e0
WDIO Snapshots Update
Jun 16, 2023
c193201
Implemented review comments
Jun 19, 2023
cc6fbef
Merge branch 'main' of https://github.com/cerner/terra-core into AH10…
Jun 19, 2023
db727b8
Merge branches 'AH106586/Dropdown-announce-index' and 'AH106586/Dropd…
Jun 19, 2023
e90e80c
Updated translations and added testcase
Jun 19, 2023
7f9273b
Updating WDIO Snapshots
Jun 20, 2023
c9b83aa
Removed setButtonNode
Jun 20, 2023
bc318e2
Merge branch 'main' of https://github.com/cerner/terra-core into AH10…
Jun 28, 2023
120d514
Added translations
Jun 28, 2023
898507c
modified test-case
Jun 28, 2023
943af57
Update: Fix Focus issue when dropdown is closed
MadanKumarGovindaswamy Jul 7, 2023
5c14beb
Update: lint error
MadanKumarGovindaswamy Jul 7, 2023
9e77a10
Update: Added back buttonRef Prop
MadanKumarGovindaswamy Jul 7, 2023
123b00a
Update: handle outside click as a separate function
MadanKumarGovindaswamy Jul 10, 2023
ea0230c
Update: adding test case for dropdown outside click
MadanKumarGovindaswamy Jul 11, 2023
2f1374e
update: test case for dropdown focus
MadanKumarGovindaswamy Jul 11, 2023
16de4db
Merge branch 'main' of https://github.com/cerner/terra-core into AH10…
Jul 13, 2023
8efdc84
Merge branch 'AH106586/Dropdown-announce-index' of https://github.com…
Jul 13, 2023
df5c65e
updated testcase as per review comments
Jul 13, 2023
8ef8326
modified as per review comments
Jul 14, 2023
7430f42
Merge branch 'main' of https://github.com/cerner/terra-core into AH10…
Jul 14, 2023
bb91545
Removed Dot
Jul 14, 2023
275521f
Removed Dot for selected text and Added activeOption for de locale
Jul 17, 2023
5b8ffd3
modified fi-fi locale
Jul 17, 2023
3e7cc7e
worked on review comments
Jul 17, 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
3 changes: 3 additions & 0 deletions packages/terra-dropdown-button/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

* Added
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Added
* Added

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there are 2 Added headings and they can be combined together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

* Added screenreader support to announce index and expand/collapse state for dropdown list items.

* Added
* Added focus styles for dropdown list items.

Expand Down
2 changes: 1 addition & 1 deletion packages/terra-dropdown-button/src/DropdownButton.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ class DropdownButton extends React.Component {
this.setState({ openedViaKeyboard: false });
}
this.toggleDropDown(event);
this.setState({ selectText: '' });
}

handleDropdownRequestClose(callback) {
Expand Down Expand Up @@ -206,7 +207,6 @@ class DropdownButton extends React.Component {
tabIndex={isDisabled ? '-1' : undefined}
aria-disabled={isDisabled}
aria-expanded={isOpen}
aria-haspopup="menu"
ref={this.setButtonNode}
aria-label={selectText ? `${selectText}, ${selectedLabel}` : ''}
onBlur={this.handleBlur}
Expand Down
12 changes: 2 additions & 10 deletions packages/terra-dropdown-button/src/_Dropdown.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,6 @@ const propTypes = {
* Width to set the dropdown to. Used when `isBlock` is true. Unset means to autosize.
*/
width: PropTypes.string,
/**
* Whether or not dropdown is opened using keyboard.
*/
openedViaKeyboard: PropTypes.bool,
/**
* Callback for reference of the dropdown button
*/
buttonRef: PropTypes.func,
sdadn marked this conversation as resolved.
Show resolved Hide resolved
/**
* Ref callback for the dropdown list DOM element.
*/
Expand All @@ -45,7 +37,7 @@ const propTypes = {
};

const Dropdown = ({
requestClose, isOpen, targetRef, children, width, openedViaKeyboard, buttonRef, refCallback, getSelectedOptionText,
requestClose, isOpen, targetRef, children, width, refCallback, getSelectedOptionText,
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 you need to find openedViaKeyboard and buttonRef references and remove them from dropdown button as well as Split button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented as suggested.

}) => (
<Hookshot
isOpen={isOpen}
Expand All @@ -59,7 +51,7 @@ const Dropdown = ({
onEsc={requestClose}
onOutsideClick={requestClose}
>
<FocusTrap focusTrapOptions={{ returnFocusOnDeactivate: true, initialFocus: openedViaKeyboard ? '' : buttonRef, clickOutsideDeactivates: true }}>
<FocusTrap focusTrapOptions={{ returnFocusOnDeactivate: true, clickOutsideDeactivates: true }}>
<DropdownList
requestClose={requestClose}
width={width}
Expand Down
35 changes: 29 additions & 6 deletions packages/terra-dropdown-button/src/_DropdownList.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
import classNamesBind from 'classnames/bind';
import ThemeContext from 'terra-theme-context';
import * as KeyCode from 'keycode-js';
import { injectIntl } from 'react-intl';
import Util from './_DropdownListUtil';
import styles from './_DropdownList.module.scss';

Expand Down Expand Up @@ -30,6 +31,12 @@ const propTypes = {
* Callback for the dropdown list selected option.
*/
getSelectedOptionText: PropTypes.func,
/**
* @private
* The intl object containing translations. This is retrieved from the context automatically by injectIntl.
*/
intl: PropTypes.shape({ formatMessage: PropTypes.func }).isRequired,

};

class DropdownList extends React.Component {
Expand All @@ -48,6 +55,7 @@ class DropdownList extends React.Component {
this.searchString = '';
this.pressed = false;
this.listRef = null;
this.expanded = this.props.intl.formatMessage({ id: 'Terra.dropdownButton.expanded' });
}

handleKeyDown(event) {
Expand All @@ -69,6 +77,7 @@ class DropdownList extends React.Component {
event.preventDefault();
} else if (keyCode === KeyCode.KEY_DOWN) {
if (!this.pressed) {
this.expanded = '';
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 add a unit test to see if this gets set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented.

if (index === Util.getChildArray(this).length - 1) {
this.changeFocusState(0);
} else {
Expand All @@ -78,6 +87,7 @@ class DropdownList extends React.Component {
event.preventDefault();
} else if (keyCode === KeyCode.KEY_UP) {
if (!this.pressed) {
this.expanded = '';
if (index === 0) {
this.changeFocusState(Util.getChildArray(this).length - 1);
} else {
Expand Down Expand Up @@ -160,11 +170,24 @@ class DropdownList extends React.Component {
* @return {Array<React.ReactNode>} the array of children
*/
cloneChildren() {
return React.Children.map(this.props.children, (child, index) => React.cloneElement(child, {
isActive: index === this.state.active,
requestClose: this.props.requestClose,
'data-terra-dropdown-list-item': true,
}));
return React.Children.map(this.props.children, (child, index) => {
const currentItemLabel = this.props.children[index]?.props.label;
const currentIndex = index + 1;
const ofText = this.props.intl.formatMessage({ id: 'Terra.dropdownButton.of' });
const totalItems = this.props.children.length;
let ariaLabel = null;
if (currentIndex === 1 && totalItems) {
ariaLabel = `${this.expanded}${currentItemLabel},(${currentIndex} ${ofText} ${totalItems})`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of building the phrase ourselves, we should make this a single string so that it can be translated within the context of the usage. e.g.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented as suggested Thanks.

} else if (currentIndex !== 1 && totalItems) {
ariaLabel = `${currentItemLabel},(${currentIndex} ${ofText} ${totalItems})`;
}
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 simplify this :

const activeOption = (SharedUtil.isMac()) ? this.props.intl.formatMessage({ id: 'Terra.dropdownButton.activeOption' }, { currentItemLabel, currentIndex, totalItems }) : currentItemLabel;
      let ariaLabel = '';
     if (currentIndex === 1 && totalItems) {
        ariaLabel = `${this.expanded}, ${activeOption}`;
      } else if (currentIndex !== 1 && totalItems) {
        ariaLabel = activeOption;
      }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified for better readability.

return React.cloneElement(child, {
isActive: index === this.state.active,
requestClose: this.props.requestClose,
'data-terra-dropdown-list-item': true,
'aria-label': ariaLabel,
});
});
}

/* eslint-disable jsx-a11y/no-noninteractive-element-interactions */
Expand Down Expand Up @@ -201,4 +224,4 @@ class DropdownList extends React.Component {
DropdownList.propTypes = propTypes;
DropdownList.contextType = ThemeContext;

export default DropdownList;
export default injectIntl(DropdownList);
4 changes: 3 additions & 1 deletion packages/terra-dropdown-button/tests/jest/DropdownButton.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,9 @@ describe('Dropdown Button', () => {
it('should set the aria-label property from ./translations', () => {
const wrapper = shallowWithIntl(
<IntlProvider locale="en" messages={translationsFile}>
<DropdownButton label="Primary Option" id="dropDown" />
<DropdownButton label="Primary Option" id="dropDown">
<Item label="PDF" onSelect={() => {}} />
</DropdownButton>
</IntlProvider>,
).dive().dive();
wrapper.setState({ selectText: 'PDF' });
Expand Down
5 changes: 3 additions & 2 deletions packages/terra-dropdown-button/tests/jest/DropdownButtonBase.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';
import ThemeContextProvider from 'terra-theme-context/lib/ThemeContextProvider';

/* eslint-disable-next-line import/no-extraneous-dependencies */
import { mountWithIntl } from 'terra-enzyme-intl';
import DropdownButtonBase from '../../src/_DropdownButtonBase';
import { Item } from '../../src/DropdownButton';

Expand Down Expand Up @@ -68,7 +69,7 @@ describe('Dropdown Button Base', () => {
});

it('correctly applies the theme context className', () => {
const wrapper = mount(
const wrapper = mountWithIntl(
<ThemeContextProvider theme={{ className: 'orion-fusion-theme' }}>
<DropdownButtonBase
items={
Expand Down
62 changes: 53 additions & 9 deletions packages/terra-dropdown-button/tests/jest/DropdownList.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
import React from 'react';
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';
import translationsFile from '../../translations/en.json';
/* eslint-disable-next-line import/no-extraneous-dependencies */
import DropdownList from '../../src/_DropdownList';
import { Item } from '../../src/DropdownButton';

describe('Dropdown List', () => {
it('renders a default dropdown list', () => {
const wrapper = shallow(
const wrapper = shallowWithIntl(
<DropdownList requestClose={() => {}}>
<Item label="1st Option" onSelect={() => {}} />
</DropdownList>,
);
).dive();

expect(wrapper).toMatchSnapshot();
// ensures both data attributes are added when list contains single item
Expand All @@ -19,7 +23,7 @@ describe('Dropdown List', () => {
});

it('renders a dropdown list with a set width', () => {
const wrapper = shallow(
const wrapper = shallowWithIntl(
<DropdownList requestClose={() => {}} width="440px">
<Item label="1st Option" onSelect={() => {}} />
</DropdownList>,
Expand All @@ -28,7 +32,7 @@ describe('Dropdown List', () => {
});

it('renders a dropdown list with multiple children', () => {
const wrapper = shallow(
const wrapper = shallowWithIntl(
<DropdownList requestClose={() => {}}>
<Item label="1st Option" onSelect={() => {}} />
<Item label="2nd Option" onSelect={() => {}} />
Expand All @@ -39,7 +43,7 @@ describe('Dropdown List', () => {
});

it('renders a dropdown list a non-default focused option', () => {
const wrapper = shallow(
const wrapper = shallowWithIntl(
<DropdownList requestClose={() => {}}>
<Item label="1st Option" onSelect={() => {}} />
<Item label="2nd Option" onSelect={() => {}} />
Expand All @@ -52,7 +56,7 @@ describe('Dropdown List', () => {
});

it('renders a dropdown list an active option', () => {
const wrapper = shallow(
const wrapper = shallowWithIntl(
<DropdownList requestClose={() => {}}>
<Item label="1st Option" onSelect={() => {}} />
<Item label="2nd Option" onSelect={() => {}} />
Expand All @@ -65,7 +69,7 @@ describe('Dropdown List', () => {
});

it('renders a dropdown list an active and focused option', () => {
const wrapper = shallow(
const wrapper = shallowWithIntl(
<DropdownList requestClose={() => {}}>
<Item label="1st Option" onSelect={() => {}} />
<Item label="2nd Option" onSelect={() => {}} />
Expand All @@ -78,7 +82,7 @@ describe('Dropdown List', () => {
});

it('correctly applies the theme context className', () => {
const wrapper = mount(
const wrapper = mountWithIntl(
<ThemeContextProvider theme={{ className: 'orion-fusion-theme' }}>
<DropdownList requestClose={() => {}}>
<Item label="1st Option" onSelect={() => {}} />
Expand All @@ -89,4 +93,44 @@ describe('Dropdown List', () => {
);
expect(wrapper).toMatchSnapshot();
});

it('should set the aria-label property to empty on keydown', () => {
const listRefMock = {
childNodes: [
{ focus: jest.fn() },
{ focus: jest.fn() },
{ focus: jest.fn() },
],
};

const eventMock = {
key: 'ArrowDown',
keyCode: 40,
target: { textContent: 0 },
preventDefault: jest.fn(),
stopPropagation: jest.fn(),
};

const wrapper = shallowWithIntl(
<IntlProvider locale="en" messages={translationsFile}>
<DropdownList id="dropdownList" requestClose={() => { }}>
<Item id="firstItem" label="1st Option" onSelect={() => { }} />
<Item label="2nd Option" onSelect={() => { }} />
<Item label="3rd Option" onSelect={() => { }} />
</DropdownList>
</IntlProvider>,
).dive().dive();

wrapper.instance().listRef = listRefMock;
const firstListItem = wrapper.find('#firstItem');
const firstListItemAriaLabelValue = firstListItem.props()['aria-label'];
const expectedAriaLabelValueInitial = `${translationsFile['Terra.dropdownButton.expanded']}1st Option,(1 ${translationsFile['Terra.dropdownButton.of']} 3)`;
expect(firstListItemAriaLabelValue).toEqual(expectedAriaLabelValueInitial);

// Simulate keydown event
wrapper.instance().handleKeyDown(eventMock);
const updatedFirstListItemAriaLabelValue = wrapper.find('#firstItem').props()['aria-label'];
const expectedAriaLabelValue = `1st Option,(1 ${translationsFile['Terra.dropdownButton.of']} 3)`;
expect(updatedFirstListItemAriaLabelValue).toEqual(expectedAriaLabelValue);
});
});
4 changes: 3 additions & 1 deletion packages/terra-dropdown-button/tests/jest/SplitButton.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ describe('Dropdown Button', () => {
it('should set the aria-label property from ./translations', () => {
const wrapper = shallowWithIntl(
<IntlProvider locale="en" messages={translationsFile}>
<SplitButton label="1st Option" id="splitDropDown" />
<SplitButton label="1st Option" id="splitDropDown">
<Item label="PDF" onSelect={() => {}} />
</SplitButton>
</IntlProvider>,
).dive().dive();
wrapper.setState({ selectText: 'PDF' });
Expand Down
Loading