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: separate form-field-multiple-label from label rule #1226

Merged
merged 7 commits into from
Nov 13, 2018

Conversation

jeeyyy
Copy link
Contributor

@jeeyyy jeeyyy commented Nov 7, 2018

This PR does the below:

  • removed multiple-label check as a part of label rule.
  • create a new rule spec multiple-label which is tagged as best-practice.
  • add integration tests

Closes issue:

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: @WilcoFiers

@jeeyyy jeeyyy requested a review from a team as a code owner November 7, 2018 15:51
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

I've set up a codepen to do some screenreader testing for this as well:
https://codepen.io/wilcofiers/pen/rQxEvL

Did some testing, NVDA works correctly, JAWS and VO have issues:

  • VoiceOver in Safari doesn't announce second labels in any of the 3 scenarios
  • VoiceOver in Chrome announces both labels in example 3
  • JAWS in IE and Firefox announced both labels in example 3

The most prominent one is VO, which doesn't announce multiple labels at all. That COULD be considered an accessibility support failure. @marcysutton do you think we should fail this under WCAG, just for VoiceOver or should we just make this a best practice?

@@ -0,0 +1,20 @@
{
"id": "multiple-label",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm inclined to name this something like "form-field-multiple-labels"?

@@ -56,6 +56,7 @@
| meta-refresh | Ensures <meta http-equiv="refresh"> is not used | Critical | cat.time, wcag2a, wcag2aaa, wcag221, wcag224, wcag325 | true |
| meta-viewport-large | Ensures <meta name="viewport"> can scale a significant amount | Minor | cat.sensory-and-visual-cues, best-practice | true |
| meta-viewport | Ensures <meta name="viewport"> does not disable text scaling and zooming | Critical | cat.sensory-and-visual-cues, wcag2aa, wcag144 | true |
| multiple-label | Ensures form element does not have multiple label elements | Serious | best-practice, cat.forms, wcag2a, wcag332 | true |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should change the impact to "moderate" for this. Best practices should not have an impact that is serious or critical. People don't like it when we do that :-)


<label for="fail3" id="l1">label one</label>
<label for="fail3">label two</label>
<input type="checkbox" id="fail3" aria-labelledby="l1">
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add tests for:

  • implicit + explicit label
  • multiple implicit labels
  • select elements

@jeeyyy jeeyyy requested review from marcysutton and a team November 8, 2018 14:42
@jeeyyy jeeyyy changed the title fix: make multiple-label a best-practice rule feat: make multiple-label a best-practice rule Nov 13, 2018
@jeeyyy jeeyyy changed the title feat: make multiple-label a best-practice rule feat: separate form-field-multiple-label from label rule Nov 13, 2018
@jeeyyy jeeyyy merged commit 0e0063c into develop Nov 13, 2018
@jeeyyy jeeyyy deleted the rule-multiple-label branch November 13, 2018 10:49
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.

None yet

2 participants