-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Conversation
A 'for' on other types causes problems for screen readers, and is not valid HTML
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 ) ) { |
There was a problem hiding this comment.
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
@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; |
@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. |
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. |
I wrote this a while ago but never posted it here: This breaks the How about a 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. |
@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. |
@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. |
@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. |
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. |
@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). |
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. |
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 However, we can't add new settings to jquery-validation, so my alternate solution would be to hardcode in the 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 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. |
Please correct me if any of the following assumptions are wrong:
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? |
@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. |
Thanks for the input everyone. Based on the discussion so far, I suggest this approach:
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? |
@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" /> |
@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). |
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. |
@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 |
Alright, with those modifications, the plan should be this:
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. |
@jzaefferer That works for me. Thanks! |
Non-label error labels will now work correctly, with aria-describedby now superseding dependency on the 'for' attribute Fixes jquery-validation#900
Hi @jzaefferer , would you mind looking at my suggested solution at #1083 please? :) Thank you! |
Non-label error labels will now work correctly, with aria-describedby now superseding dependency on the 'for' attribute Fixes jquery-validation#900
Closing this since we're making good progress in #1083. |
A 'for' on other types causes problems for screen readers, and is not valid HTML