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

API FIX Use aria-describedby and default error element changed to span #1083

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@tractorcow
Collaborator

tractorcow commented Apr 15, 2014

Non-label error labels will now work correctly, with aria-describedby now superseding dependency on the 'for' attribute

This provides a solution to the discussion noted in #900.

A lot of work had to be done to rewrite the test cases to respect the 'span' error label... apologies if I have made any incorrect assumptions about this module.

For legacy reasons, if the error element is set as a label explicitly, it still sets the 'for' attribute, but it isn't used anywhere in the module.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Apr 15, 2014

Collaborator

Travis fails with "too many var statements"... are variables obsolete now? 🍔

I'll look at this again tomorrow. :)

Collaborator

tractorcow commented Apr 15, 2014

Travis fails with "too many var statements"... are variables obsolete now? 🍔

I'll look at this again tomorrow. :)

Show outdated Hide outdated src/core.js
@@ -221,7 +221,7 @@ $.extend($.validator, {
rules: {},
errorClass: "error",
validClass: "valid",
errorElement: "label",
errorElement: "span",

This comment has been minimized.

@staabm

staabm Apr 15, 2014

Member

changing the default reults in a BC break and should therefore be noted somewhere (if the BC is accepted by project owners)

@staabm

staabm Apr 15, 2014

Member

changing the default reults in a BC break and should therefore be noted somewhere (if the BC is accepted by project owners)

This comment has been minimized.

@tractorcow

tractorcow Apr 17, 2014

Collaborator

I've changed this back to label by default.

@tractorcow

tractorcow Apr 17, 2014

Collaborator

I've changed this back to label by default.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Apr 15, 2014

Collaborator

Thanks @tractorcow, this looks promising. Fixing the jshint warning should be easy enough, you can get those locally by running grunt.

In #900 I wrote those:

  • 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.

I don't see that fully implemented here. You're always setting aria-describedby, instead of making that depend on where the span is appended. I don't see any new tests for the other aspect.

Even with that addressed I'm not sure how to deal with the backwards compatibility. Changed the element type would break any CSS that's based on the element name (instead of the class etc.). Maybe an intermediate step would be to keep the default option value, while doing the other changes. Anyone who knows better can set the option to span (if not already in use). We'll stop setting the for-attribute either way. Thoughts?

Collaborator

jzaefferer commented Apr 15, 2014

Thanks @tractorcow, this looks promising. Fixing the jshint warning should be easy enough, you can get those locally by running grunt.

In #900 I wrote those:

  • 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.

I don't see that fully implemented here. You're always setting aria-describedby, instead of making that depend on where the span is appended. I don't see any new tests for the other aspect.

Even with that addressed I'm not sure how to deal with the backwards compatibility. Changed the element type would break any CSS that's based on the element name (instead of the class etc.). Maybe an intermediate step would be to keep the default option value, while doing the other changes. Anyone who knows better can set the option to span (if not already in use). We'll stop setting the for-attribute either way. Thoughts?

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Apr 15, 2014

Collaborator

Thanks for the feedback all! I think the API should work with existing labels, but I'll add a test to confirm this.

Do you think there is any harm in consistently applying the aria-describedby? It would imagine the resulting html of the above situation appear as below;

<input type="text" name="Email" id="Email" aria-describedby="Email-label" />
<label for="Email">
    Email <span id="Email-label" class="error">Email is invalid</span>
</label>

Also (which I think was discussed earlier) is that we need to respect existing non-error labels (as above), so we can't just assume that any label with for=elementid is an error holder. I'm not sure it's just enough to assume that label means it's the error holder, that's why I propose enforcing the aria-describedby property for all errors.

Maybe I have misunderstood the use case somewhat?

Collaborator

tractorcow commented Apr 15, 2014

Thanks for the feedback all! I think the API should work with existing labels, but I'll add a test to confirm this.

Do you think there is any harm in consistently applying the aria-describedby? It would imagine the resulting html of the above situation appear as below;

<input type="text" name="Email" id="Email" aria-describedby="Email-label" />
<label for="Email">
    Email <span id="Email-label" class="error">Email is invalid</span>
</label>

Also (which I think was discussed earlier) is that we need to respect existing non-error labels (as above), so we can't just assume that any label with for=elementid is an error holder. I'm not sure it's just enough to assume that label means it's the error holder, that's why I propose enforcing the aria-describedby property for all errors.

Maybe I have misunderstood the use case somewhat?

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Apr 15, 2014

Collaborator

I just realised that I will need to update all of the demo/ folder files, since the default element has changed.

@jzaefferer, I've updated the convention issues and added some test cases to demonstrate that this solution should work seamlessly with label elements, if the user so decides. This doesn't mean there's no breaking changes, as any users upgrading will either need to set errorElement to label or update their css / html.

I suggest to merge this into a major release, if you haven't released 1.12.1 yet, and make this a part of 1.13. How does this sound?

I also haven't squashed my commits; I'm not sure of the convention you prefer, if you like me to and are happy with the api changes and test cases then I'll do that. I figured that we should agree and lock down the functionality changes before I go and update the demo code, since it would be a waste of effort to rewrite them should we decide to tweak this api. :)

Collaborator

tractorcow commented Apr 15, 2014

I just realised that I will need to update all of the demo/ folder files, since the default element has changed.

@jzaefferer, I've updated the convention issues and added some test cases to demonstrate that this solution should work seamlessly with label elements, if the user so decides. This doesn't mean there's no breaking changes, as any users upgrading will either need to set errorElement to label or update their css / html.

I suggest to merge this into a major release, if you haven't released 1.12.1 yet, and make this a part of 1.13. How does this sound?

I also haven't squashed my commits; I'm not sure of the convention you prefer, if you like me to and are happy with the api changes and test cases then I'll do that. I figured that we should agree and lock down the functionality changes before I go and update the demo code, since it would be a waste of effort to rewrite them should we decide to tweak this api. :)

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Apr 16, 2014

Collaborator

Thanks for the updates. Sounds good so far, though I won't have time to check the details until next Tuesday (where I reserve a few hours each week for this plugin).

As for "always applying aria-describedby": The original discussion specifically concluded with not doing that. While I haven't tried it myself either way, I suspect that in your sample, the error label would get announced twice - or run into some other issue. So we shouldn't do that.

Regarding commits: I'm fine with rebasing them before I merge. Please make sure the changes itself are good though, so indent fixes should be done asap.

Collaborator

jzaefferer commented Apr 16, 2014

Thanks for the updates. Sounds good so far, though I won't have time to check the details until next Tuesday (where I reserve a few hours each week for this plugin).

As for "always applying aria-describedby": The original discussion specifically concluded with not doing that. While I haven't tried it myself either way, I suspect that in your sample, the error label would get announced twice - or run into some other issue. So we shouldn't do that.

Regarding commits: I'm fine with rebasing them before I merge. Please make sure the changes itself are good though, so indent fixes should be done asap.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Apr 16, 2014

Collaborator

Ok, well having thought about it, we can probably assume that anything with the appropriate error classes and the requisite 'for' can probably leave out the describedby attribute. I'll make a fix and submit it before the weekend.

Collaborator

tractorcow commented Apr 16, 2014

Ok, well having thought about it, we can probably assume that anything with the appropriate error classes and the requisite 'for' can probably leave out the describedby attribute. I'll make a fix and submit it before the weekend.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Apr 16, 2014

Collaborator

Thanks greatly for the feedback... I had some issues with the commitplease plugin failing with sourcetree. It was a fun morning. :) You've done some really neat things with node I must say!

I've taken on your suggestion and re-implemented this so it only conditionally adds the aria-describedby. If you are using a label as your errorElement, it'll simply follow the old behaviour. This means users who are upgrading should only have to change one option to get the same behaviour.

I've also added some additional test cases to ease my fears regarding how well the error element and other related labels will work. Everything seems to play well together. :)

Collaborator

tractorcow commented Apr 16, 2014

Thanks greatly for the feedback... I had some issues with the commitplease plugin failing with sourcetree. It was a fun morning. :) You've done some really neat things with node I must say!

I've taken on your suggestion and re-implemented this so it only conditionally adds the aria-describedby. If you are using a label as your errorElement, it'll simply follow the old behaviour. This means users who are upgrading should only have to change one option to get the same behaviour.

I've also added some additional test cases to ease my fears regarding how well the error element and other related labels will work. Everything seems to play well together. :)

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Apr 16, 2014

Collaborator

What if we don't change the default just yet? We can still make the change in tests etc. (setting the option or changing the default). That way there should be no compatibility issues with the next release and we can make the switch in the future. Doesn't have to be a 2.0 as long as there's some time to adjust from communicating the change to actually landing it.

Collaborator

jzaefferer commented Apr 16, 2014

What if we don't change the default just yet? We can still make the change in tests etc. (setting the option or changing the default). That way there should be no compatibility issues with the next release and we can make the switch in the future. Doesn't have to be a 2.0 as long as there's some time to adjust from communicating the change to actually landing it.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Apr 16, 2014

Collaborator

We could :) That way we could have less breaking changes... but still change the default at a later date. It will at least solve the issue for those using spans, while no one else notices a thing.

I'll go ahead and make the necessary changes. :P

Collaborator

tractorcow commented Apr 16, 2014

We could :) That way we could have less breaking changes... but still change the default at a later date. It will at least solve the issue for those using spans, while no one else notices a thing.

I'll go ahead and make the necessary changes. :P

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Apr 16, 2014

Collaborator

Should the bulk of the testing use the to-be-default span, or should we leave testing using the current default label?

Collaborator

tractorcow commented Apr 16, 2014

Should the bulk of the testing use the to-be-default span, or should we leave testing using the current default label?

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Apr 16, 2014

Collaborator

There is a rather frustrating bug in these test cases... testForm1clean has an error label as below.

<label id="errorFirstname" for="firstname" class="error">error for firstname</label>

The 'firstname' in the 'for' actually refers to the input element on another form. for only refers to elements using the name property if they are checkable... =/

Fixing this breaks heaps of the test cases, because they rely on this broken behaviour. XD Some rely on the validation rules from the other form interfering with this form. Copying the validation rules to this break others. Am I doing something wrong here?

The library itself is fine... it's just the test cases not behaving well. =/

Collaborator

tractorcow commented Apr 16, 2014

There is a rather frustrating bug in these test cases... testForm1clean has an error label as below.

<label id="errorFirstname" for="firstname" class="error">error for firstname</label>

The 'firstname' in the 'for' actually refers to the input element on another form. for only refers to elements using the name property if they are checkable... =/

Fixing this breaks heaps of the test cases, because they rely on this broken behaviour. XD Some rely on the validation rules from the other form interfering with this form. Copying the validation rules to this break others. Am I doing something wrong here?

The library itself is fine... it's just the test cases not behaving well. =/

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Apr 16, 2014

Collaborator

I also uncovered what might be a bug... but I'll have to look at this later.

If an element has a data-rule-required property, then you can't use .rules('remove', 'required').. This seems to only work with programatically set rules. I don't have the time to investigate this properly, but noting it here since I've found this. I might be wrong, since it could have just been my own fault for breaking something. :)

Collaborator

tractorcow commented Apr 16, 2014

I also uncovered what might be a bug... but I'll have to look at this later.

If an element has a data-rule-required property, then you can't use .rules('remove', 'required').. This seems to only work with programatically set rules. I don't have the time to investigate this properly, but noting it here since I've found this. I might be wrong, since it could have just been my own fault for breaking something. :)

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Apr 16, 2014

Collaborator

I've rewritten the test cases to be much less element-critical; Rather than selecting the error element by $("label.error") I'm using the less sensitive $(".error:not(input)"), which should work whether or not the element is a span.error or a label.error.

Hope the review goes well. :) Thanks for your time in helping look at this...

Collaborator

tractorcow commented Apr 16, 2014

I've rewritten the test cases to be much less element-critical; Rather than selecting the error element by $("label.error") I'm using the less sensitive $(".error:not(input)"), which should work whether or not the element is a span.error or a label.error.

Hope the review goes well. :) Thanks for your time in helping look at this...

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Apr 29, 2014

Collaborator

@jzaefferer Hope you're doing well. :) Have you been able to look at this issue again? Sorry for needlessly bothering you; I'm just keen to get this resolved for good. :)

Collaborator

tractorcow commented Apr 29, 2014

@jzaefferer Hope you're doing well. :) Have you been able to look at this issue again? Sorry for needlessly bothering you; I'm just keen to get this resolved for good. :)

Show outdated Hide outdated src/core.js
var label = this.errorsFor( element );
var label = this.errorsFor( element ),
place,
group;

This comment has been minimized.

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

Can you change this to match the assignment rules in the jQuery Style Guide? http://contribute.jquery.org/style-guide/js/#assignments

var place, group,
  label = ...;

(using tabs, not spaces)

The existing code has a lot of issues, but we can at least fix anything we're changing anyway.

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

Can you change this to match the assignment rules in the jQuery Style Guide? http://contribute.jquery.org/style-guide/js/#assignments

var place, group,
  label = ...;

(using tabs, not spaces)

The existing code has a lot of issues, but we can at least fix anything we're changing anyway.

Show outdated Hide outdated src/core.js
@@ -711,7 +711,9 @@ $.extend($.validator, {
},
showLabel: function( element, message ) {
var label = this.errorsFor( element );
var label = this.errorsFor( element ),

This comment has been minimized.

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

Let's rename this variable, maybe error? Or errorElement? output?

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

Let's rename this variable, maybe error? Or errorElement? output?

This comment has been minimized.

@staabm

staabm Apr 29, 2014

Member

should be plural... errors or errorElements?

@staabm

staabm Apr 29, 2014

Member

should be plural... errors or errorElements?

This comment has been minimized.

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

Code below expects this to be a single element with an id-attribute. I'm not sure if errorsFor ever returns more then one element - maybe we should rename it to errorFor instead?

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

Code below expects this to be a single element with an id-attribute. I'm not sure if errorsFor ever returns more then one element - maybe we should rename it to errorFor instead?

Show outdated Hide outdated src/core.js
@@ -720,19 +722,40 @@ $.extend($.validator, {
} else {
// create label
label = $("<" + this.settings.errorElement + ">")
.attr("for", this.idOrName(element))
.attr("id", this.idOrName(element) + "-label")

This comment has been minimized.

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

As above, but spacing:

.attr( "id", this.idOrName( element ) + "-label" )
@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

As above, but spacing:

.attr( "id", this.idOrName( element ) + "-label" )
Show outdated Hide outdated src/core.js
.addClass(this.settings.errorClass)
.html(message || "");
// Place label

This comment has been minimized.

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

What does this mean?

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

What does this mean?

This comment has been minimized.

@tractorcow

tractorcow Apr 30, 2014

Collaborator

At some point its possible for the error element to be wrapped. While we need to maintain a reference to the error element itself (e.g. to set the 'for' for labels, and so on), we also need to know which element we need to place into the HTML. By default (without a wrapped element) these reference the same item.

I'll make the comment actually describe what this means.

@tractorcow

tractorcow Apr 30, 2014

Collaborator

At some point its possible for the error element to be wrapped. While we need to maintain a reference to the error element itself (e.g. to set the 'for' for labels, and so on), we also need to know which element we need to place into the HTML. By default (without a wrapped element) these reference the same item.

I'll make the comment actually describe what this means.

Show outdated Hide outdated src/core.js
}
if ( !this.labelContainer.append(label).length ) {
if ( !this.labelContainer.append(place).length ) {

This comment has been minimized.

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

This line is weird. Why bother calling append if the length is zero? Let's fix that now, too.

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

This line is weird. Why bother calling append if the length is zero? Let's fix that now, too.

Show outdated Hide outdated src/core.js
if ( this.settings.errorPlacement ) {
this.settings.errorPlacement(label, $(element) );
this.settings.errorPlacement(place, $(element) );

This comment has been minimized.

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

More spacing issues, as above, more below.

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

More spacing issues, as above, more below.

Show outdated Hide outdated src/core.js
return this.errors().filter("[for='" + name + "']");
} else if ( describer ) {
// Non-label elements should use aria-describedby
return this.errors().filter("#" + describer);

This comment has been minimized.

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

Using IDs to filter seems wrong. If there's the possibility that the ID is not unique, we should address that instead, then select by ID directly, e.g. $( "#" + describer )

@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

Using IDs to filter seems wrong. If there's the possibility that the ID is not unique, we should address that instead, then select by ID directly, e.g. $( "#" + describer )

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Apr 29, 2014

Collaborator

This is starting to look pretty good, I've only found minor issues. I haven't spend enough time with the tests though, will do that after these are addressed.

Let me know and I'll review again immediately.

Collaborator

jzaefferer commented Apr 29, 2014

This is starting to look pretty good, I've only found minor issues. I haven't spend enough time with the tests though, will do that after these are addressed.

Let me know and I'll review again immediately.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Apr 30, 2014

Collaborator

Thanks so much for the review @jzaefferer

I've renamed the error element variable to simply "error". We probably can't rename errorsFor without breaking backwards compatibility, so I've left it as is.

Style guidelines and a few more have been tidied up. Hopefully I'm learning to do things the right way! :)

Collaborator

tractorcow commented Apr 30, 2014

Thanks so much for the review @jzaefferer

I've renamed the error element variable to simply "error". We probably can't rename errorsFor without breaking backwards compatibility, so I've left it as is.

Style guidelines and a few more have been tidied up. Hopefully I'm learning to do things the right way! :)

Show outdated Hide outdated src/core.js
// replace message on existing label
label.html(message);
error.html(message);
} else {
// create label

This comment has been minimized.

@jzaefferer

jzaefferer Apr 30, 2014

Collaborator

Lets change this, too:

// Create error element
@jzaefferer

jzaefferer Apr 30, 2014

Collaborator

Lets change this, too:

// Create error element
Show outdated Hide outdated src/core.js
}
// Link error back to the element
if ( /label/.test( this.settings.errorElement) ) {

This comment has been minimized.

@jzaefferer

jzaefferer Apr 30, 2014

Collaborator

I wonder if this should be more strict, e.g. /^label$/. That seems sane, since the errors method would break if someone tried to put attributes in the option.

@jzaefferer

jzaefferer Apr 30, 2014

Collaborator

I wonder if this should be more strict, e.g. /^label$/. That seems sane, since the errors method would break if someone tried to put attributes in the option.

Show outdated Hide outdated src/core.js
});
var name = this.idOrName( element ),
describer = $( element ).attr( "aria-describedby" );
if ( /label/.test( this.settings.errorElement ) ) {

This comment has been minimized.

@jzaefferer

jzaefferer Apr 30, 2014

Collaborator

Same regex as above, let's fix both (if at all).

@jzaefferer

jzaefferer Apr 30, 2014

Collaborator

Same regex as above, let's fix both (if at all).

Show outdated Hide outdated test/test.js
form.validate({
errorPlacement: function(error) {
form.find("label[for='testForm16text']").append(error);

This comment has been minimized.

@jzaefferer

jzaefferer Apr 30, 2014

Collaborator

errorPlacement also receives the element as an argument - can you adjust this test to use that? More on that in my next comment...

@jzaefferer

jzaefferer Apr 30, 2014

Collaborator

errorPlacement also receives the element as an argument - can you adjust this test to use that? More on that in my next comment...

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Apr 30, 2014

Collaborator

Could you add a demo for "show error message inside existing label", using errorPlacement properly (not quite like in the test right now), so that it works for multiple elements?

I looked at the radio-checkbox-select-demo.html demo to modify that, but its pretty broken, always overwriting the messages specified in the markup. It has likely been broken for a long time. Might as well ignore it a little longer, though if you're so inclined, I'd appreciate the help.

All in all, one comment, two regexes, one test, one demo, and this should be good to land.

Collaborator

jzaefferer commented Apr 30, 2014

Could you add a demo for "show error message inside existing label", using errorPlacement properly (not quite like in the test right now), so that it works for multiple elements?

I looked at the radio-checkbox-select-demo.html demo to modify that, but its pretty broken, always overwriting the messages specified in the markup. It has likely been broken for a long time. Might as well ignore it a little longer, though if you're so inclined, I'd appreciate the help.

All in all, one comment, two regexes, one test, one demo, and this should be good to land.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Apr 30, 2014

Collaborator

Hi @jzaefferer , all done as asked.

I also decided to change the default ID for the error message from whateverelementid-label to whateverelemnetid-error to match the change in variable name. I think it better distinguishes the ID from any label concept.

The demo seems to work pretty well, but I don't know how to link it directly from github so you'll need to check it out to test. It makes sense sometimes to show the field label / error as a single multi-styled string, so I believe the use case is very well presented. :)

I'm really enjoying your feedback, so please keep it coming. Just so you know tomorrow we are holding a hackfest event for Silverstripe, and we are keen to include this update in our upcoming release. :) So you can see I have purely selfish motives in getting this out the door. 🌾

Collaborator

tractorcow commented Apr 30, 2014

Hi @jzaefferer , all done as asked.

I also decided to change the default ID for the error message from whateverelementid-label to whateverelemnetid-error to match the change in variable name. I think it better distinguishes the ID from any label concept.

The demo seems to work pretty well, but I don't know how to link it directly from github so you'll need to check it out to test. It makes sense sometimes to show the field label / error as a single multi-styled string, so I believe the use case is very well presented. :)

I'm really enjoying your feedback, so please keep it coming. Just so you know tomorrow we are holding a hackfest event for Silverstripe, and we are keen to include this update in our upcoming release. :) So you can see I have purely selfish motives in getting this out the door. 🌾

Show outdated Hide outdated src/core.js
error.attr( "for", this.idOrName( element ) );
} else {
// In case id field is modified by the custom errorPlacement method
$( element ).attr( "aria-describedby", error.attr( "id" ) );

This comment has been minimized.

@jzaefferer

jzaefferer May 3, 2014

Collaborator

Thanks for adding the demo. That helped me verify the one thing still missing here. Adding aria-describedby is supposed to only happen when the error isn't added to the label, to avoid "duplicate content".

From #900:

Change default error element to , associate that to the input using aria-describedby, except when its appended to an existing label element (leave it out then).

I guess the only way to know where the error element ends up is to check its ascendents, since we can't tell what errorPlacement will do. Though I suppose we only have to do that when settings.errorPlacement is used, since that's the only case where it might end up inside the label.

@jzaefferer

jzaefferer May 3, 2014

Collaborator

Thanks for adding the demo. That helped me verify the one thing still missing here. Adding aria-describedby is supposed to only happen when the error isn't added to the label, to avoid "duplicate content".

From #900:

Change default error element to , associate that to the input using aria-describedby, except when its appended to an existing label element (leave it out then).

I guess the only way to know where the error element ends up is to check its ascendents, since we can't tell what errorPlacement will do. Though I suppose we only have to do that when settings.errorPlacement is used, since that's the only case where it might end up inside the label.

This comment has been minimized.

@tractorcow

tractorcow May 4, 2014

Collaborator

I see, I can probably do that. It is probably enough to do it with a related-label-child-selector query.

My question is, what is the potential harm in not always explicitly applying the aria-describedby? Without it you are increasing the number of paths between the input and the label. It's now either by associated label (directly), but aria-describedby (directly) or as the descendent of an associated non-error label (indirectly). With each additional case the code becomes more complicated and harder to read. Maybe I can work some magic and find a simpler solution that meets each of the above needs...

Another case you didn't consider is when there is no errorPlacement, and the label is already present in the HTML.

@tractorcow

tractorcow May 4, 2014

Collaborator

I see, I can probably do that. It is probably enough to do it with a related-label-child-selector query.

My question is, what is the potential harm in not always explicitly applying the aria-describedby? Without it you are increasing the number of paths between the input and the label. It's now either by associated label (directly), but aria-describedby (directly) or as the descendent of an associated non-error label (indirectly). With each additional case the code becomes more complicated and harder to read. Maybe I can work some magic and find a simpler solution that meets each of the above needs...

Another case you didn't consider is when there is no errorPlacement, and the label is already present in the HTML.

Show outdated Hide outdated src/core.js
if ( group ) {
$.each( this.groups, function( name, testgroup ) {
if ( testgroup === group ) {
$( "[name='" + name + "']" ).attr( "aria-describedby", error.attr( "id" ) );

This comment has been minimized.

@staabm

staabm May 3, 2014

Member

This assumes page wide unqiue fieldnames.. This should be scoped to the currentForm

@staabm

staabm May 3, 2014

Member

This assumes page wide unqiue fieldnames.. This should be scoped to the currentForm

This comment has been minimized.

@staabm

staabm May 3, 2014

Member

This also looks like it wont work when name contains a '

@staabm

staabm May 3, 2014

Member

This also looks like it wont work when name contains a '

This comment has been minimized.

@tractorcow

tractorcow May 4, 2014

Collaborator

I see, I didn't realise that ' was a valid html input name character.

I'll just rewrite it with a javascript condition instead of a jquery selector.

@tractorcow

tractorcow May 4, 2014

Collaborator

I see, I didn't realise that ' was a valid html input name character.

I'll just rewrite it with a javascript condition instead of a jquery selector.

This comment has been minimized.

@staabm

staabm May 4, 2014

Member

Just did a quick search.. You are right, ' is not a valid char within the name attribute.

@staabm

staabm May 4, 2014

Member

Just did a quick search.. You are right, ' is not a valid char within the name attribute.

This comment has been minimized.

@tractorcow

tractorcow May 4, 2014

Collaborator

Well, it needs to be rewritten anyway. :) I'll keep it in mind.

@tractorcow

tractorcow May 4, 2014

Collaborator

Well, it needs to be rewritten anyway. :) I'll keep it in mind.

This comment has been minimized.

@tractorcow

tractorcow May 7, 2014

Collaborator

There is some code at http://stackoverflow.com/questions/2786538/need-to-escape-a-special-character-in-a-jquery-selector-string that would allow us to properly escape all attribute selectors, but I feel it would probably be a separate job to replace this behaviour across the whole module. Perhaps this could be a separate pull request if the devs feel this is necessary?

@tractorcow

tractorcow May 7, 2014

Collaborator

There is some code at http://stackoverflow.com/questions/2786538/need-to-escape-a-special-character-in-a-jquery-selector-string that would allow us to properly escape all attribute selectors, but I feel it would probably be a separate job to replace this behaviour across the whole module. Perhaps this could be a separate pull request if the devs feel this is necessary?

This comment has been minimized.

@tractorcow

tractorcow May 7, 2014

Collaborator

I've now scoped this query against this.currentForm

@tractorcow

tractorcow May 7, 2014

Collaborator

I've now scoped this query against this.currentForm

Show outdated Hide outdated src/core.js
}
// Link error back to the element
if ( /^label$/.test( this.settings.errorElement) ) {

This comment has been minimized.

@staabm

staabm May 3, 2014

Member

Why do the regex here? We could either fo .is('label') or even use tagName instead?

@staabm

staabm May 3, 2014

Member

Why do the regex here? We could either fo .is('label') or even use tagName instead?

This comment has been minimized.

@tractorcow

tractorcow May 4, 2014

Collaborator

That was copied from the original solution... I'll make the suggested improvement, thanks! :)

@tractorcow

tractorcow May 4, 2014

Collaborator

That was copied from the original solution... I'll make the suggested improvement, thanks! :)

This comment has been minimized.

@jzaefferer

jzaefferer May 6, 2014

Collaborator

Now that I look at it again, /^label$/.test( this.settings.errorElement) could also be written as this.settings.errorElement === "label". Though error.is( "label" ) seems good.

@jzaefferer

jzaefferer May 6, 2014

Collaborator

Now that I look at it again, /^label$/.test( this.settings.errorElement) could also be written as this.settings.errorElement === "label". Though error.is( "label" ) seems good.

This comment has been minimized.

@tractorcow

tractorcow May 7, 2014

Collaborator

I used error.is( "label" );

@tractorcow

tractorcow May 7, 2014

Collaborator

I used error.is( "label" );

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow May 7, 2014

Collaborator

Hey @jzaefferer I hope I've met all concerns! Now the aria-describedby doesn't appear if we are adding labels, or nesting error elements within labels. I've updated the test case to explicitly assert that the aria-describedby is not added.

Collaborator

tractorcow commented May 7, 2014

Hey @jzaefferer I hope I've met all concerns! Now the aria-describedby doesn't appear if we are adding labels, or nesting error elements within labels. I've updated the test case to explicitly assert that the aria-describedby is not added.

Show outdated Hide outdated demo/errors-within-labels.html
$().ready(function() {
// validate the form when it is submitted
var validator = $("#form1").validate({
errorLabel: 'span',

This comment has been minimized.

@staabm

staabm May 7, 2014

Member

Why is the label "span"?

@staabm

staabm May 7, 2014

Member

Why is the label "span"?

This comment has been minimized.

@tractorcow

tractorcow May 7, 2014

Collaborator

Because the error will be nested within another label, so we want to avoid labels nested within one another; It's not necessary nor good practice.

@tractorcow

tractorcow May 7, 2014

Collaborator

Because the error will be nested within another label, so we want to avoid labels nested within one another; It's not necessary nor good practice.

This comment has been minimized.

@staabm

staabm May 7, 2014

Member

Isn't errorElement used for that? This seems like a leftover/typo? I think we dont have a errorLabel option in validate()

@staabm

staabm May 7, 2014

Member

Isn't errorElement used for that? This seems like a leftover/typo? I think we dont have a errorLabel option in validate()

This comment has been minimized.

@tractorcow

tractorcow May 7, 2014

Collaborator

Er, that's a good point. I wonder why it was working at all?

@tractorcow

tractorcow May 7, 2014

Collaborator

Er, that's a good point. I wonder why it was working at all?

This comment has been minimized.

@staabm

staabm May 8, 2014

Member

I think it works because you define errorElement a few lines below

@staabm

staabm May 8, 2014

Member

I think it works because you define errorElement a few lines below

Show outdated Hide outdated demo/errors-within-labels.html
errorLabel: 'span',
errorPlacement: function(error, element) {
// Append error within linked label
$( "label[for='" + element.attr( "id" ) + "']" ).append( error );

This comment has been minimized.

@staabm

staabm May 7, 2014

Member

Should this also be scoped to the currentForm?

@staabm

staabm May 7, 2014

Member

Should this also be scoped to the currentForm?

This comment has been minimized.

@tractorcow

tractorcow May 7, 2014

Collaborator

Maybe, although there's only one form in this case, so it's probably not necessary. It would only be a problem if in this specific example there were multiple forms with conflicting element IDs (which should be unique anyway within the document). Element names can be non-distinct though, although that's not what we're matching against. :)

@tractorcow

tractorcow May 7, 2014

Collaborator

Maybe, although there's only one form in this case, so it's probably not necessary. It would only be a problem if in this specific example there were multiple forms with conflicting element IDs (which should be unique anyway within the document). Element names can be non-distinct though, although that's not what we're matching against. :)

This comment has been minimized.

@staabm

staabm May 7, 2014

Member

Since this is a demo file, i think it should be implemented in the most optimal way - people will look at it and copy/paste things ;-). I agree its not necessary from a technical point of view

@staabm

staabm May 7, 2014

Member

Since this is a demo file, i think it should be implemented in the most optimal way - people will look at it and copy/paste things ;-). I agree its not necessary from a technical point of view

This comment has been minimized.

@tractorcow

tractorcow May 7, 2014

Collaborator

Fair enough, I'll update it. :)

Glad to have the thorough critique! ❤️

@tractorcow

tractorcow May 7, 2014

Collaborator

Fair enough, I'll update it. :)

Glad to have the thorough critique! ❤️

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow May 8, 2014

Collaborator

I've updated it again; Thanks @staabm for the feedback.

Collaborator

tractorcow commented May 8, 2014

I've updated it again; Thanks @staabm for the feedback.

Show outdated Hide outdated demo/errors-within-labels.html
errorPlacement: function(error, element) {
// Append error within linked label
$( element )
.parents( "form" )

This comment has been minimized.

@staabm

staabm May 11, 2014

Member

Shouldn't this be closest instead? Nested forms are not that common but new browsers support them and this might lead to things you don't want here

@staabm

staabm May 11, 2014

Member

Shouldn't this be closest instead? Nested forms are not that common but new browsers support them and this might lead to things you don't want here

This comment has been minimized.

@tractorcow

tractorcow May 11, 2014

Collaborator

Done.

@tractorcow

tractorcow May 11, 2014

Collaborator

Done.

@staabm

This comment has been minimized.

Show comment
Hide comment
@staabm

staabm May 11, 2014

Member

@tractorcow thanks.

@jzaefferer for my eyes this seems good to go

Member

staabm commented May 11, 2014

@tractorcow thanks.

@jzaefferer for my eyes this seems good to go

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow May 11, 2014

Collaborator

Thanks @jzaefferer and @staabm for the hard work reviewing my code. It's been an educational experience for me!

Collaborator

tractorcow commented May 11, 2014

Thanks @jzaefferer and @staabm for the hard work reviewing my code. It's been an educational experience for me!

// If no describer is used then errors are either associated labels, or children of non-error labels
return this
.errors()
.filter( "label[for='" + name + "'], label[for='" + name + "'] *" );

This comment has been minimized.

@jzaefferer

jzaefferer May 12, 2014

Collaborator

Which of the tests, new or old, covers the second branch here, the children of non-error labels?

@jzaefferer

jzaefferer May 12, 2014

Collaborator

Which of the tests, new or old, covers the second branch here, the children of non-error labels?

This comment has been minimized.

@tractorcow

tractorcow May 15, 2014

Collaborator

See testForm16 in index.html. It's a new one recently added. The test has the title "test label used as error container".

@tractorcow

tractorcow May 15, 2014

Collaborator

See testForm16 in index.html. It's a new one recently added. The test has the title "test label used as error container".

Show outdated Hide outdated test/test.js
@@ -1357,6 +1357,141 @@ test("validate input with no type attribute, defaulting to text", function() {
errors(0);
});
test("test label used as error container", function() {

This comment has been minimized.

@jzaefferer

jzaefferer May 12, 2014

Collaborator

Can you move these new tests to a separate file, error-placement.js and add that to the index for the testsuite? A first step in splitting apart this monstrous test.js.

Also here and in a few places below, can you fix the whitespacing?

@jzaefferer

jzaefferer May 12, 2014

Collaborator

Can you move these new tests to a separate file, error-placement.js and add that to the index for the testsuite? A first step in splitting apart this monstrous test.js.

Also here and in a few places below, can you fix the whitespacing?

This comment has been minimized.

@tractorcow

tractorcow May 15, 2014

Collaborator

No problem... I realise I wasn't as thorough with the test.js as I should have been, but I'll fix that now.

@tractorcow

tractorcow May 15, 2014

Collaborator

No problem... I realise I wasn't as thorough with the test.js as I should have been, but I'll fix that now.

Show outdated Hide outdated test/test.js
field = $("#testForm16text");
form.validate({
errorElement: "label" // When this is no longer the default

This comment has been minimized.

@jzaefferer

jzaefferer May 12, 2014

Collaborator

Comments go before the line they're commenting, so in this case we can just remove the comment. As long as we have special behaviour for label, we will keep the test, the default isn't that important.

@jzaefferer

jzaefferer May 12, 2014

Collaborator

Comments go before the line they're commenting, so in this case we can just remove the comment. As long as we have special behaviour for label, we will keep the test, the default isn't that important.

Show outdated Hide outdated test/test.js
ok( !field.valid() );
equal( 1, field.next("label.error[for='username']").length );
equal( "missing", field.next("label.error[for='username']").text() );
ok( field.next("label.error[for='username']").is(":visible") );

This comment has been minimized.

@jzaefferer

jzaefferer May 12, 2014

Collaborator

We repeat these three assertions a lot around here. If you're willing to spend a bit more time on this PR, let's extract these into a helper function or even a custom assertion. Maybe errorFor( "username", "missing" ), which then checks the length, text and visibility.

@jzaefferer

jzaefferer May 12, 2014

Collaborator

We repeat these three assertions a lot around here. If you're willing to spend a bit more time on this PR, let's extract these into a helper function or even a custom assertion. Maybe errorFor( "username", "missing" ), which then checks the length, text and visibility.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer May 12, 2014

Collaborator

Sorry to ruin the party, not quite done, yet ;) Still, I'm looking forward to landing this, I intend to release 1.13 shortly after to get it live, too.

Collaborator

jzaefferer commented May 12, 2014

Sorry to ruin the party, not quite done, yet ;) Still, I'm looking forward to landing this, I intend to release 1.13 shortly after to get it live, too.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow May 12, 2014

Collaborator

No problem about that, I also want this to be the best out can be. If there
are still things to improve let's improve them.
On May 12, 2014 9:27 PM, "Jörn Zaefferer" notifications@github.com wrote:

Sorry to ruin the party, not quite done, yet ;) Still, I'm looking forward
to landing this, I intend to release 1.13 shortly after to get it live, too.


Reply to this email directly or view it on GitHubhttps://github.com/jzaefferer/jquery-validation/pull/1083#issuecomment-42811979
.

Collaborator

tractorcow commented May 12, 2014

No problem about that, I also want this to be the best out can be. If there
are still things to improve let's improve them.
On May 12, 2014 9:27 PM, "Jörn Zaefferer" notifications@github.com wrote:

Sorry to ruin the party, not quite done, yet ;) Still, I'm looking forward
to landing this, I intend to release 1.13 shortly after to get it live, too.


Reply to this email directly or view it on GitHubhttps://github.com/jzaefferer/jquery-validation/pull/1083#issuecomment-42811979
.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow May 15, 2014

Collaborator

Hey @jzaefferer, I've done as requested.

Tests to do with error placement are now in error-placement.js

I've done a massive amount of code-quality cleanup, both in tests and in core.js, as per the coding conventions that are in place.

I've made some custom assertions and put them in test.js as below

// Asserts that there is a visible error with the given text for the specified element
QUnit.assert.hasError = function( element, text, message ) {
    var errors = $( element ).closest( "form" ).validate().errorsFor( element[ 0 ] ),
        actual = ( errors.length === 1 && errors.is( ":visible" ) ) ? errors.text() : "";
    QUnit.push(actual, actual, text, message);
};

// Asserts that there is no visible error for the given element
QUnit.assert.noErrorFor = function( element, message ) {
    var errors = $( element ).closest( "form" ).validate().errorsFor( element[ 0 ] ),
        hidden = ( errors.length === 0 ) || errors.is( ":hidden" ) || ( errors.text() === "" );
    QUnit.push(hidden, hidden, true, message );
};

I haven't gone over ALL tests to replace these; It's just too big a task for tonight. I've demonstrated their usefulness in error-placement.js at least.

I hope this is good enough to merge finally. ;)

Collaborator

tractorcow commented May 15, 2014

Hey @jzaefferer, I've done as requested.

Tests to do with error placement are now in error-placement.js

I've done a massive amount of code-quality cleanup, both in tests and in core.js, as per the coding conventions that are in place.

I've made some custom assertions and put them in test.js as below

// Asserts that there is a visible error with the given text for the specified element
QUnit.assert.hasError = function( element, text, message ) {
    var errors = $( element ).closest( "form" ).validate().errorsFor( element[ 0 ] ),
        actual = ( errors.length === 1 && errors.is( ":visible" ) ) ? errors.text() : "";
    QUnit.push(actual, actual, text, message);
};

// Asserts that there is no visible error for the given element
QUnit.assert.noErrorFor = function( element, message ) {
    var errors = $( element ).closest( "form" ).validate().errorsFor( element[ 0 ] ),
        hidden = ( errors.length === 0 ) || errors.is( ":hidden" ) || ( errors.text() === "" );
    QUnit.push(hidden, hidden, true, message );
};

I haven't gone over ALL tests to replace these; It's just too big a task for tonight. I've demonstrated their usefulness in error-placement.js at least.

I hope this is good enough to merge finally. ;)

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow May 15, 2014

Collaborator

For your reference, I've been following the below link for style conformance.

http://contribute.jquery.org/style-guide/js/

If I'm making any errors please let me know how I can follow this properly. I really hope this is acceptable.

🐱

Collaborator

tractorcow commented May 15, 2014

For your reference, I've been following the below link for style conformance.

http://contribute.jquery.org/style-guide/js/

If I'm making any errors please let me know how I can follow this properly. I really hope this is acceptable.

🐱

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer May 20, 2014

Collaborator

This is getting quite epic. Unfortunately there's still some more work to do here:

  • Using QUnit.assert to add custom assertions is correct. To make use of those custom assertion, you should use the assert argument in the test callback: test( name, function( assert ) { assert.noErrorFor( ... ); });
  • Trying to merge this with master results in merge conflicts in src/core.js and test/test.js, likely due to the whitespace changes. Could you do a rebase and look into resolving those?

Thanks again for all your work on this.

Collaborator

jzaefferer commented May 20, 2014

This is getting quite epic. Unfortunately there's still some more work to do here:

  • Using QUnit.assert to add custom assertions is correct. To make use of those custom assertion, you should use the assert argument in the test callback: test( name, function( assert ) { assert.noErrorFor( ... ); });
  • Trying to merge this with master results in merge conflicts in src/core.js and test/test.js, likely due to the whitespace changes. Could you do a rebase and look into resolving those?

Thanks again for all your work on this.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow May 20, 2014

Collaborator

Hey @jzaefferer thanks for your patience.

I've fixed the assert usage and resolved the merge conflicts.

Collaborator

tractorcow commented May 20, 2014

Hey @jzaefferer thanks for your patience.

I've fixed the assert usage and resolved the merge conflicts.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow May 20, 2014

Collaborator

Green tick! 🏁

Collaborator

tractorcow commented May 20, 2014

Green tick! 🏁

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow May 21, 2014

Collaborator

We have a big release due next week and I'm anxious to get this included... hope that this time the review is all spot on. ;)

Collaborator

tractorcow commented May 21, 2014

We have a big release due next week and I'm anxious to get this included... hope that this time the review is all spot on. ;)

@staabm

This comment has been minimized.

Show comment
Hide comment
@staabm

staabm May 21, 2014

Member

yay, great job @tractorcow 👏

Member

staabm commented May 21, 2014

yay, great job @tractorcow 👏

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer May 21, 2014

Collaborator

Yes, all good. Thank you!

Collaborator

jzaefferer commented May 21, 2014

Yes, all good. Thank you!

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow May 21, 2014

Collaborator

yay! Thanks so much @jzaefferer and @staabm too ;)

Collaborator

tractorcow commented May 21, 2014

yay! Thanks so much @jzaefferer and @staabm too ;)

@tractorcow tractorcow deleted the tractorcow:pulls/aria-describedby branch May 21, 2014

@nschonni nschonni added this to the 1.12.1 milestone May 22, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment