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

ARIA Accessibility Fixes #2729

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

blarsonnu
Copy link

Using the Deque Axe DevTools (https://www.deque.com/axe/devtools/) we've found some ARIA accessibility bugs and would like to have them fixed. If you're not familiar with Axe, it is a great tool that also has free developer browser extensions for testing. Below are the details.

Issue #1: ARIA role combobox not allowed on a button element
Issue #2: Insufficient color contrast on the placeholder text

Some background on Issue #1:
The issue is that the button role set by Bootstrap Select is ‘combobox’, however this is not a valid ARIA role for a button and thus fails the Axe testing. A button element already has native semantics through HTML/Browsers.

In the majority of cases setting an ARIA role and/or aria-* attribute that matches the default implicit ARIA semantics is unnecessary and not recommended as these properties are already set by the browser.

Many HTML5 elements come with default implicit ARIA semantics, and explicitly setting these default values is "unnecessary and not recommended".

Looking at the button element, you can see that it has the button role by default. So, setting role="button" is "not recommended. As the W3C documentation says : button element has default role="button".

Allowed ARIA role attribute values:
button (default - do not set), link, menuitem, menuitemcheckbox, menuitemradio or radio.

Role of combobox is not a valid role for a button as defined above.

I've also attached the Axe test failures for your reference.
button-invalid-aria-role
placeholder-aria-contrast-error

Copy link
Collaborator

@NicolasCARPi NicolasCARPi left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

@NicolasCARPi NicolasCARPi added the mergeable Should be merged label Apr 16, 2022
Copy link
Member

@caseyjhol caseyjhol left a comment

Choose a reason for hiding this comment

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

Based on the examples here: https://www.w3.org/TR/wai-aria-practices-1.1/examples/combobox/aria1.1pattern/listbox-combo.html, I think we'd still want to include role="combobox", but on the parent div instead. If you (and Axe) agree, please make the change. Thanks!

@caseyjhol caseyjhol changed the title ARIA Assessibility Fixes ARIA Accessibility Fixes Apr 20, 2022
@blarsonnu
Copy link
Author

Based on the examples here: https://www.w3.org/TR/wai-aria-practices-1.1/examples/combobox/aria1.1pattern/listbox-combo.html, I think we'd still want to include role="combobox", but on the parent div instead. If you (and Axe) agree, please make the change. Thanks!

Your example link is not working, can you provide one that is for reference. I believe I looked at your suggestion and it caused other issues with Axe, but will take a look at this. Your reference example would be helpful as most of what I've seen does not match.

@blarsonnu
Copy link
Author

I've looked into adding role="combobox" to the parent div, however this is causing other ARIA errors within Axe that I'm not sure how to, or if they can easily be addressed. Based on the examples I've seen and in looking at child elements which are all well defined and that Axe is not seeing any errors as is, I'm not sure this change is necessary and at this time would recommend against adding this. Below are the sample errors from Axe after adding role="combobox" to the parent div for your reference
soap-axe-error1
soap-axe-error2
.

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

Successfully merging this pull request may close these issues.

3 participants