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

I'll look at this again tomorrow. :)

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?

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?

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. :)

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.

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.

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. :)

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.

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

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

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. =/

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. :)

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

@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. :)

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.

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! :)

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.

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

@tractorcow thanks.

@jzaefferer for my eyes this seems good to go

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

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.

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//pull/1083#issuecomment-42811979
.

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. ;)

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.

🐱

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.

Hey @jzaefferer thanks for your patience.

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

Green tick! 🏁

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. ;)

yay, great job @tractorcow 👏

Yes, all good. Thank you!

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