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

fix(multiple-label): no longer raises issue when aria-labelledby overrides how AT views multiple labels #1538

Merged
merged 1 commit into from
May 17, 2019

Conversation

AutoSponge
Copy link
Contributor

@AutoSponge AutoSponge commented May 2, 2019

When authors use aria-labelledby on a labeled input, multiple labels will be consumed by AT. In cases where less than all labels are included, the excluded label must have aria-hidden="true".

issue: #689 (don't autoclose, docs needed)

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

@AutoSponge AutoSponge force-pushed the fix-multiple-label-labelledby branch 3 times, most recently from 3f96aa1 to 1255554 Compare May 7, 2019 20:39
@straker
Copy link
Contributor

straker commented May 7, 2019

The tests pass as expected, but I got a bit lost in the logic trying to figure out was trying to be tested. Maybe a few comments explaining the whys of the logic would be helpful

straker
straker previously requested changes May 7, 2019
lib/checks/label/multiple-label.js Outdated Show resolved Hide resolved
@straker
Copy link
Contributor

straker commented May 8, 2019

LGTM.

Because of the extent of the rule change, I just want to make sure @WilcoFiers is Ok with the direction before we merge (see #689 (comment)).

@dylanb
Copy link
Contributor

dylanb commented May 16, 2019

Needs a change to the more info page to explain the logic

@dylanb
Copy link
Contributor

dylanb commented May 16, 2019

@jeankaplansky please take up the documentation requirement and when the ticket is created, add the link here

@jeankaplansky
Copy link
Contributor

jeankaplansky commented May 16, 2019

@jeankaplansky please take up the documentation requirement and when the ticket is created, add the link here

@dylanb: Please confirm: documentation requirement exists for both Attest AND axe-core?

I know this is rule help related specifically to this rule: https://dequeuniversity.com/rules/axe/3.2/form-field-multiple-labels

I also know that @AutoSponge told me to refer to this: https://codepen.io/straker/full/PvqONy. We will mark valid the middle 3 code examples that are all green as supported. This tells me we're going to constrain the original "any time there are multiple, visible labels" gist of the current rule help. I'll create a backlog ticket for the documentation reqs and link to this PR as a dependency for the PR being marked complete.

@stephenmathieson
Copy link
Member

Merging this. We'll figure out the documentation stuff later on.

@stephenmathieson stephenmathieson merged commit fbae36b into develop May 17, 2019
@stephenmathieson stephenmathieson deleted the fix-multiple-label-labelledby branch May 17, 2019 15:17
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.

6 participants