[ARIA] Add more Accessible Tags in Input and MenuList

[0xTheProDev]

Added combo box role for Input item, listbox role to MenuList and option role to MenuItems. Added relationship between elements by ID.

Signed-off-by: Progyan Bhattacharya bprogyan@gmail.com

[changeset-bot]

⚠️ No Changeset found

Latest commit: 463b02c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, 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]

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 463b02c:

Sandbox	Source
react-codesandboxer-example	Configuration

[0xTheProDev]

@kylemh Can you review again?

[kylemh]

Great work on this btw. This is a complex component for sure!

[kylemh]

The last thing that would go far is if you added snapshot tests that actually output role="listbox" it seems like all the snapshot tests are for searchable selects.

[0xTheProDev]

@kylemh Added a snapshot in Select.test, let me know if you need me to add in other tests as well!

[kylemh]

Looks good to me! May wanna @ one of the contributors.

[0xTheProDev]

@JedWatson @gwyneplaine Either of you can look?

[kylemh]

Maybe @ebonow

[ad1992]

@kylemh we are waiting for this to get merged. When can we expect the next release to happen?

[ebonow]

@ad1992 Sounds like Jed will be putting some time aside early next week to continue to groom through the remaining PRs. He is very anxious to move forward with a new release/releases so could be as early as then. We got through about 1/4 of the existing PR's the last time we met up, so it all depends on how far we get.

That said, there was a pre-existing implementation of aria tags throughout the library, but it sounds like there were some inconsistencies so it's hard to say what was tried and how this is different from that previous approach vs perhaps advances in the aria standards.

[ad1992]

@ebonow sure if the release can happen sometime this week or early next week would be great. In case it gets delayed we would like to release a forked version and start using it, would be great if you can guide us on how we can do that.

[kylemh]

You should simply fork and keep eyes on this PR!

[ad1992]

You should simply fork and keep eyes on this PR!

@kylemh I meant the steps needed to build and publish this branch so we can start using it in production in case the release gets delayed

[kylemh]

There's no guarantee this PR even gets in the next release.

[ebonow]

@ad1992 +1 to what @kylemh said.

As mentioned above, there is some uncertainty about why react-select removed their previous implementation of aria tags to instead providing aria live messaging instead especially with all of the various configurations of react-select.

This all said, most all of the changes can already be done using the components api. The only difference appears to be automatically generating an id for the menuList, which should be straight forward enough to pass that in too to the Select.

const Option = props => { 
  const innerProps = { ...props.innerProps, role: 'option', 'aria-disabled': props.isDisabled };
  return <components.Option {...props} innerProps={innerProps} />
}

const MenuList = props => {
  const innerProps = { ...props.innerProps, id: props.selectProps.menuId, role: 'listbox', 'aria-expanded': true };
  return <components.MenuList {...props} innerProps={innerProps} />
}

const Input = props => {
    // aria-label and aria-labelledby are already spread by the existing props if provided
  const innerProps = { 
    ...props.innerProps, 
    role: props.selectProps.isSearchable ? 'combobox' : 'listbox',
    'aria-owns': props.selectProps.menuId,
    'aria-autocomplete': 'list',
  };

  return <components.Input  {...props} innerProps={innerProps} />
}

const ariaSelect = props => <Select menuId="unique_menu_id" components={{ Option, MenuList, Input }} {...props} />

@kylemh please correct me if Im wrong, but does this not accomplish everything the PR is doing and without changing any internal methods outside of providing a unique id for the menu?

Oh which reminds me that this PR is missing aria-disabled for the disabled options which the above code accounts for.

[kylemh]

Indeed @ebonow

To highlight the problem and why it's not as simple as always providing these defaults, imagine if a developer were to make an instance of react-select where the menu is alwyas open and also where an option moves the app's focus to the next form element. This would essentially make the react-select behave like a radio input and require different considerations for aria attributes - it certainly wouldn't be a listbox or combobox.

Finding a system to that provides accessible default for majority use cases is important, which is why I reviewed this PR; however, we don't have a system of delivering those defaults without fucking over the consumers using this component in a more unique manner.

The important note is that you already have the tools you need to create YOUR perfect, accessible use-case. You could fork the library to conform the default accessibility to your uses; however, alternatively, you could simply define a component that uses react-select and uses the "components API" to define the defaults within your app/company.

[ebonow]

@kylemh I'm confused with what "defaults" you are referring to. I simply moved the internal "ariaProps" component logic into these custom components. I was hoping you could review to see if I missed anything as it seems you are the most active/familiar with this PR with the intention was to help @ad1992 (and any others) to achieve the exact same functionality today without unnecessarily forking.

I appreciate your passion for accessibility and want to reiterate that I agree that accessibility is both important and should be integrated, but that said, there was a decision to move from aria tags to aria-live messaging instead. I think this merits a better understanding of why that decision was reached and I am hoping to accomplish that tonight to better convey any short-comings that were experienced.

As is, it's possible that this could conflict with the assertive live-messaging but I haven't tested this. If it were the case that this conflicts with the messaging, it likely means we would need some way to provide the flexibility to let the dev choose which implementation which could be less straightforward than proposed in the PR.

I hope this seems like a reasonable consideration. In the meanwhile, given the PR changes, is there anything the code I supplied doesn't do that that this PR does from a functionality perspective?

[kylemh]

I wasn't contradicting you or debating with you! I was trying to agree with you with an additional perspective for the others to see. I brought up ideal defaults because I believe that was the goal in creating this PR; however, as you've pointed out - it may not be correct.

[0xTheProDev]

@ebonow Updated as you asked.

[JedWatson]

As discussed above, and to @kylemh's points, this is a pretty complex and opinionated thing to default that we can't predict will be accurate for all use cases so I've removed it from 3.2 for the time being and we can revisit it in the next major version

[ebonow]

@Progyan1997 I also wanted to add good job. I appreciate the speedy replies and want to be responsible in turn to keep you up-to-date.

Accessibility is something we want to prioritize and the existing implementation supports aria live messaging as the primary means of accessibility as it provides a more consistent and customizable experience than what is supported by various screen readers on various OS's especially as react-select continues to grow in complexity.

One of the blockers for this will be testing against changes being made to the aria live messaging. It could likely be counter-productive to include two competing standards and so the ability to apply these default values as defined in your PR will likely be contingent on being able to disable aria live messaging.

Please continue to be patient as there is a bit of a backlog on issues and PR's that we are working through. I and the other collaborators are committed to ensuring that contributors are being acknowledged for their work and we are working through the list and will do our best to keep you in communication as I believe this PR will likely be a good starting point to address the ability to better support WAI-ARIA should users decide that the existing aria live messaging is not adequate for their purposes.

[0xTheProDev]

Thanks for the support @ebonow and thanks @JedWatson for reviewing it.

[gauravahir28]

Good work @Progyan1997  by adding the roles and attributes on the dropdown elements!

We wanted to use this PR in hour project, but I have found that only adding roles and attribute creates problems with MAC and IPhone.
On IPhone, The voiceover isn't allowing me to select the  options as I am focusing on the listbox, the listbox gets collapsed.
And on MAC, As I navigate within the dropdown using arrow keys, the voiceover isn't reading anything!

therefore, it seems that adding roles and attributes isn't enough to achieve expected accessibility as per the W3C's combobox design pattern.

Thanks.

[0xTheProDev]

Hey @gauravahir28, can't say about IPhone, but I developed and thoroughly tested this feature on Mac. I have also tested this on Linux system. I believe the problem you're facing is not related to this changes.

[Annshuk]

@ad1992 +1 to what @kylemh said.

As mentioned above, there is some uncertainty about why react-select removed their previous implementation of aria tags to instead providing aria live messaging instead especially with all of the various configurations of react-select.

This all said, most all of the changes can already be done using the components api. The only difference appears to be automatically generating an id for the menuList, which should be straight forward enough to pass that in too to the Select.

const Option = props => { 
  const innerProps = { ...props.innerProps, role: 'option', 'aria-disabled': props.isDisabled };
  return <components.Option {...props} innerProps={innerProps} />
}

const MenuList = props => {
  const innerProps = { ...props.innerProps, id: props.selectProps.menuId, role: 'listbox', 'aria-expanded': true };
  return <components.MenuList {...props} innerProps={innerProps} />
}

const Input = props => {
    // aria-label and aria-labelledby are already spread by the existing props if provided
  const innerProps = { 
    ...props.innerProps, 
    role: props.selectProps.isSearchable ? 'combobox' : 'listbox',
    'aria-owns': props.selectProps.menuId,
    'aria-autocomplete': 'list',
  };

  return <components.Input  {...props} innerProps={innerProps} />
}

const ariaSelect = props => <Select menuId="unique_menu_id" components={{ Option, MenuList, Input }} {...props} />

@kylemh please correct me if Im wrong, but does this not accomplish everything the PR is doing and without changing any internal methods outside of providing a unique id for the menu?

Oh which reminds me that this PR is missing aria-disabled for the disabled options which the above code accounts for.

this is what we are also looking for.. react select is very nice library but in terms of accessibility it not much good. also the html elements used not the standard one, it should be wrapped in UL LI so it would be more readable as well when render i.e can refer below

[pleunv]

this is what we are also looking for.. react select is very nice library but in terms of accessibility it not much good. also the html elements used not the standard one, it should be wrapped in UL LI so it would be more readable as well when render i.e can refer below

Wrapping things in <ul> and <li> serves no purpose here. ul has an implicit "list" role while li has an implicit "listitem" role, yet you are overriding both roles with "listbox" and "option" respectively. <div> is just fine, not to say better.