Skip to content
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

Accessibility Enhancements - Aria tags, Space/Enter keys #1428

Closed
wants to merge 1 commit into from
Closed
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
20 changes: 18 additions & 2 deletions src/Select.js
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,10 @@ const Select = React.createClass({
this.selectFocusedOption();
return;
case 13: // enter
if (!this.state.isOpen) return;
if (!this.state.isOpen) {
this.focusNextOption();
return;
}
event.stopPropagation();
this.selectFocusedOption();
break;
Expand All @@ -498,6 +501,17 @@ const Select = React.createClass({
event.stopPropagation();
}
break;
case 32: // space
if (!this.props.searchable) {
event.preventDefault();
}
if (!this.state.isOpen) {
this.focusNextOption();
return;
}
event.stopPropagation();
this.selectFocusedOption();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey guys, sorry to drudge up an old PR discussion, but I'm noticing some quirky behavior since this code. Bisecting for bug #2156 led me to PR #2092 and therefore here.

It would seem this didn't intentionally cause the functionality I'm noticing on multiselect (selecting options on space or creating options on space for Creatable), so I'm trying to figure out how I might fix this bug while keeping the intentions of this change.

What use case are we trying to add here? As an example of a use case we don't want (I'm assuming) is if you're on the example page in the first example of states and click "United States," then start typing to New York, when you hit space after New it will select New Hampshire instead of continuing to filter down.

Tagging @gwyneplaine as the merger.

Copy link
Collaborator

@gwyneplaine gwyneplaine Nov 21, 2017

Choose a reason for hiding this comment

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

Hey @buob thanks for catching this, the intention was to mimic native select behaviour, to enable selection of a focused option on SPACE. the behaviour you've identified is definitely an unintended side-effect.

Let me have a think about how to best solve for both these cases (it may very well be removing this behaviour when props.searchable is true). Happy to hear your thoughts on this as well

break;
case 38: // up
this.focusPreviousOption();
break;
Expand Down Expand Up @@ -866,12 +880,14 @@ const Select = React.createClass({
aria-expanded={isOpen}
aria-owns={isOpen ? this._instancePrefix + '-list' : this._instancePrefix + '-value'}
aria-activedescendant={isOpen ? this._instancePrefix + '-option-' + focusedOptionIndex : this._instancePrefix + '-value'}
aria-labelledby={this.props['aria-labelledby']}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Looks like there is a space before the dash, please remove.

aria-label={this.props['aria-label']}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please update this also.

className={className}
tabIndex={this.props.tabIndex || 0}
onBlur={this.handleInputBlur}
onFocus={this.handleInputFocus}
ref={ref => this.input = ref}
aria-readonly={'' + !!this.props.disabled}
aria-disabled={'' + !!this.props.disabled}
Copy link
Collaborator

Choose a reason for hiding this comment

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

One more 😄

style={{ border: 0, width: 1, display:'inline-block' }}/>
);
}
Expand Down
43 changes: 41 additions & 2 deletions test/Select-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ var PLACEHOLDER_SELECTOR = '.Select-placeholder';
var ARROW_UP = { keyCode: 38, key: 'ArrowUp' };
var ARROW_DOWN = { keyCode: 40, key: 'ArrowDown' };
var KEY_ENTER = { keyCode: 13, key: 'Enter' };
var KEY_SPACE = { keyCode: 32, key: 'Space' };

class PropsWrapper extends React.Component {

Expand Down Expand Up @@ -3191,7 +3192,6 @@ describe('Select', () => {
});

it('disabled option link is still clickable', () => {
var selectArrow = ReactDOM.findDOMNode(instance).querySelector('.Select-arrow');
var selectArrow = ReactDOM.findDOMNode(instance).querySelector('.Select-arrow');
TestUtils.Simulate.mouseDown(selectArrow);
var options = ReactDOM.findDOMNode(instance).querySelectorAll('.Select-option');
Expand Down Expand Up @@ -3845,7 +3845,7 @@ describe('Select', () => {
</span>);
});

it('updates the active descendant after a selection', () => {
it('updates the active descendant after a selection using enter key', () => {

return expect(wrapper,
'with event keyDown', ARROW_DOWN, 'on', <div className="Select-control" />,
Expand All @@ -3861,6 +3861,45 @@ describe('Select', () => {
});

});

it('expands the drop down when the enter key is pressed', () => {

return expect(wrapper,
'with event keyDown', KEY_ENTER, 'on', <div className="Select-control" />,
'queried for', <input role="combobox" />)
.then(input => {
expect(instance.state.focusedOption, 'to equal', { value: 'one', label: 'label one' });
});

});

it('updates the active descendant after a selection using space bar', () => {

return expect(wrapper,
'with event keyDown', ARROW_DOWN, 'on', <div className="Select-control" />,
'with event keyDown', KEY_SPACE, 'on', <div className="Select-control" />,
'queried for', <input role="combobox" />)
.then(input => {

// [ 'three', 'two', 'one' ] is now selected,
// therefore in-focus should be 'four'

const activeId = input.attributes['aria-activedescendant'].value;
expect(ReactDOM.findDOMNode(instance), 'queried for first', '#' + activeId, 'to have text', 'label four');
});

});

it('expands the drop down when the space bar is pressed', () => {

return expect(wrapper,
'with event keyDown', KEY_SPACE, 'on', <div className="Select-control" />,
'queried for', <input role="combobox" />)
.then(input => {
expect(instance.state.focusedOption, 'to equal', { value: 'one', label: 'label one' });
});

});
});
});
});