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

Conversation

sushmabadam
Copy link

This PR contains the fixes for the issue #1399 Accessibility Enhancements - Aria tags, Space/Enter keys

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 92.278% when pulling 1e781a8 on Kroger-Technology:master into ef3a468 on JedWatson:master.

@cschoder
Copy link

cschoder commented Jul 5, 2017

Is the only thing preventing this merge the test coverage?

Copy link
Collaborator

@agirton agirton left a comment

Choose a reason for hiding this comment

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

Overall this is looking good. I noticed the coverage was reduced so we will need to update to approve these changes. Let me think of a few more uses cases of what should be added.

@@ -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.

@@ -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']}
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 😄

@sattaman
Copy link

It would be great to get this merged!

@dsumer
Copy link

dsumer commented Oct 11, 2017

bump :) .. would be really nice to see those changes in the next version of react-select!

gwyneplaine added a commit that referenced this pull request Oct 25, 2017
fixed failing tests, resolved merge conflicts on #1428
@gwyneplaine
Copy link
Collaborator

Thanks for this @sushmabadam I've cleaned this up and merged it into master in the following PR #2092

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants