-
Notifications
You must be signed in to change notification settings - Fork 167
[Terra-Dropdown] VO does not announce Expanded state and Index of list items while navigating through arrow keys #3805
Changes from 14 commits
3c92a1a
68f588e
bc7b3f2
43f300d
22eeb9b
6808c17
6430ad4
1adf2f2
2731c94
452467c
2ca0d2c
8c91883
c01ce54
f81a8b5
dd64555
05892e7
82755a4
e77e019
0b6bada
34742e0
c193201
cc6fbef
db727b8
e90e80c
7f9273b
c9b83aa
bc318e2
120d514
898507c
943af57
5c14beb
9e77a10
123b00a
ea0230c
2f1374e
16de4db
8efdc84
df5c65e
8ef8326
7430f42
bb91545
275521f
5b8ffd3
3e7cc7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
*/ | ||
|
@@ -45,7 +37,7 @@ const propTypes = { | |
}; | ||
|
||
const Dropdown = ({ | ||
requestClose, isOpen, targetRef, children, width, openedViaKeyboard, buttonRef, refCallback, getSelectedOptionText, | ||
requestClose, isOpen, targetRef, children, width, refCallback, getSelectedOptionText, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you need to find There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented as suggested. |
||
}) => ( | ||
<Hookshot | ||
isOpen={isOpen} | ||
|
@@ -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} | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -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'; | ||||||||
|
||||||||
|
@@ -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 { | ||||||||
|
@@ -48,6 +55,15 @@ class DropdownList extends React.Component { | |||||||
this.searchString = ''; | ||||||||
this.pressed = false; | ||||||||
this.listRef = null; | ||||||||
this.expanded = this.props.intl.formatMessage({ id: 'Terra.dropdownButton.expanded' }); | ||||||||
} | ||||||||
|
||||||||
componentDidMount() { | ||||||||
// Set focus to first focusable menu item | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changes made. Thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you provide me link to the commit in which above changes are made. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please find the link. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still see comments are not updated :
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comments added as suggested. |
||||||||
const items = this.listRef.querySelectorAll('[data-terra-dropdown-list-item]'); | ||||||||
if (items.length) { | ||||||||
items[0].focus(); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
handleKeyDown(event) { | ||||||||
|
@@ -69,6 +85,7 @@ class DropdownList extends React.Component { | |||||||
event.preventDefault(); | ||||||||
} else if (keyCode === KeyCode.KEY_DOWN) { | ||||||||
if (!this.pressed) { | ||||||||
this.expanded = ''; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a unit test to see if this gets set? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented. |
||||||||
if (index === Util.getChildArray(this).length - 1) { | ||||||||
this.changeFocusState(0); | ||||||||
} else { | ||||||||
|
@@ -78,6 +95,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 { | ||||||||
|
@@ -160,11 +178,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})`; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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})`; | ||||||||
} | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 */ | ||||||||
|
@@ -201,4 +232,4 @@ class DropdownList extends React.Component { | |||||||
DropdownList.propTypes = propTypes; | ||||||||
DropdownList.contextType = ThemeContext; | ||||||||
|
||||||||
export default DropdownList; | ||||||||
export default injectIntl(DropdownList); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.