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

feat(a11y): add support for adding aria attributes to input element #4132

Closed

Conversation

kiranyedidi
Copy link

@kiranyedidi kiranyedidi commented Jul 20, 2020

We are using react-select in our project and we are doing accessibility check and found out that it is not compliant to WCAG 2. Right now, there is no way to provide the required aria attributes to the input element. This PR is to add the ability to provide aria attributes which can be added to the input element.

https://www.digitala11y.com/aria-autocomplete-properties/

Above is the reference link to what we are trying to achieve. Below is the short story from the above link.

If an element has aria-autocomplete set to list or both, authors MUST ensure both of the following conditions are met:

The element has a value specified for aria-controls that refers to the element that contains the collection of suggested values.
Either the element or a containing element with role combobox has a value for aria-haspopup that matches the role of the element that contains the collection of suggested values.

Please let me know if I miss something.

@changeset-bot
Copy link

changeset-bot bot commented Jul 20, 2020

💥 No Changeset

Latest commit: 60365e9

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 20, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 60365e9:

Sandbox Source
react-codesandboxer-example Configuration

@bladey bladey added pr/enhancement PRs that are specifically enhancing the project pr/needs-review PRs that need to be reviewed to determine outcome labels Jul 21, 2020
// Helper function to form aria attributes from aria object prop
const attributesFromObject = (prefix, items) =>
Object.keys(items).reduce((acc, name) => {
acc[`${prefix}-${name}`] = items[name];

Choose a reason for hiding this comment

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

To be honest, this is not straightforward API. For me, I would like to pass the aria attributes to the component directly

<Select aria-describedby="sth" />

not via separate property. Also, now there are 2 different ways to provide the aria props. And this is not tested nor documented too.

Copy link
Author

Choose a reason for hiding this comment

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

So the reason to go this route (aria prop) was to make it generic for adding aria props, so that even in future, if need to add few more aria props, we do not have to open another pull request. This is how react-modal is currently supporting for adding aria props.

And I just did not want to remove the existing aria props because of backward compatibility.

Copy link

Choose a reason for hiding this comment

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

I don't think we would want to expose every possible aria attribute.

For example, look at the list of aria attributes in @types/react. Not all of these make sense and can lead to even more broken keyboard accessibility for people who have best intentions.

Instead, we should indeed limit it to just the attributes that make sense as the value input for react-select.

@LKF92
Copy link

LKF92 commented Jul 27, 2020

We've run into the same problem regarding the aria-autocomplete attribute being not allowed on the input (tested with Continuum Explorer Community tool).
Would this PR allow us to remove the aria-attribute completely ?
Thanks!

@bladey bladey added pr/needs-review PRs that need to be reviewed to determine outcome and removed pr/needs-review PRs that need to be reviewed to determine outcome labels Aug 24, 2020
@ebonow
Copy link
Collaborator

ebonow commented Jan 21, 2021

Greetings @kiranyedidi and my apologies for such a long wait until receiving a response to this PR.

I wanted to thank you for the effort and time you put into making this for the community. It seems unacceptable that so much time had passed from your time of development to a response about having it reviewed. I apologize that this was the case and our contributor community deserves to know that they are heard and trying to contribute to a mutual cause.

Myself, the team and the community appreciate any and all efforts to improve react-select.

I wanted to let you know that the collaboration team reviewed the code and decided that it would not be a good fit to be merged. We agree with the thoughts above that it does not do enough to limit the aria attributes only to what would be relevant and would prefer a strategy that allows users to enter the aria props directly.

There is currently another PR #4322 that is more thorough in regards to implementation and I think if we are going to address accessibility, we should consolidate our attention there. The challenges of applying the correct roles and attributes are not arbitrary so we do want to do so in a way that does not conflict with the aria-live implementation which I am currently reviewing.

In the time being, aria-attributes can be added via custom components. This is not to understate the importance of the issue, but simply to offer that the tools are available to provide accessibility until the proper strategy can be implemented without having to break users should we release something that isn't completely thought out.

Again thank you for your contribution and looking forward to any support you can provide us with the impending discussions on accessibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/enhancement PRs that are specifically enhancing the project pr/needs-review PRs that need to be reviewed to determine outcome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants