Skip to content

Only add 'for' value if errorElement is a label #900

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

Closed

Conversation

nschonni
Copy link
Collaborator

A 'for' on other types causes problems for screen readers, and is not valid HTML

A 'for' on other types causes problems for screen readers, and is not
valid HTML
@nschonni
Copy link
Collaborator Author

nschonni commented Oct 7, 2013

Needs to include a fix for the "for" assumption described here wet-boew/wet-boew#3074

.addClass(this.settings.errorClass)
.html(message || "");

if ( /label/.test( this.settings.errorElement ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would make sense to move this regex up so it wont get compiled everytime again

@pjackson28
Copy link
Contributor

@nschonni The following fix is needed on https://github.com/nschonni/jquery-validation/blob/bc2395ef3f1710f2b3630548e91a6b7c02fc96cc/src/core.js#L722 to ensure the errors don't get duplicated when the error element is nested within a label element (also removes the unnecessary jQuery object creation to help performance):

Before:

return $(this).attr("for") === name;

After:

return this.getAttribute("for") === name || this.parentNode.getAttribute("for") === name;

@jzaefferer
Copy link
Collaborator

@nschonni as the comment from @pjackson28 suggests, simply adding the for-attribute only to label elements falls short. I think what's needed as a separate marker, to connect the error element to the input. Just gussing that the label is the parent element is not enough. The obvious choice seems to use data attributes, since those can contain anything. I'm not sure if that is useful here - can you give that a try?

Maybe my concern isn't even correct. It would be good to start with a unit test that checks that non-label error elements are hidden or removed properly.

@jzaefferer
Copy link
Collaborator

See also #967, which adds an option to configure the attribute. Adding options isn't acceptable, this needs a solution that works without configuration, with label and other types of elements.

@jzaefferer
Copy link
Collaborator

I wrote this a while ago but never posted it here:

This breaks the errorsFor method for non-label elements, as that matches error elements based on the for-attribute. We need a unit test that actually checks that an error element gets updated after its first use, which your new test doesn't cover - and it looks like no existing test does either.

How about a data-for attribute, replacing the for-attribute as its used right now, then also adding a regular for-attribute for labels?

Thinking more about this, maybe its a bad idea to use a label element for errors in the first place. We can just add a click-handler on the label to link it to the input (click to focus). We could use aria-describedby to link the error messages to the input. Or maybe there's something that's more appropriate in this context.

@pjackson28
Copy link
Contributor

@jzaefferer It would be ideal to have the ability to include the error message in an existing label because that is what is best supported by screen readers. Screen reader support gets a little dicey when trying to have multiple label elements for a single element or multiple sources of labeling information. There is nothing wrong with having the flexibility to take different approaches (such as through data-for) but the primary approach should be the best supported one.

@ghost ghost assigned nschonni Jan 14, 2014
@jzaefferer
Copy link
Collaborator

@pjackson28 thanks for the input. Could you outline what you consider the best approach here? Should the plugin use something other then a label element by default? The primary motivation for using a label was getting "click to focus on input" events for free, but those are easy enough to hook up manually. It also seemed like the right markup at the time, though if using proper ARIA attributes does the same job and cause less issues, then we should go with that. I can't quite tell right now.

@pjackson28
Copy link
Contributor

@jzaefferer From a standards perspective (e.g., WCAG 2.0), all field labels should already be associated to their fields with label elements to get both the "click to focus on input" and for it to be read off by screen readers when each field gains focus. So the ideal assumption would be that there are already labels in place. Since screen reader support for more than one label per field isn't great, the best approach would be to reuse the label elements that should already be present, so you get for the error messages "click to focus on input" and screen readers reading them. Now there could be cases where people choose not to use labels, but that should be the exception to the rule rather than the norm.

So to answer your question, I think the plugin shouldn't be using the label element by default, but to instead use a generic element (or a user specified element) that would be a child of an existing label element. There should also be a mechanism to override this for those that aren't using labels for whatever reason.

@jzaefferer
Copy link
Collaborator

Okay, thanks. Since this is now a more fundamental discussion, let's see if other people experienced with ARIA (and forms) may want to chime in. @fnagel @dylanb - do you have recommendations for this? For a quick start, read the three comments above this one. Thanks!

@dylanb
Copy link

dylanb commented Jan 16, 2014

Used to be that we would recommend the label wrapping approach and then putting errors inside the label (say in a span, so you can style it differently) because that was widely supported. However, with the very wide support for aria-labelledby and (slightly lower, but still wide support for aria-describedby), we now recommend using aria-labelledby and aria-describedby and NOT wrapping the label around the input because the wrapped label can lead to double-announcements on OS-X.

This also provides the widest control about where the labels and the errors can go because the DOM location and the spoken order can be controlled independently.

Only exception would be the checkboxes.

@pjackson28
Copy link
Contributor

@dylanb I agree that you shouldn't wrap a label around a text input field but that is not what I'm suggesting. We keep the label and input elements separate (for more flexibility over DOM location) and build the relationship between them using the for attribute. We extensively use WAI-ARIA to enhance our sites and widgets but I don't see the benefit of relying on WAI-ARIA in this case when well supported native semantics already exist, are well supported and are equally as flexible. We still have users that use screen readers that don't have WAI-ARIA support so making aria-labelledby or aria-sescribedby the primary solution wouldn't work for us. Those users can't expect the same experience as those with WAI-ARIA support but at the same time we shouldn't be cutting off basic forms support for them either when WAI-ARIA doesn't offer any improvement (when the label and input elements are separate).

@dylanb
Copy link

dylanb commented Jan 16, 2014

Having now read through most of the above thread and not just the immediately preceding posts, I agree with @pjackson28 in that the labeling and the validation should probably be totally decoupled. Let the form designer figure out how they want to layout their form and how they want to do label and error association and simply provide to the validator in some way the information about where it should place the information it generates.

If the validator needs some way to identify that information so it can be removed/updated, wrap it in a and supply a data- attribute to track that.

@madmatt
Copy link

madmatt commented Jan 17, 2014

I created a PR (#967) which allowed you to customise the errorAttribute as well as the errorLabel, so for example by default, you would have errorElement: 'label' and errorAttribute: 'for', which would create <label for="form_field_id">Error message</label>, which would be created in addition to the existing <label> if you had one to give the form field title. To comply with WCAG etc. that suggests only one <label> per <input>, <select> etc. you could instead have errorElement: 'span' and errorAttribute: 'data-for' which would still be valid (I think, but I'm not any kind of expert) for screen readers. This would create HTML like <span data-for="form_field_id">Error message</span>.

However, we can't add new settings to jquery-validation, so my alternate solution would be to hardcode in the for and data-for options depending on whether errorElement === 'label' or not.

Would this be a good solution, or are we wanting a more flexible solution than that? It appears from the above comments that the idea is still to keep the original <label> (if any), <input> tag and validation message that appears completely separate - is that right?

I'm happy to code up a PR for discussion if we can decide on the best way to approach the problem without adding any new settings.

@dylanb
Copy link

dylanb commented Jan 17, 2014

Please correct me if any of the following assumptions are wrong:

  1. The validation plugin currently adds a new element to the form when a validation error occurs and inserts the validation message into this element,
  2. Currently it does this by creating a element with a "for" attribute that points to the input
  3. If a with that "for" attribute exists, the validation results in two different elements both pointing to the same element
  4. We want a fix that cannot add any new options to the plugin
  5. You can create a totally custom validation markup generator, I have done that here https://github.com/dylanb/a11yfy/blob/master/a11yfy/jquery.a11yfy.core.js#L45

So, given that 5 is possible, is the aim here to simply create standard validation markup that will work with screen readers? If so, do we want to keep the current scenario, where the standard markup places the error message in the DOM after the appropriate field?

If all of that is true, then I would recommend adding a element and using aria-describedby to point to that. This would work for most screen readers and if there is some user of the plugin who has to support older screen readers that do not support this, then they can do 5 above.

thoughts?

@pjackson28
Copy link
Contributor

@dylanb Even though 5 may be possible, I don't see it as being reasonable to expect implementers to do it. I'm already force overriding jQuery Validation to get the result I want so I don't want to switch to another overriding scenario to do something as simple as injecting the error message in the element of my choice. jQuery Validation is supposed to make it easier not harder to implement, I shouldn't have to battle the plugin and do custom markup handling for something this simple. Part of the reason we walked away from jQuery Mobile was we got tired of battling the framework to do the basics.

All I'm looking for is simple and built-in flexibility to put the error message in the element of my choice. Currently it lets me specify the element but blindly forces on the "for" attribute which is only allowed on a label element (which causes screen reader issues when on the wrong element). I don't want the for attribute forced on blindly when you let me specify the element. I don't want two label elements because screen reader support for multiple label elements associated is quite flaky. I don't want to be forced over to aria-describedby, which doesn't have as wide of support as the native label element, because you don't want to build in control over the attribute being added to the element I can specify.

There are simple ways to enable what I'm looking for. Allowing me to specify the referring attribute or to automatically use the data-for attribute for non-label elements is one way. Adding support for the error message being the child of a label element (as covered above) is another. All of these options can be done with less code and with better performance than what you describe in 5.

@jzaefferer
Copy link
Collaborator

Thanks for the input everyone. Based on the discussion so far, I suggest this approach:

  • Change default error element to <span>, associate that to the input using aria-describedby
  • Make sure the API is flexible enough to put that error element into an existing label. This should be possible already (using errorPlacement), but needs to be verified.

With that, the framework would assume that inputs always have a proper label (wrapping the input or using the for-attribute). There would be only that one label. Error messages would be associated using aria-describedby, which the framework can also use to find existing error messages when trying to update them.

What do you think?

@pjackson28
Copy link
Contributor

@jzaefferer that likely would work for me. We should check on a few screen readers though to make sure this doesn't result in double reading of the error message (from the label and the aria-describedby). I can check on Jaws 14.0 and NVDA 2013.3 on Windows 7. I would suggest using something like the following for the screen reader test (easier to test this with straight markup without jQuery Validation involved):

<label for="fname">First name <span id="test1">Error: First name is required</span></label><input type="text" id="fname" name="fname" aria-describedby="test1" />

@pjackson28
Copy link
Contributor

@jzaefferer I just checked and it unfortunately does double read in NVDA 2013.3. It reads the label contents first, field information, then the aria-describedby last. The following is what was read to me:

"First Name Error: First name is required Edit for auto-complete. Error: First name is required blank."

What about using something like data-for by default instead of aria-describedby if the default element is span and it is expected to be nested in a label? If you need to cover the scenario where it is not nested in a label element then you could potentially quickly check to see if the parent is a label element and apply aria-describedby as well if it isn't (simpler code to always rely on one attribute for finding the error).

@pjackson28
Copy link
Contributor

Also note that it sounded odd to hear the aria-described error message in between the field information and blank. It actually sounded like the requirement was for the field to be blank (although a period at the end of the error message could help a bit). Really unfortunate that NVDA chose to read aria-describedby in between those two items. Ideally the order would have been label contents first, aria-describedby second, field information third and blank last to group describing information together and field information together.

@dylanb
Copy link

dylanb commented Jan 17, 2014

@jzaefferer If you modify your logic just a bit to only put the aria-describedby when the error placement is outside the label, then this should work fine

@jzaefferer
Copy link
Collaborator

Alright, with those modifications, the plan should be this:

  • Change default error element to <span>, associate that to the input using aria-describedby, except when its appended to an existing label element (leave it out then).
  • Make sure the API is flexible enough to put that error element into an existing label. This should be possible already (using errorPlacement), but needs to be verified.

How does that sound?

I would appreciate if someone could try to implement that (@nschonni?). It'll be a week or two until I can spend enough time on this plugin again.

@pjackson28
Copy link
Contributor

@jzaefferer That works for me. Thanks!

@jzaefferer jzaefferer added this to the 1.13.0 milestone Apr 1, 2014
tractorcow pushed a commit to tractorcow/jquery-validation that referenced this pull request Apr 15, 2014
Non-label error labels will now work correctly, with aria-describedby now superseding dependency on the 'for' attribute
Fixes jquery-validation#900
@tractorcow
Copy link
Collaborator

Hi @jzaefferer , would you mind looking at my suggested solution at #1083 please? :) Thank you!

tractorcow pushed a commit to tractorcow/jquery-validation that referenced this pull request Apr 16, 2014
Non-label error labels will now work correctly, with aria-describedby now superseding dependency on the 'for' attribute
Fixes jquery-validation#900
tractorcow pushed a commit to tractorcow/jquery-validation that referenced this pull request Apr 16, 2014
@jzaefferer
Copy link
Collaborator

Closing this since we're making good progress in #1083.

@jzaefferer jzaefferer closed this Apr 29, 2014
tractorcow pushed a commit to tractorcow/jquery-validation that referenced this pull request Apr 30, 2014
tractorcow pushed a commit to tractorcow/jquery-validation that referenced this pull request Apr 30, 2014
tractorcow pushed a commit to tractorcow/jquery-validation that referenced this pull request May 7, 2014
tractorcow pushed a commit to tractorcow/jquery-validation that referenced this pull request May 8, 2014
tractorcow pushed a commit to tractorcow/jquery-validation that referenced this pull request May 11, 2014
tractorcow pushed a commit to tractorcow/jquery-validation that referenced this pull request May 15, 2014
tractorcow pushed a commit to tractorcow/jquery-validation that referenced this pull request May 15, 2014
tractorcow pushed a commit to tractorcow/jquery-validation that referenced this pull request May 15, 2014
tractorcow pushed a commit to tractorcow/jquery-validation that referenced this pull request May 20, 2014
jzaefferer pushed a commit that referenced this pull request May 21, 2014
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.

7 participants