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(rule): Require unique aria labels in checkboxgroup & radiogroup #1316

Merged
merged 4 commits into from
Jan 23, 2019

Conversation

WilcoFiers
Copy link
Contributor

@WilcoFiers WilcoFiers commented Jan 16, 2019

Update the group-labelledby check to enforce that in addition to having a shared label, radiobuttons and checkboxes with the same name also have a unique label. This impacts the checkboxgroup and radiogroup rules.

I merged in a fix from #1314, which changes the gruntfile. We should probably more that PR first.

Closes #1111

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: @dylanb

@WilcoFiers WilcoFiers requested a review from a team as a code owner January 16, 2019 13:39
if (uniqueLabels.length > 0 && sharedLabels.length === 0) {
data.failureCode = 'no-shared-label';
} else if (uniqueLabels.length === 0 && sharedLabels.length > 0) {
data.failureCode = 'no-unique-label';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not always fail this even when there is no shared label?

@dylanb
Copy link
Contributor

dylanb commented Jan 22, 2019

@WilcoFiers seems like the change to make @stephenmathieson a code owner broke our ability to approve changes in that code

@jeeyyy
Copy link
Contributor

jeeyyy commented Jan 23, 2019

@dylanb PR - #1331, fixed the codeowners issue.

@jeeyyy jeeyyy merged commit c9b310d into develop Jan 23, 2019
@jeeyyy jeeyyy deleted the unique-labelledby-groups branch January 23, 2019 14:06
@naltatis
Copy link

naltatis commented Mar 8, 2019

Feedback: Just updated axe an ran into this check. We finally could find the issue, we were having, but had to read through the tests and code modifications here to understand what was wrong.

This is the message we recieved

Ensures related <input type="radio"> elements have a group and that the group designation is consistent https://dequeuniversity.com/rules/axe/3.2/radiogroup?application=axeAPI

Fix any of the following:
  Elements with the name "segments-deliveryaddress-deliveryButtons" do not all have a unique label, referenced through aria-labelledby
  Element does not have a containing fieldset or ARIA group
  #billing-radiobutton

The actual problem was, that our radio buttons had a aria-labelledby that was pointing to the groups legend. Additionally the radios had an individual label (connected via for/id and label > input nesting), which we thought was sufficient. We were able to get rid of the error by expanding the aria annotiation by the radio inputs label-id aria-labelledby="legend-id" -> aria-labelledby="legend-id label1-id".
The error message above and the linked documentation did not point to that.

Maybe you should consider improving the message.

@dylanb
Copy link
Contributor

dylanb commented Mar 11, 2019

@naltatis Do you think you could create a new issue in this project that references this and also maybe suggest a text that might have helped you figure out what was going on?

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.

False negative: 'group-labelledby' check in 'checkboxgroup' rule does not fail corner cases
4 participants